Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite many selectors with :is() to reduce size #208

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

joshyrobot
Copy link
Contributor

I understand this is a major (unasked for) change, and I expect it needing a conversation to decide if it gets merged. To outline what this change means: the compiled output shrinks from 92.6kB -> 65.5kB uncompressed, and from 12.6kB -> 9.6kB gzipped. Using brotli and some further simple tweaks to the compiling process, I was able to get it down to 7.5kB.

I recommend looking at the changes one commit at a time, as some are less major than other. Each should work on its own, so if one commit seems particularly nasty it could be omitted. This PR does not include the changes for any compiled files, I figured those could be added when a final changeset is decided on.

This might somewhat impact compatibility depending on what browser share you're currently targeting. Can I Use estimates this change would work for about 90% of people.

Readability is another potential concern. I personally find it easier to read and understand selectors that use :is() when compared to long lists of repeated selectors, but if you disagree I imagine there's a comfortable middle-ground.

Performance is perhaps my biggest concern, because I understand it the least. I don't know how to test this or what scale this change is on, but I imagine it would make applying the CSS slower to some extent.

As for whether this changes how any of the styles are applied, my tests seem to indicate no. I opened the site on various pages with the original and modified CSS and ran the code attached below. I then diffed the results for each page, and none of them had any differences. This does not test for states like :hover or similar, but by looking at the code and doing some quick manual checks I feel confident enough that they work as expected.

function* walk(node, depth = 0) {
    yield [depth, node]
    for (const child of node.childNodes) {
      if (child instanceof Element) yield* walk(child, depth + 1)
    }  
}

function summarize(element) {
  return JSON.stringify({
    x: element.tagName,
    s: getComputedStyle(element),
    b: getComputedStyle(element, ':before'),
    a: getComputedStyle(element, ':after'),
  })
}

const summaries = []
for (const [depth, node] of walk(document.body)) {
  summaries.push('  '.repeat(depth) + summarize(node))
}
console.log(summaries.join('\n'))

@mosra
Copy link
Owner

mosra commented Jan 5, 2022

(Sorry for embarrassingly late replies, finally got a chance to get back to this project.)

Hey, wow, this is very cool, thanks a lot!!

I'll do a more thorough pass over the changes later to catch any suspicious differences, but overall the direction of the changes is great, and the size savings are amazing.

Regarding performance, my wishful thinking would be that browsers either have an one-time preprocessing step to expand the :is() back to what I did there originally, resulting in negligible extra cost during the stylesheet parsing, and no penalty whatsoever during page render. Or maybe, from a completely different perspective, the :is() actually makes browsers process the style faster because there's not that many combinations to check for and apply? Interesting topic, I'll see if I can find some rationale for why :is() was introduced, whether it was purely for webdevs convenience or because browser vendors can optimize that better.

On the other hand, given that GitHub (which is actually one of the lighter sites out there) takes several seconds to render any page and refuses to display 1000-line diffs "because it might slow down the browser", I think we're fine here 😆 In my experience, m.css pages of basically any length (say, this one or this) always rendered immediately on any browser I tried, without any visible pop-ins or relayouting and I can scroll through them without any stutter whatsoever. So even if this slowed them down 2x (which I seriously doubt) it'd be still an acceptable tradeoff for such massive size savings.

@joshyrobot
Copy link
Contributor Author

Would you like me to rebase to resolve the conflict and change the target branch to next?

@mosra
Copy link
Owner

mosra commented Jan 6, 2022

Uh oh, did I cause a conflict with my recent CSS changes? Thought it would be fine, but apparently it clashed with your fixes to the ::before and ::after selectors. I merged that part in d3227ff -- thanks for separating the work into those tiny commits by the way, that's very helpful -- so if you rebase on top of that, dropping the commit from your side, it should be conflict-less again. I hope. (The next branch is just a temporary branch where I push commits until they pass the CI, it fluctuates a lot and as such is not meant to be a target for PRs.)

Hmm, actually, I still need to think about browser compatibility. I didn't even adopt CSS grids (#31) yet for fear of cutting some users out (and I was even processing CSS variables and @imports to keep compatibility with IE 8, although that's probably pointless today). This requires way more recent browsers than grids. Since this project took off, it's no longer used just by tech people for tech people, but is used by poets, writers or an orchestra, and there it's more likely that the content is viewed from older computers with software that isn't as easy to update anymore.

I don't want to just discard your changes or put them aside and let them rot, though. As a middle-ground solution I'm thinking I could update the css/postprocess.py script to expand the :is() selectors during postprocessing same as it does with variables and imports. That way it would still produce mostly the same .compiled.css files as before for compatibility, and this postprocessing step eventually gets dropped or becomes opt-in for these kind of sites that have to preserve compatibility.

@joshyrobot
Copy link
Contributor Author

Another option to consider instead of post-processing away all the :is() selectors is to try including the prefixed versions. That would bring support from ~90% to ~97%, but it would also increase the bundle size a fair bit because you'd have to duplicate all the rulesets that use :is() 2 more times. Probably worth testing for exact numbers because of how much simpler it seems to implement.

Unfortunately, any kind of post-processing is going to be a bit beyond me, but do feel free to let me know if you need me to mess with this PR some more! For now, I imagine it'll stay on the back burner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants