Page MenuHomePhabricator

[EPIC] Review, rearrange and remove unnecessary CSS in the mobile site
Closed, DeclinedPublic

Description

Background

Back in Sept 2015, render blocking CSS for mobile was only 6.4kb for an anonymous user (T97289) with JS disabled.
Since then it's been steadily increasing and it's now 9.3kb.

Render blocking CSS is problematic as it blocks rendering meaning that it slows down the time a user sees content.
According to real world data it currently our 95th percentile of users still wait up to 5-7s to read Wikipedia on mobile devices. The median is just over 1s, so good but not awesome. On a 3G connection the Barack Obama article takes around 2s to render.

Using uncss and spending time investigating our CSS I found that much of this CSS is unnecessary.

Problem statement

  • Our CSS is messy. This is natural for a 6 year old repository. It could do with some cleanup. There are many outdated and unused CSS rules. This makes maintenance more difficult.
  • Removing render blocking CSS seems to have an impact on real world performance (but not webpagetest see T193570)
  • At least 2.6kb of our render blocking CSS is redudant

Phase 1 - cleanup

I recommend the following changes:

  • Remove usage of data URIs in render blocking styles (T198930)

Estimated savings: 1.6kb
https://gerrit.wikimedia.org/r/444143

  • Do not load mediawiki.ui.button on startup (T193821)

Estimated savings: 0.6kb
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/444276

  • Do not ship 2 versions of the same icon

Estimated savings: 0.2kb
https://gerrit.wikimedia.org/r/444141

  • Do not send JavaScript-only styles as render blocking

Estimated savings: 0.2kb
Various classes are not used if JS is never loaded - .truncated-text,.cloaked-element,.position-fixed, lazy loading related css, last modified active color, overlay header styles
https://gerrit.wikimedia.org/r/444141
https://gerrit.wikimedia.org/r/444134
https://gerrit.wikimedia.org/r/444137
https://gerrit.wikimedia.org/r/444135
https://gerrit.wikimedia.org/r/444138

Phase 2 - prevention

  • Establish a baseline and block merges that impact it - We'll want to prevent the CSS bloating again by measuring the amount of render blocking CSS we serve to anonymous users on each commit. We have something similar in the Page previews codebase for JavaScript.
  • In code review sessions we'll want to talk about the way we write CSS going forward and how we can keep bloat down. webpack may help with this (T160128)
  • Comments would be added to the CSS files and style entry points giving guidelines to developers to prevent good word being undone. It would make clear when something is going to be added to critical CSS and when it is not.

Phase 3 - Inlining css

Although such optimisations in 1&2 are unlikely to make much of a change to the start render time (a casual glance at webpagetest showed no improvement to first paint for a slow 3G connection), it does go a long way to improving how our CSS is architected and sets us up for loading CSS as an inline style in future.

The leaner the CSS is, the more compelling it would be to inline the CSS in the head of the document per this recommendation. This would remove the need for an additional HTTP request for CSS and would give us the fastest first paint possible.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 12 2017, 1:36 AM

@Volker_E apologies for the draft nature of this task but would you be interested in helping me shave those bytes off over xmas in time for the new year?

@Jdlrobson I'd be very interested to collaborate with you here, but the timeframe could be problematic. Do you mean 24–31 Dec?

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Dec 13 2017, 12:20 PM
Jdlrobson updated the task description. (Show Details)Dec 13 2017, 12:27 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from Review and cull unnecessary CSS in the mobile site to Review, rearrange and remove unnecessary CSS in the mobile site.Dec 13 2017, 12:29 PM
Jdlrobson updated the task description. (Show Details)

@Volker_E timeline is flexible but I was planning to submit a few patches next week (I'm off on vacation now until Wed). I've done a bit more research into this and I see some very clear wins. I think culling CSS should be an annual activity! Lots of cruft can appear over the course of a year and I'd love to cull some and then blog about it. It seems like a good practice.

@VolkerE to illustrate my investigations I've created a feature branch and uploaded some patches. Given it's a feature branch we should feel free to merge safely there - they won't end up on production until we merge back to master.

Some of my ideas are non-controversial (e.g. css cruft that we no longer use!) others are more controversial and may require a conversation between the two of us to work out a solution (e.g. loading mediawiki ui via js). I'd be interested in you taking a look, merging some that are non-controversial, flagging the ones you would like to talk about (you should also feel free to submit patches git review firsterpaint to demonstrate any further optimisations you can see.

Jdlrobson updated the task description. (Show Details)Dec 20 2017, 4:23 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Jan 17 2018, 11:32 PM
Jdlrobson updated the task description. (Show Details)Jan 18 2018, 1:05 AM

One thing to be careful about, is apart from first render, to not have interaction delays on crucial elements because the styles are missing. Possible candidates:

  • .mw-ui-button – would you want to split it into two files? One for normal styles, one for the button flags like progressive?
  • .mw-ui-button-group is also added in a few other places

Can you clarify:

  • Load touch-events, position-fixed rules via JavaScript – which parts are you thinking of?
  • Move display:none rules to MobileFormatter e.g. #filetoc,.mw-editsection – first, off, you need to explain the gaining reason for moving to MobileFormatter, second, even then I don't think that this will have a lot of impact and is probably not worth the time
  • Limit return-link css rule to pages that need it – what is this about?

Other notes:

  • (max-width: 320px - 1) seems like an invalid media query to me on first sight.
  • .no-js-only, .client-js .jsonly – CSS class naming to improve on

Print styles inlined vs moving them to an external stylesheet, also
@media print and (max-device-width: 720px) {}

Replace CSS reset with something less intrusive like normalize.css or sanitize.css

I think mediawiki-ui-button has the biggest games, so yes, splitting this up into a crucial module would be very worthwhile.

> Load touch-events, position-fixed rules via JavaScript – which parts are you thinking of?
These classes are added via JavaScript, but the style rules are loaded via CSS.

Move display:none rules to MobileFormatter

The MobileFormatter will strip the HTML from the DOM, so we'd move the logic of hiding them from CSS to HTML, but I agree probably not a big gain here.

Limit return-link css rule to pages that need it – what is this about?

There's a rule in resources/skins.minerva.content.styles/links.less but the return-link. A micro-optimisation by itself but there are lots of places where we follow this similar pattern.

(max-width: 320px - 1)

This is correct, I forget why though but assure you it is :)

