Fix CSS concatenation to preserve original stylesheet order#90
Conversation
The previous implementation grouped all concatenatable CSS files into a single bundle that was always output first, before non-concatenatable items. This broke the relative order between concatenated stylesheets and non-concatenated items (such as files with inline styles). This caused issues where theme stylesheets that should appear after Gutenberg's global styles were instead output before them, allowing Gutenberg's `a:where(:not(.wp-element-button))` rule to override theme link styling due to CSS cascade order. The fix adopts the same approach used in concat-js.php: use a level counter where non-concatenatable items break the current concat group (double-increment pattern), preserving the original WordPress enqueue order. Fixes DOTTHEM-159
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical CSS concatenation bug where the previous implementation broke the relative order between concatenated stylesheets and inline styles. The fix adopts the level-based grouping approach from concat-js.php to preserve WordPress's original stylesheet enqueue order.
Changes:
- Replaced index-based grouping with a level counter that increments when non-concatenatable items break concat groups
- Restructured the
$stylesheetsarray to use type-based entries ('concat' or 'do_item') at different levels - Refactored the output loop to process stylesheets in order by iterating through level-indexed entries
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@josephscott I just came across p9o2xV-JF-p2 it's better to be correct than fast, but i'm wondering if this PR raises any red flags for you? |
|
Thanks for the PR, the old approach of putting everything in one concat bucket is definitely the wrong way to go. Before doing anything else, I wrote a set of tests to document the broken behavior and the other edge cases where ordering was still breaking (#92, #93). I have 10 failing tests on the main branch (there is some overlap). Some of the edge cases covered are media interleaving (a.css[media=all] -> b.css[media=screen] -> c.css[media=all]) and RTL stylesheets. |
|
@dsas and @mreishus, thank you for working on this! It's a terrible thing that Page Optimize has been reordering CSS rules (which affects CSS specificity). The fact that non-concat items were being moved to the end is new to me. Thank you for addressing that. It's been a while since I've looked at this plugin, but the changes read well to me. It's great that @mreishus added ordering tests that can give us more confidence. Other CSS ordering issuesIf either of you are tackling CSS ordering issues in general, there is another big bug that could use some attention: For something we activate on all WoA sites by default, it would be good to fix this as well. A while back I explored fixing this in #67 by concatenating all stylesheets as data URIs like this: @import url( 'data:text/css,body { color: blue; }' );
@import url( 'data:text/css,@import "some-other-stylesheet";\nhtml, body { margin: 0; }' );
@import url( 'data:text/css,.someClass { background: red; }' );The idea was that the the CSS nested in the data URIs could have its own Note: This approach probably would break for relative imports unless we do additional work to adjust @import URLs. Unfortunately, when testing these changes with WebPageTest, CSS loading was noticeably slower, even though the data URI contents did not have to be decoded. These were casual tests. This approach might be worth revisiting. Another approach would be to just examine the CSS files for Again, thank you for any help with this! |
|
@brandonpayton Thanks, I'll make sure this is addressed before releasing a new version. I've found another bug that precedes this one - the |
@mreishus this is awesome. I really appreciate it. Knowing there were unfixed ordering issues has haunted me for a while. |
|
I've also done the CSS |
🎉 @mreishus No, I think we should be good. You added much more test coverage. If you're happy with the tests and maybe have smoke tested briefly on a WoA site, please go ahead. It seems like I sort of own this because of the history and being the one of the last to work on it, but really, it is everyone's. Please feel free to do as you see fit. Thank you again! 🙇♂️ |
|
Ok, I have also confirmed we fixed the original problem Dean reported on a WoW/WoA site: I'm ready to send it, but I don't have perms :) Asked in slack. |
|
@dsas The new page optimize should be on all (non-dev, I suppose?) WoA/WoW sites as of an hour ago. Can you check if there are any still issues - or maybe it's not possible due to the :G: upgrade. |
Thanks @mreishus. I've updated page optimize on one of my dev sites that had an older version of GB and the problem has been fixed. I've tried with a couple of themes and haven't noticed anything else amiss. |
Summary
concat-js.phpProblem
The previous implementation grouped all concatenatable CSS files into a single bundle output first, before non-concatenatable items. This broke the relative order between concatenated stylesheets and inline styles.
This caused Gutenberg's
a:where(:not(.wp-element-button)) { text-decoration: underline }rule to override theme link styling (e.g.,a { text-decoration: none }) due to CSS cascade order—when selectors have equal specificity, the last one wins.Solution
Use a
$levelcounter where non-concatenatable items break the current concat group (double-increment pattern fromconcat-js.php), preserving the original stylesheet order.Fixes https://linear.app/a8c/issue/DOTTHEM-159 and #84
Test plan
_static/??URLs in page source)