Page MenuHomePhabricator

Reconsider loading icon packs in enableOOUI for optimising critical rendering path
Open, HighPublic

Description

When using enableOOUI in the implementation of the Special:MobileOptions form in T169369 I am seeing a spike of 21.3kb (gzipped) of render blocking CSS. Most of this (about 60%) appears to come from the loading of icon packs 175kb.

Please only load these modules if the icons are actually used as part of the form:

  • oojs-ui.styles.icons-alerts
  • oojs-ui.styles.icons-content
  • oojs-ui.styles.icons-interactions
  • oojs-ui.styles.icons-interactions-indicators
  • oojs-ui.styles.icons-interactions-textures

The URL to the stylesheet
http://reading-web-staging.wmflabs.org/w/load.php?debug=true&lang=en&modules=mediawiki.hlist%7Cmediawiki.ui.button%2Ccheckbox%2Cicon%2Cinput%2Cradio%7Cmediawiki.widgets.styles%7Cmobile.messageBox.styles%7Cmobile.special.mobileoptions.styles%7Cmobile.special.styles%7Coojs-ui-core.styles%7Coojs-ui.styles.icons-alerts%2Cicons-content%2Cicons-interactions%2Cindicators%2Ctextures%7Cskins.minerva.base.reset%2Cstyles%7Cskins.minerva.content.styles%7Cskins.minerva.icons.images%7Cskins.minerva.tablet.styles&only=styles&skin=minerva

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 21 2017, 11:59 PM

These icon packs are required to render some core OOjs UI widgets, so we must load them. I agree that having the entire icon packs always loaded is ridiculous [1], but we need some of the specific icons. Previously this was accomplished using a relatively small core icon pack, but that was removed per T166730. As far as I know, the only solution would be re-introducing it, but that was declined – see T166948: Re-introduce a core icon pack with the icons necessary to render the built-in widgets for details. Perhaps you can convince James and Volker that we should actually do it. (I think this task should be merged into T166948 though.)

[1] 70471939f99abe606f2a14df21fb966a79ca53c0

This comment was removed by matmarex.
This comment was removed by matmarex.
This comment was removed by matmarex.

(No idea how I submitted that comment four times here, please don't mind me.)

Jdlrobson triaged this task as High priority.Nov 27 2017, 5:50 PM
Jdlrobson added subscribers: Volker_E, Jdforrester-WMF.

These icon packs are required to render some core OOjs UI widgets

Why are icons required? What do the widgets look like without the icons?
Is it necessary for these to be loaded in the head? Could these widgets not render without the icons and use JS to load?

In mobile, we load the majority of icons via JS only. I think there's only 4 we consider necessary for first paint. In the last modified bar, there are 2 icons which do not load on non-JavaScript browsers as they are not vital for the UI.

In mobile we used to load all icons in the head and when we removed this average first paint dropped by approx 2s. We obviously load the correct dimensions to avoid reflows, but the icons can be filled as and when needed by JS. The number of icons there was however tiny compared to the icons I'm seeing here on this special page.

If mobile is to adopt OOjs UI in more of it's UIs the stylesheet really needs to be trimmed down to the absolute basics, even if the widgets don't look 100% the same as they do with JS.

cc @Jdforrester-WMF @Volker_E
(Looking at T166948 I see no mention of first paint or loading via render blocking stylesheet which I see as the big issue here...)

@Jdlrobson Related to this there's also T160690: Be able to request a custom icon pack on-the-fly without bloating RL module registry. From my understanding we are not able to break packs down any smaller (or just load the specific icons if necessary) due to the limitation of maximum number of RL modules.

Even if you can't break down the icon packs... Why can't you load them (or at least the majority of them) via JavaScript? Why do so many icons need to be loaded in the head of the special page?

@Jdlrobson Looking at the PHP demos I currently don't see the practicability of not loading icons. It would often just result in an insufficient user experience:

  • ButtonWidget (frameless, icon only)‎
  • ButtonWidget (indicator)‎
  • TextInputWidget (icon)‎
  • TextInputWidget (required)‎
  • TextInputWidget (type=search)‎
  • CheckboxInputWidget
  • DropdownInputWidget

would all massively be affected in their appearance in such being unidentifiable/becoming unusable without icons in no-JS env?!

We've removed some of the output code (324 incl. comments out of the 5691 debug=true CSS lines of code; ~5.7%) in https://gerrit.wikimedia.org/r/395680 respectively T110051.

Two icons of mentioned packs were proposed to removal: 'transparent' of 'textures' & 'bellOn' in 'alerts'.

@matmarex With the following I'm probably more fighting symptoms than causes (that's up in T160690), still:
'content' icons pack seems to hold only three icons that are fundamental for common form widgets (actually two, but three connected):

  • info
  • download/upload
  • tag

We could move 'info' to 'alerts' and 'download'/'upload' to 'interaction', leaving 'tag' behind as TagMultiselectWidget isn't used too often – resulting in not loading 'content' (~5KB SVG only) by default?

Volker_E renamed this task from Do not load icon packs in enableOOUI to Reconsider loading icon packs in enableOOUI for optimising critical rendering path.Dec 15 2017, 2:31 AM

After a short conversation with @Jdforrester-WMF emphasizing on comparing ROI in bigger code savings of getting rid of the vendor-prefix fallbacks than (costly, as deprecating process is lenghty) moving around icons.

When looking at CSS of OOUI core icons (w/o gzipping, P6469) before above, merged changes we were at 181.570 bytes.
After we are at 130.965 bytes or a reduction of 27.87%.

Options remaining, again before weighing if optimization makes sense regarding time spent to achieve:

Further, long-term options, with diverse blocking dependencies:

  • Removing linear-gradient() hack. ~77.375 bytes
  • Getting rid of duplication of .oo-ui-icon-id, .mw-ui-icon-id:before {}
  • Loading just the icons needed, and no icon packs at all T160690
  • Altering color and inverted state with CSS filter or other method instead of extra background-image

This is a breakdown of only the OOUI core icons possible improvements, a lot of the SVG optimization accomplished recently is applying to every product and every image loaded through RL. At best scenario it's about 4.5 KB after gzipping.

Volker_E updated the task description. (Show Details)Jan 11 2018, 1:04 AM

@Jdlrobson With the latest additions to RL's CSSMinifier we would be already down at 116.140 bytes and about 2 KB after gzipping. How often does Reading Web Staging gets updated?

Volker_E moved this task from Backlog to Next-up on the OOUI board.Feb 2 2018, 2:00 AM
Jdlrobson added a comment.EditedMay 18 2019, 11:59 AM

This came up again today when changing 5 icons on the FlaggedRevisions extension (which pulled in OOjs UI). The critical path CSS on desktop for my local instance has jumped from 13.5kb to 32.7kb (after gzip). Without the icon packs (only oojs-ui-core styles) this drops to 21.3kb. All to render 2 icon (4 of the icons are interchangeable and never show at same time)

FlaggedRevisions is only enabled on 5% of pages so maybe this is not a huge problem, but I'm reluctant to +2 this change right now. Is using the BlankTheme an option as an interim solution? See proposed workaround.

T160690 reduces the impact of loading OOUI core styles by about 5 KB (after gzip).

Maybe it's good enough now?