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

test: ensure consistent CSS ordering #6222

Merged
merged 7 commits into from
Dec 30, 2021
Merged

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Dec 29, 2021

Motivation

It's easy to modify CSS insertion order by mistake (just changing ES import orders).

Docusaurus should guarantee a stable CSS insertion ordering over time for all the different ways to provide styling.

This test is just a way to ensure we keep the exact same order. We'll fix ordering issues in a separate PR.

See also

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

test integrated in Netlify deployment (not ideal but seems better than a new GH workflow)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 29, 2021
@netlify
Copy link

netlify bot commented Dec 29, 2021

✔️ [V2]

🔨 Explore the source changes: 4f4f571

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61cd98a0ab25330008f61976

😎 Browse the preview: https://deploy-preview-6222--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Dec 29, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 68
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-6222--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Dec 29, 2021

Size Change: +673 B (0%)

Total Size: 670 kB

Filename Size Change
website/build/assets/css/styles.********.css 102 kB +296 B (0%)
website/build/assets/js/main.********.js 498 kB +377 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 40.1 kB
website/build/index.html 29.6 kB

compressed-size-action

@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Dec 29, 2021
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for now, may not be fully future-proof & not elegant, but it works

@slorber
Copy link
Collaborator Author

slorber commented Dec 30, 2021

Yes, agree with that, just want a quick way to ensure we don't regress on CSS ordering again as it can be disruptive for users

We'll figure out how to make this better over time

@Josh-Cena
Copy link
Collaborator

In the future, if we have a docusaurus-tests repo (as I mentioned on Discord), we can move this to a separate E2E test repo

':root', // TODO should be first
'.DocSearch-Hit-content-wrapper', // TODO should not be there?
'.navbar__title',
'.test-marker-site-custom-css-shared-rule', // TODO should not be there!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...why is that?

Copy link
Collaborator Author

@slorber slorber Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagine the following Infima CSS:

.ifm-full-width {
  width: "100%";
}
.ifm-half-width {
  width: "50%";
}

.ifm-item {
  border: solid;
}

Imagine this element:

<div className="ifm-half-width ifm-item"/>

Now a site owner wants to override some CSS with custom site CSS:

.ifm-item {
  width: "100%"
}

User should expect the item to be 100% width, with the following CSS:

.ifm-full-width {
  width: "100%";
}

.ifm-half-width {
  width: "50%";
}

.ifm-item {
  border: solid;
}

.ifm-item {
  width: "100%";
}

Now it's not what happens in practice, and the override does not work, because CSS is optimized as:

.ifm-full-width, 
.ifm-item {
  width: "100%";
}

.ifm-half-width {
  width: "50%";
}

.ifm-item {
  border: solid;
}

.ifm-half-width still wins as it's last in the stylesheet, and the CSS optimization messed-up with the intent of the user.

If the site owner chose a unique value, like "width: 99%" instead of a duplicate value like 100%, this would be applied.

Does it make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was just missing the click word "optimization" there. You mentioned optimization below and it all makes sense. I don't think there's any overriding here, so it shouldn't be problematic how we order them

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case, there might not be any overriding leading to such weird behaviors, but in userland, it might still happen.

Don't we want to prevent that to happen? Remember custom site CSS is optimized too, so it might impact some users in the end 🤷‍♂️ And upgrading docusauurs means our own internal CSS also changes, which can lead to unexpected CSS ordering changes from one release to another (ie some beta.1 customCSS could be at the bottom, and with beta.2 this exact same custom CSS ends up being at the very top)

Copy link
Collaborator

@Josh-Cena Josh-Cena Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you re-try with a class name with not completely overlapping rules (e.g. add an extra content: "hey". As soon as the user is really attempting to override anything, the selector would go to the bottom. (But in fact in order for her to "override" anything she has to use the same selector as an existing one, instead of a new selector. Relative positions of distinct selectors don't matter; only their specificities do)

Copy link
Collaborator Author

@slorber slorber Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you re-try with a class name with not completely overlapping rules

this is already the case with test-marker-site-custom-css-unique-rule, probably not worth adding another entry 🤷‍♂️

As soon as the user is really attempting to override anything, the selector would go to the bottom.

I don't know, using width: 100% is a legit use-case for me, and the CSS selector of a user does not need to contain multiple rules, so it's eligible for de-duplication, even if it may not be the most common use-case

But in fact in order for her to "override" anything she has to use the same selector as an existing one, instead of a new selector. Relative positions of distinct selectors don't matter; only their specificities do

🤷‍♂️ not really, there are many ways to create selectors targeting the same element, where those elements can have the exact same specificity, and insertion order still matters in those cases.

<div className="x y z"/>

You can target it with .x, .y or .z, all have the same specificity, and class insertion order will matter.

If one of those classes gets optimized and put at the top for any reason (including custom site CSS), order might change in unexpected ways.

This also means that a user adding a custom rule might actually unintentionally trigger infima classes optimizations, leading to bugs in our theme

Copy link
Collaborator

@Josh-Cena Josh-Cena Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider your example with .ifm-full-width and .ifm-item containing the same rules. Does it matter if the user uses className="ifm-full-width" or className="ifm-item"?—No, they both lead to the same style being applied. Yes, Infima classes may get hoisted by a user class name, but if they are always perfectly identical in bahavior, why does it matter which one we use? As soon as the user's class has the slightest difference in bahavior it will be duplicated.

I think we should trust the optimization algorithm, fix the site CSS insertion first, and after that try to come up with an actual counter-case to break the CSS

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, these are edge cases and less important than the general insertion order

'.test-marker-theme-layout',
'.test-marker-site-index-page',
'.DocSearch-Modal',
];
Copy link
Collaborator Author

@slorber slorber Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lex111 @Josh-Cena as we can see the output CSS is currently in a weird order.

Due to CSS optimizations, Infima classes are re-ordered. Site custom CSS classes are "merged" with Infima classes sharing similar rules and put at the very beginning. We end up with CSS classes from different places (site, client modules, Infima, theme...) being interleaved in an unpredictable way.

I'm not sure if this is the correct thing to do in all cases, and I'd rather disable such optimizations, as it's better to do something that works than save a few Kb.

Are these optimizations always guaranteed 100% safe?


The main problem remains that site custom CSS should rather be last so that a site owner can more easily override existing CSS without !important

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if it's because of optimization it makes perfect sense, and that's the entire purpose of optimization: combine duplicated classes where it can. If you have two class names that are identical in every way, it will be safe to combine them. I wouldn't worry about that since there's nothing to override in this case either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a bloated bundle size that I've been looking for ways to minimize (e.g. #3655 could be a way out). Optimizations are meant to be optimizations: there aren't behavior differences but you can't expect everything to be the same, in the same way you can't rely on someFunction.name since it can be minified

@slorber
Copy link
Collaborator Author

slorber commented Dec 30, 2021

Removed the comments as we don't 100% agree on what is wrong with current CSS ordering

Going to merge now and we'll figure out a better CSS ordering in other PRs

@Josh-Cena
Copy link
Collaborator

Yes, let's fix the obvious problems first :D

@slorber slorber merged commit 7adc1c0 into main Dec 30, 2021
@slorber slorber deleted the slorber/test-website-css-order branch December 30, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants