HomePhabricator

Performance (1/2): Remove unused stylesheets

Authored by Krinkle on May 30 2020, 4:44 PM.

Description

Performance (1/2): Remove unused stylesheets

This reduces the main CSS payload size from 367KB to 217KB.
(For skin.wikimediaapiportal.styles|zzz.ext.bootstrap.styles)

Summary

  • Chameleon stylesheet:

    Except for about 5 lines of code, none of the Chameleon styles were needed.
  • Chameleon HTML template framework:

    The used components were either locally created already, or subclassed an upstream component only to disabled or skip everything it provided – leaving only the local customisation and what MW does by default already.

    This HTML framework added significant complexity and indirection, thus presumably being quite hard to maintain and debug for us and our contractors. This indirection did not actually bring reduction in the amount of code and complexity needed to build components – a core value I would have expected Chameleon to provide.

    In addition to providing very little of the value I expect a template framework to bring, it was also in quite poor condition in terms of code quality, and best practices in terms security and performance. I estimate it would take months to get into a decent shape. If Wikimedia had not already adopted and invested in an HTML template engine since 2015, there may've been something here to take inspiration from. However, that is not the case.

    I've converted the components that were local to this skin to Mustache, which reduced the amount of code needed drastically. To my surprise this was much easier than I expected, as there was essentially no logic inherited at all. All needed logic was either already custom to this skin, or already available in MW by default.

Style module structure

  • The skin registered multiple modules for the same delivery endpoint ("JS for all pages"). I've made these ship via the same module bundle rather than multiple.

    FIXME: Move files into subdirectories that match their bundle names (per T193826).
  • The skin has multiple LESS entrypoints which is hiding a performance issue.

    FIXME: It is generally expected to place these in the LESS file instead.

    For example, rather than a module with 5 LESS files than 5 media queries, one would have one LESS file (e.g. styles.less) where the media query blocks would reside, with their respective @import statements.

    Doing so would also make it more obvious than currently that this is currently shipping 3-4 copies of most styles. (See next point).
  • The skin loads the common.less hierarchy four times.

    FIXME: This should probably be included as the first file without a media block, rather than including from each media block separately.
  • The skin loads the xl.less hierarchy twice.

    FIXME: Either adjust 'media' for lg.less to include the xl-media query (and remove the manual code duplication currently between lg and xl), ... or, acknowledge that they are meant to be separate, keep bits currently duplicated, and remove the include of 'xl' from 'lg'.

