fix(w3c/style): put the dark stylesheet last on export so the theme toggle works - #5436
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes exported-document stylesheet ordering in the W3C profile so the TR theme toggle (from W3C fixup.js) actually switches to dark mode by ensuring the dark stylesheet ends up last in <head> on export.
Changes:
- Always register the export-time
styleMoverfordark.css, not only when the spec opts into dark mode. - Update integration/exporter tests to assert the corrected stylesheet order on export.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/w3c/style.js |
Always moves dark.css to the end of <head> during export so it wins the cascade when enabled. |
tests/spec/w3c/style-spec.js |
Updates W3C style export assertions to expect dark stylesheet last. |
tests/spec/core/exporter-spec.js |
Updates exporter test to expect dark.css after the maturity-level W3C stylesheet. |
Suppressed comments (1)
src/w3c/style.js:145
- The comment mentions that the earlier mover puts
base.cssat the end ofheadon export, but the mover above actually targetsfinalStyleURL(the maturity-level stylesheet, which is oftenW3C-*, notbase.css). Updating the wording would avoid misleading future readers about what is being reordered.
);
// Make sure the W3C stylesheet is the last stylesheet, as required by W3C Pub Rules.
sub("beforesave", styleMover(finalStyleURL));
// Add color scheme meta tag and style
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
|
Confirmed manually locally that this works as expected on exported specs. |
|
@marcoscaceres I understand that PR addresses the issue where the dark mode isn't working as expected of exported documents such as https://w3c.github.io/wcag/guidelines/. That said, there seems to be a problem with the respec source documents. I tried that PR with the guidelines source document (see https://nix.w3.org/~denis/respec/guidelines/) and while it's working fine on firefox, chrome 151 renders the spec sometimes in light mode and some other times in dark mode (I have the toggle set to auto). There may be some kind of race condition... |
|
Yep, that’s being addressed an a follow up. Coming in after this PR. |
|
I observe the same issue as Denis, specifically when ReSpec is run client-side (not when viewing an already-processed document). Amusingly, it is somehow always wrong:
Does that branch exist yet so we can get an idea of its complexity? Otherwise it doesn't seem to make much sense to approve this PR that is visibly broken... Moreover, how many more upcoming PRs will there be? And why are we addressing a regression caused by a PR that did not fix anything by adding more complexity (and maybe more regressions), rather than by reverting the one PR in question?
By this logic, 37.3.3+ should not have released with #5408 to begin with. It is also unclear to me, did these issues not already exist in earlier versions when opted into dark mode? So far, 11 documents have been published to w3.org/TR/ since Monday that exhibit the broken theme toggle (out of 32 total). That number will only increase until it is resolved. This is why I immediately advocated for reverting #5408, which could have limited the number of affected publications to 1 or 2. There is also still the core issue of #5408 having spontaneously changed default behavior in a patch release, and #5437 plans to move further in that direction. A change this major should only happen after all of the following:
|
|
@deniak @kfranqueiro The race is real, it reproduces on On whether the dark styles were already broken before #5408: you are right, they were. The docs point is correct and unfixed too: respec.org still documents dark mode as On reverting #5408: I would rather fix these. A revert does stop the race, by taking the Your three preconditions are reasonable and I am not going to pretend they were met. The |
@kfranqueiro and I have reviewed the PRs and confirm #5439 fixes the issue with chrome. Can you merge #5436 and #5439 so we don't end up with a conflict? It will also be easier to follow the changes if there's only one PR.
Thank you for addressing my request about allowing the editors to opt-in. I initially thinking it would be easier to keep the old behavior (which is adding the I'm willing to contact the editors to see how they feel about enabling dark mode support for all specs and point them to #5437 that adds the new config property. |
This statement does not accurately describe #5200. The document that it was reported against was already opted into dark mode using the meta tag, and the report was that the theme controls did render, but didn't work - which I could never reproduce, even when using the versions of ReSpec and As far as I could tell, there was nothing for #5408 to fix, which is why I suggested reverting it instead of adding more. (I realize there may be other motivations for moving in that direction.) |
|
Specref outage is also blocking us 😢 all the bots are sad. |
198b427 to
3cf22c2
Compare
…oggle works Co-Authored-By: Claude <noreply@anthropic.com>
fixup.js drives the link live and both disabled and media are reflected attributes, so a document saved while the reader had dark selected exported an enabled, unconditional dark sheet. Now that the sheet is last in head it would win, rendering dark for everyone. Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
3cf22c2 to
6c2dca8
Compare
Refs #5200, and fixes a regression from #5408. On an exported spec that never opted into dark mode the theme toggle did nothing:
styleMoverwas subscribed for the maturity stylesheet unconditionally but fordark.cssonly when the spec had opted in, so on save the maturity sheet moved past it and won the cascade. The dark sheet now moves for every spec, and itsdisabledandmediastate is restored on export, so a document saved while the reader had dark selected no longer ships a permanently dark spec.Two existing tests asserted the old order and were rewritten.
This makes dark mode reachable where it previously did nothing, which exposes that several of ReSpec's own stylesheets are not dark-ready yet. Those are separate PRs and no release should go out before they land.
Written with AI: this change was generated by Claude. Per AI_POLICY.md.
Proof: the same source exported with the base build and with this branch. Without the fix
dark.csslands beforeW3C-EDand enabling it changes nothing; with the fix it lands last and the toggle works. Marcos then confirmed it manually on exported specs: #5436 (comment)