-
-
Notifications
You must be signed in to change notification settings - Fork 18
fix: inline styles were not being shifted to custom properties #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
21caf74
5c19e33
d61fb36
56adbe3
bc7b531
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||||||||||||||
| import { nanoid } from 'nanoid/non-secure'; | ||||||||||||||||||
|
|
||||||||||||||||||
| import { POLYFILLED_STYLE_ATTRIBUTE } from './cascade.js'; | ||||||||||||||||||
| import { POLYFILLED_STYLE_ATTRIBUTE, SHIFTED_PROPERTIES } from './cascade.js'; | ||||||||||||||||||
| import { querySelectorAllRoots } from './dom.js'; | ||||||||||||||||||
| import { | ||||||||||||||||||
| type AnchorPositioningRoot, | ||||||||||||||||||
|
|
@@ -31,7 +31,7 @@ | |||||||||||||||||
| if (!data.url) { | ||||||||||||||||||
| return data as StyleData; | ||||||||||||||||||
| } | ||||||||||||||||||
| // TODO: Add MutationObserver to watch for disabled links being enabled | ||||||||||||||||||
| // https://github.com/oddbird/css-anchor-positioning/issues/246 | ||||||||||||||||||
| if ((data.el as HTMLLinkElement | undefined)?.disabled) { | ||||||||||||||||||
| // Do not fetch or parse disabled stylesheets | ||||||||||||||||||
|
|
@@ -63,8 +63,26 @@ | |||||||||||||||||
| return results.filter((loaded) => loaded !== null); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const ELEMENTS_WITH_INLINE_ANCHOR_STYLES_QUERY = '[style*="anchor"]'; | ||||||||||||||||||
| const ELEMENTS_WITH_INLINE_POSITION_AREA = '[style*="position-area"]'; | ||||||||||||||||||
| // Inline styles are collected so that `cascadeCSS` can shift their declarations | ||||||||||||||||||
| // into custom properties, like it does for the rest of the CSS. That has to | ||||||||||||||||||
| // cover every property the polyfill later reads back through | ||||||||||||||||||
| // `getCSSPropertyValue` — insets, margins, sizing, padding, self-alignment, | ||||||||||||||||||
| // `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 | ||||||||||||||||||
| // module are part of an import cycle, so `SHIFTED_PROPERTIES` is not | ||||||||||||||||||
| // necessarily initialized yet when this module is evaluated. | ||||||||||||||||||
| let inlineAnchorStylesQuery: string | undefined; | ||||||||||||||||||
| function elementsWithInlineAnchorStylesQuery() { | ||||||||||||||||||
| inlineAnchorStylesQuery ??= [ | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there performance implications with this length of query? If it helps,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Engines don't bucket Median ms per
(Quadrupling the styled elements to 4k barely moves the first column — 16.9ms in Chromium — confirming it's per-element-traversed.)
Agreed, and it generalizes: any term containing another term is redundant. Applied mechanically, 53 terms → 16 ( 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. |
||||||||||||||||||
| '[style*="anchor"]', | ||||||||||||||||||
| ...Object.keys(SHIFTED_PROPERTIES).map( | ||||||||||||||||||
| (property) => `[style*="${property}"]`, | ||||||||||||||||||
| ), | ||||||||||||||||||
| ].join(','); | ||||||||||||||||||
| return inlineAnchorStylesQuery; | ||||||||||||||||||
| } | ||||||||||||||||||
| // Searches for all elements with inline style attributes that include `anchor`. | ||||||||||||||||||
| // For each element found, adds a new 'data-has-inline-styles' attribute with a | ||||||||||||||||||
| // random UUID value, and then formats the styles in the same manner as CSS from | ||||||||||||||||||
|
|
@@ -74,16 +92,10 @@ | |||||||||||||||||
| ? elements.filter( | ||||||||||||||||||
| (el) => | ||||||||||||||||||
| el instanceof HTMLElement && | ||||||||||||||||||
| (el.matches(ELEMENTS_WITH_INLINE_ANCHOR_STYLES_QUERY) || | ||||||||||||||||||
| el.matches(ELEMENTS_WITH_INLINE_POSITION_AREA)), | ||||||||||||||||||
| el.matches(elementsWithInlineAnchorStylesQuery()), | ||||||||||||||||||
| ) | ||||||||||||||||||
| : Array.from( | ||||||||||||||||||
| document.querySelectorAll( | ||||||||||||||||||
| [ | ||||||||||||||||||
| ELEMENTS_WITH_INLINE_ANCHOR_STYLES_QUERY, | ||||||||||||||||||
| ELEMENTS_WITH_INLINE_POSITION_AREA, | ||||||||||||||||||
| ].join(','), | ||||||||||||||||||
| ), | ||||||||||||||||||
| document.querySelectorAll(elementsWithInlineAnchorStylesQuery()), | ||||||||||||||||||
| ); | ||||||||||||||||||
| const inlineStyles: Partial<StyleData>[] = []; | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the first time we've run into this cycle- is there a different file org that would avoid that?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the real runtime graph, there are exactly two cycles:
1.
cascade → utils → dom → cascade(withutils ↔ domnested inside it)This is the one that actually bites. Probing module-body evaluation shows
SHIFTED_PROPERTIESis not yet initialized whendom.ts's body runs — it only gets away with it because the read happens insidegetCSSPropertyValuerather 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 ↔ fallbackOnly
isIdentifieris a real value edge —AnchorPosition,AnchorPositionsandTryBlockare types and already erase.parseneedsparsePositionFallbacks;fallbackneeds those four.Perhaps look at improving this in a new PR?