fix(publisher): order viewport contexts by a total order, not a partial one - #482
Open
borskyj-symph wants to merge 1 commit into
Open
Conversation
…al one Width order between viewport contexts is only defined within a query kind. compareViewportContextCascade compared min-vs-min and max-vs-max by width and fell back to registry index for every other pair, which makes it non-transitive. Array.prototype.sort is only well defined for a consistent comparator, so a registry mixing both kinds produced an order that depended on how the kinds happened to interleave. With [min-width: 1024px, max-width: 900px, min-width: 768px] the two min-width contexts are never compared to each other. The 1024px block is emitted before the 768px one, specificity is equal, and source order decides, so the 768px rule wins at every viewport above 1024px. Mobile-first CSS silently inverts, and the stored rule reads back exactly as authored, so nothing looks wrong until the page renders. Replace the comparator with sortViewportContextCascade, which partitions by kind over registry order and sorts each kind group within the slots that group already occupies. Cross-kind order stays registry order as documented, min and max groups get their width order however the kinds interleave, and 'other' (mixed or non-pixel queries, where width order is undefined) is never reordered. Sorting by registry index first also makes emission independent of contextStyles key order, which is authoring order rather than registry order. Both call sites move to the new function. Fixes CoreBunch#464 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or 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
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 #464.
Viewport contexts were ordered with
compareViewportContextCascade, which compares min-vs-min and max-vs-max by width and falls back to registry index for every other pair. That makes the comparator non-transitive.Array.prototype.sortis only well defined for a consistent comparator, so a registry holding bothmin-widthandmax-widthcontexts produced an order that depended on how the two kinds happened to interleave.With
[min-width: 1024px, max-width: 900px, min-width: 768px]the two min-width contexts are never compared to each other. The 1024px block is emitted before the 768px one, specificity is equal, source order decides, and the 768px rule wins at every viewport above 1024px. Mobile-first CSS silently inverts. The stored rule reads back exactly as authored, so nothing looks wrong until the page renders.Correction to the issue
The issue reports this for a registry of two
min-widthcontexts alone. That case is already correct onmain, in both registry orders. The missing precondition is amax-widthcontext sitting between them, which is what the reporting site's breakpoint set looks like (min-width: 1024px,max-width: 1023px,max-width: 900px). The reported symptom is real, the trigger in the repro steps is incomplete.How
sortViewportContextCascadereplaces the comparator. It partitions by query kind over registry order and sorts each kind group within the slots that group already occupies:minascending andmaxdescending hold however the kinds interleave.other(mixed or non-pixel queries, where width order is undefined) is never reordered.contextStyleskey order, which is authoring order rather than registry order.Both call sites (
createStyleRuleCssEmitter,resolveAutoSizes) move to the new function. Per CONTRIBUTING, the old export is removed rather than kept as a shim.Tests
Three regression tests in
src/__tests__/publisher/classStyleInjector.test.ts. All three fail onmainand pass with the fix:contextStyleskey orderThe two existing cascade-order tests (pure max-width, pure min-width) are unchanged and still pass.
Deliberately not changed
Whether
min-widthandmax-widthgroups should be ordered relative to each other by convention (mobile-first: all min ascending, then all max descending) rather than by registry order. That is a behaviour change beyond this bug and needs a maintainer call. This PR only makes the documented ordering actually hold.Checks
bun run lint,bun run build, andbun test src/__tests__/publisher/ src/__tests__/canvas/ src/__tests__/page-tree/(950 pass, 0 fail) all clean. The fullbun testrun has ~226 failures on this Windows checkout onmainas well, all in collab/socket/SQLite-temp-file areas (EBUSYon cleanup); the set is unchanged by this PR.