@Jhernandez @pmiazga @Niedzielski @Jdrewniak hey there!
The patches posted were all to a feature branch. I'm happy to work on them, but I've also been pondering if it's a worthwhile exercise at this time.

I've actually been thinking a lot about this and how we can keep things maintainable and paintable (;-)) going forward. I think fixing it is fine, but we want to stop it getting out of control again, right? We had an interesting discussion over in T160128 but never got some conclusions.

https://github.com/paypal/glamorous seems like an interesting idea if we go down the Preact route. It would move all our CSS for components into JS. How would you feel about js in your css? https://www.youtube.com/watch?v=lmrQTpJ_3PM gives a nice walkthrough of its features.

@Jdlrobson a pox on your CSS in JS! I will have no such sorcery! 🧙‍♂️

...So that being said, I like the idea of tying CSS to a single component, because many styles tend to be "component specific". There are other styles however, that are global, and I think we should take advantage of those when possible. I do think the "css in js" stuff is kinda weird, but I don't mind the "css uncomfortably close to js" stuff as much, like in Marvin, where the Webpack style-loader was used to pull in CSS files when a JS component was used. I think something like that could leverage our existing mixins from Less (or postCSS) while helping avoid "dead" styles (because component not used? styles not loaded).

I will admit though, these things that take your CSS in JS and output "atomic" classes are amazing "the mystical never-growing stylesheet"...
Also this repo is a good overview of all this kind of stuff.

@Jdlrobson a pox on your CSS in JS! I will have no such sorcery! 🧙‍♂️

haha!

ic". There are other styles however, that are global, and I think we should take advantage of those when possible.

Agreed. I was thinking primarily for the pattern we have in MobileFrontend of having a stylesheet and a JavaScript file for a component e.g. https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/tree/master/resources/mobile.search

These seem to get out of date quickly and are badly maintained and full of specificity wars so I think we should try a new approach as some point during the refactor project.

glamorous is based on glamor which is one of the CSS in JS solutions of many. They have varied tradeoffs and I'm not sure they are as good of an idea for what we do.

If this were to happen I'd suggest it did so with CSS modules. See https://github.com/css-modules/css-modules

It is plain CSS, it is parsed at compile time and has no runtime cost (like many css in js solutions), the files are simple CSS ones, and it defaults to local scopes, and mangled class names for making the collisions impossible and keeping the styles encapsulated.

It also has implementations for webpack, postcss and others.

Example:

banana.css
.button {
  color: green;
}
banana.js
import { button } from "./banana.css";

element.innerHTML = `<div class="${button}">`;

The problem I'm concerned with is redundant CSS rules. Glamorous allows you to define CSS rules on the components themselves

e.g.

GlamDiv = glamorous.div({ color: 'red' } )
Component = () => { return <GlamDiv/>; }

It takes the props you pass and turns them into css rules, so it seems impossible for a css rule to become unused and means you don't have to worry about class naming.

Another approach would be adopting BEM and giving up on the nesting we are currently heavily making use of. This would still require manual grepping to keep up to date, but would at least make our CSS a little more grokable.

A third approach would be to use some kind of tooling to parse our CSS and throw out the redundant bits. Not sure what this would look yet but no such tools exist.

Just as we've hit issues with global JS and components accessing other component elements, I feel we have a problem with our CSS and how a stylesheet can impact other components.

It would be cool to get together and "review" our css, outline our problems and how we might solve them.

If this were to happen I'd suggest it did so with CSS modules. See https://github.com/css-modules/css-modules

Looks like it ticks the box for scoping. Will take a look at what other problems it could solve.

The problem I'm concerned with is redundant CSS rules.

With CSS modules, you find who is importing the CSS file, and check for usages of your class name. Usually, CSS files are smaller and used only by one JS file since they are locally scoped, so it should be easy to check.


The reason I think it is a better fit is because there is no runtime cost, and it is plain CSS, which means it is easier to bailout if something happens, the learning curve is smaller, and there are no workarounds needed for media-queries, declaring animations, and other things that can be weird in CSS-in-JS solutions.

Jdlrobson renamed this task from Review, rearrange and remove unnecessary CSS in the mobile site to [EPIC] Review, rearrange and remove unnecessary CSS in the mobile site.Apr 11 2018, 9:28 PM
Jdlrobson added a project: Epic.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Apr 11 2018, 10:03 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Apr 11 2018, 10:16 PM
Jdlrobson updated the task description. (Show Details)May 1 2018, 9:06 PM
Jdlrobson updated the task description. (Show Details)Jul 6 2018, 7:53 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson triaged this task as Normal priority.Jul 10 2018, 9:37 PM
Jdlrobson updated the task description. (Show Details)Aug 10 2018, 8:55 PM
Izno moved this task from Backlog to Change CSS on the CSS board.Sep 7 2018, 4:47 PM
Jdlrobson closed this task as Declined.Thu, Aug 1, 8:19 PM

I don't think it's useful having this open. It's too open ended and 2 years old.