Chameleon stylesheet

  • Remove specials/SpecialSearch. It had one unused style rule (for #mw-search-toggleall, used only by "Advanced" tab on Special:Search). However this seems an unintentional style rule, as the button it styles looks out of place with the other blue buttons. I assume this is still a TODO to locally override, so seems fine either way.
  • Remove Components/NewtalkNotifier. Not used in this layout.
  • Remove utilities/sticky. Not used in this layout.
  • Remove utilities/icons. Not used in this layout.
  • Remove utilities/float. Deprecated. Not used in this layout.
  • Remove _print. Mostly redundant with core's print styles from 'mediawiki.skinning.content'. Also looks like print use case wasn't finished/tested yet, so I've added this one line of code to this skin, and also added missing display toggles for wm-personal-tools, wm-notification, and wm-footer-links.
  • Remove _mw-elements. Not used in this layout, or redundant with core styles already loaded.
  • Remove _fixes.
    • "alert" is not used and would likely have the wrong style.
    • "rtl dropdown" (kept), although FIXME: dropdown is broken either way in RTL.
  • Remove Components/SearchBar.scss. Its styles were unused or overridden.
  • Remove Components/PersonalTools. Not used in this layout.
  • Remove Components/FooterComponents. Not used in this layout.
  • Remove Components/PageTools. Styles don't apply to this layout.
  • Remove Components/MainContent. Styles were redundant, unused, or already overriden.
  • Remove Components/NavbarHorizontal.
    • "navbar-light" was redundant, as overridden.
    • "navbar" is used but is identical to the default, use this class name directly in the component.
    • link colors redundant, locally overriden.
    • "dropdown-menu" list styles unused (this layout overrides it with its own styles).
  • Remove 'cmln-collapse-point' and 'cmln' grid breakpoint. It was overriden to 992px, which matches the standard 'lg' breakpoint. Use that directly instead, or add it directly to the Bootstrap input.

After this, the skin worked and looked the same with and without Chameleon installed.

Chameleon HTML template components

Various issues I noticed along the way.

  • LinkButton: Fix double-escaping for wmlinks/*/text data. Wrongly used escaped() instead of text() for a value passed to Html::element(). This is likely a mistake due to the fact that Chameleon's wrapper for MW's Html::element (IdRegistry::element) actually takes raw HTML as opoosed to text.
  • UserMenu: Fix PHP warning emitted from the call to SpecialPageFactory::getLocalNameFor(). The string 'UserLogin' is not a valid canonical name, use 'Userlogin' instead.
  • MainContent: The parent class outputted 'siteSub' (tagline) and 'indicators', which our CSS then hid for all media and print modes. Omit from the output instead,
  • MainContent: Replace broken accessibility jump-to links. These didn't work becauses #p-search and #mw-navigation IDs are unreliable in Chameleon, due to the duplicate "responsive" headers.

    These are actually not needed in Chameleon layout because it outputs the nav and search above the content in this HTML structure. In Vector and other default skins, the content is in the HTML *before* the sidebar and header, which is why jump-to links existed, so that AT users (assistive technology) can skip from the top directly to there.

    For this skin, one would need the inverse ("jump to content"). I've done this as part of this commit. I've also removed use of the legacy 'jumpto' styles in favour of the newer 'mw-jump-link' that works in MW out-of-the-box.

PHP WikimediaApiPortal\Skin::init code

  • Don't change globals at runtime. This is an anti-pattern and makes for fragile code due. It did not check what the current skin is or if we are even on a web request currently (e.g. api.php, load.php, CLI, or other 'useskin' options would likely lead to problems). Changing it after Setup.php is also unsupported as services may have already read the values they needed by this point, which meant it could stop working ahy time without notice and be quite hard to detect.
    • $egChameleonLayoutFile: Unnecessary. The Chameleon interface provides a getLayoutFilePath() method that should be overridden by the subclass instead of transporting this through the global scope.

      The getLayoutFilePath() replacement was eventually removed in this patch, given that Mustache avoids this issue (it takes the template name directly as function parameter, without the indirection).
    • $egChameleonExternalStyleVariables: Unnecessary. Call BootstrapManager->setScssVariable directly instead.
    • $wgLogo: Site logos must be considered in LocalSsettings.php, and the value of $wgLogo is expected to point to an image that is safe for use by extensions and other consumers of this variable. In particular, it is expected to be a non-vector file with dimensions approx. 135x135px.

      This was being used to hardcode a specific image to display in the site header. This can be accomplished by simply passing this file name directly to the Header/Logo component, and leaving $wgLogo as-is.
    • $wgSkipSkins: Does not belong to the skin. This is a sysadmin decision to be done in LocalSettings.php. This could be documented in Installation.md instead. In this case I ended up removing it as was only needed to satisfy chameleon.
  • Document that $wgNamespacesWithSubpages is needed for the site to work as expected. This took me a while to figure out.
  • Document ther reason for the "zzz.ext.bootstrap.styles" hack.

PHP WikimediaApiPortal\Skin and WikimediaApiPortalTemplate classes

  • Skin::isViewMode(): Use WebRequest:getRawVal instead of ::getText as the string 'view' is not user input and does not require UTF-8 normalisation.
  • Logo component: Fix logo url to use $wgStylePath instead of assuming $wgScriptPath/skins.
  • PageNav component: Use Title::getSubpageText instead of hardcoded explode() logic.
  • Remove unused 'availableLanguages' data field.
  • Remove unused 'siteLanguage' data field.
  • Remove unused 'activeTitleURL' data field.
  • Use Linker::titleAttrib instead of Linker::tooltipAndAccesskeyAttribs.

    The accesskeys don't work in this skin (nor in plain Chameleon), because the responsive header duplicates the jump targets for these accesskeys. The browser refuses when the target is ambiguous. If this feature is wanted, one would likely either need to make the layout more responsive (through CSS rather than duplicate HTML) so that there is only one target, or keep track of which target is visible through some JavaScript code. Given it wasn't used / didn't work, I've changed it to call titleAttrib() instead, which avoids the issue.

Misc notes

  • User rights:
    • Change prefix from 'wa-' to 'wikimediaapiportal-'.
  • Hooks
    • AuthChangeFormFields removes 'realname' from signup form. What is this for? Why?
    • FIXME: Does should probably move to an extension or LocalSettings.php instead, nor appropiate for a skin - unless it is merely to hide it from the output, in which case display hooks should be used, not auth hooks. Exposing this to SUL/CentralAuth could be risky.
  • MediaWiki Config:
    • Remove need to document and copy wgWikimediaApiPortalSkinAdjustPageTitle as part of install method. Set it correctly by default instead.
  • SetupAfterCache hook:
    • Remove all the unused code from the SetupAfterCache hook.
    • Remove fontawesome includes, it was unused (this skin uses WMUI/OOUI icons).
    • Use the more appropiate ResourceLoaderRegisterModules hook instead.

Ref T247837.

Change-Id: If8d50571a4473101dfab9b0a4977798b6c59f48f

Details

Committed
ReedyJun 9 2020, 2:55 PM
Parents
rMSWA461d321f95fb: Skin implementation
Branches
Unknown
Tags
Unknown
References
refs/changes/00/602200/4
Tasks
T247837: Create API Portal MVP
ChangeId
If8d50571a4473101dfab9b0a4977798b6c59f48f