iAPI Router: Fix CSS rule order in some constructed style sheets #68923
+56
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DAreRodz
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
6 tasks
DAreRodz
changed the title
|
Size Change: +22 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
luisherranz
approved these changes
Jan 28, 2025
There was a problem hiding this comment.
Looks good to me 🙂
My only question is if we should add more tests to also check that the CSS rules are introduced in the correct order for different stylesheets. Like:
<style>.classname{ color: blue }</style>
<style>.classname{ color: red }</style><style>.classname{ color: blue }</style>
<link rel="stylesheet" href="red-color.css"><link rel="stylesheet" href="blue-color.css">
<link rel="stylesheet" href="red-color.css">and so on...
Would that cover more cases or would it be redundant?
|
Flaky tests detected in e4cf22e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13010383359
|
|
Can we merge this? |
Yes, sorry. The tests mentioned by Luis were not blocking this PR and can be added later if necessary. |
I haven't added them because of an implementation detail: the only type of style sheets processed rule by rule are the referenced style sheets on the initial page. We could test other types as well, but as all the CSS code is handled at once, I don't think it's necessary to check the order in these cases. |
luisherranz
added
Backport to Gutenberg Minor Release
|
Oops, I forgot to add a changelog entry. I've just done it at #68945. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #68691.
The bug affected referenced style sheets that were present on the initial page. When those style sheets were converted into constructed style sheets, the rules were cloned in reverse order, as mentioned in #68691 (comment).
Why?
Styles from such referenced style sheets were suddenly corrupted after client-side navigation.
How?
It was a problem in the following loop:
gutenberg/packages/interactivity-router/src/assets/styles.ts
Lines 44 to 46 in 12f7764
The
sheet.insertRule()method accepts an optionalindexparameter. When omitted, the value is0, making all rules to be placed at the beginning instead of appended.To fix it,
sheet.insertRule()is now called with the correctindexvalue.gutenberg/packages/interactivity-router/src/assets/styles.ts
Lines 44 to 47 in e4cf22e
Testing Instructions
Just follow the steps mentioned in #68691 and ensure the bug has been solved.