fix: inline styles were not being shifted to custom properties - #448
fix: inline styles were not being shifted to custom properties#448jpzwarte wants to merge 3 commits into
Conversation
✅ Deploy Preview for anchor-position-wpt canceled.
|
✅ Deploy Preview for anchor-polyfill ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jamesnw
left a comment
jamesnw
left a comment
There was a problem hiding this comment.
Looks good, just a couple questions on this. Thanks!
| // necessarily initialized yet when this module is evaluated. | ||
| let inlineAnchorStylesQuery: string | undefined; | ||
| function elementsWithInlineAnchorStylesQuery() { | ||
| inlineAnchorStylesQuery ??= [ |
There was a problem hiding this comment.
Are there performance implications with this length of query?
If it helps, anchor-scope and anchor-name are in SHIFTED_PROPERTIES, and duplicated by the style*=anchor query. Also, all the padding variants could be caught with a single wildcard, all the margin variants, etc.
There was a problem hiding this comment.
Are there performance implications with this length of query?
Yes. Engines don't bucket [style*=…] by attribute presence, so all 53 clauses get tested against every element in the traversal — cost tracks document size, not how many elements actually have inline styles.
Median ms per querySelectorAll, 20k elements / 1k styled:
| Engine | current (53) | minimized (16) | [style] + one regex |
|---|---|---|---|
| Chromium | 14.04 | 3.94 | 0.35 |
| Firefox | 7.75 | 2.45 | 0.15 |
| WebKit | 3.25 | 0.95 | 0.15 |
(Quadrupling the styled elements to 4k barely moves the first column — 16.9ms in Chromium — confirming it's per-element-traversed.)
…duplicated by the
style*=anchorquery. Also, all the padding variants could be caught with a single wildcard…
Agreed, and it generalizes: any term containing another term is redundant. Applied mechanically, 53 terms → 16 (anchor, left, right, top, bottom, inset, margin, width, height, block-size, inline-size, justify-self, align-self, place-self, position-area, padding).
Matched set is unchanged: a term is dropped only when a strictly shorter term is a substring of it, so every removal chain ends at a retained term that matches the same strings. Kept terms are a subset of the originals, so no false positives either. All three variants matched identical sets in every benchmark case.
| // `position-area` — and not just the anchor-specific ones: a target can take | ||
| // its `position-area` from a stylesheet while setting its margin inline. | ||
| // `anchor` is matched on its own as well, for `anchor()`/`anchor-size()` values. | ||
| // Built on first use rather than at module evaluation: `cascade.js` and this |
There was a problem hiding this comment.
This isn't the first time we've run into this cycle- is there a different file org that would avoid that?
There was a problem hiding this comment.
Checking the real runtime graph, there are exactly two cycles:
1. cascade → utils → dom → cascade (with utils ↔ dom nested inside it)
This is the one that actually bites. Probing module-body evaluation shows SHIFTED_PROPERTIES is not yet initialized when dom.ts's body runs — it only gets away with it because the read happens inside getCSSPropertyValue rather than at module scope. All three edges are single-use:
dom → cascadeexists solely forSHIFTED_PROPERTIES(dom.ts:51)utils → domexists solely forstrategyForElement(utils.ts:292→getCSSPropertyValue)dom → utilsexists solely forgetRootStyleContainer(dom.ts:105)
2. parse ↔ fallback
Only isIdentifier is a real value edge — AnchorPosition, AnchorPositions and TryBlock are types and already erase. parse needs parsePositionFallbacks; fallback needs those four.
Perhaps look at improving this in a new PR?
2 participants
Fixes #435
fetchInlineStyles()only collected elements whose style attribute contains anchor or position-area, so an inlinemargin-left: 40pxnever went throughcascadeCSS()and never got shifted into--margin-left-<uuid>.The query is now built from
SHIFTED_PROPERTIES, so inline insets, margins, sizing, padding and self-alignment are collected as well. Shifting appends the custom property rather than replacing the declaration, so the extra elements just get--margin-left-<uuid>: 40pxalongside what they already had.Without the fix:
With the fix:
Native:
