-
-
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
Changes from 5 commits
21caf74
5c19e33
d61fb36
56adbe3
bc7b531
c3a864d
fe08c46
dc5f29a
5c5272f
3e0b420
e922b94
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,44 +63,74 @@ | |
| 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. | ||
| // | ||
| // Matching tests the `style` attribute against a single regex rather than | ||
| // handing `querySelectorAll` one `[style*="..."]` clause per property. Engines | ||
| // do not bucket attribute-substring selectors by attribute presence, so a | ||
| // ~50-clause query runs every substring test against every element in the | ||
| // document; querying `[style]` and filtering here is an order of magnitude | ||
| // faster, and scales with the number of styled elements rather than with the | ||
| // size of the document. | ||
| // | ||
| // A term that contains another term is redundant -- `margin` already matches | ||
| // `margin-inline-start`, `anchor` already matches `anchor-name` -- so only the | ||
| // shortest distinct ones are kept. | ||
| // | ||
| // Built on first use rather than at module evaluation: `cascade.js` and this | ||
|
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. This isn't the first time we've run into this cycle- is there a different file org that would avoid that?
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. Checking the real runtime graph, there are exactly two cycles: 1. This is the one that actually bites. Probing module-body evaluation shows
2. Only Perhaps look at improving this in a new PR? |
||
| // module are part of an import cycle, so `SHIFTED_PROPERTIES` is not | ||
| // necessarily initialized yet when this module is evaluated. | ||
| let inlineAnchorStylesRegex: RegExp | undefined; | ||
| /** | ||
| * Checks if the given element has inline styles used by the polyfill, including | ||
| * margin, inset, sizing, padding, self-alignment, `position-area`, and anchor | ||
| * properties. | ||
| * | ||
| * @param el The element to check. | ||
| * @returns True if the element has inline styles used by the polyfill. | ||
| */ | ||
| function hasInlineAnchorStyles(el: HTMLElement) { | ||
| if (!inlineAnchorStylesRegex) { | ||
| const terms = ['anchor', ...Object.keys(SHIFTED_PROPERTIES)]; | ||
| inlineAnchorStylesRegex = new RegExp( | ||
| terms | ||
| .filter( | ||
| (term) => | ||
| !terms.some((other) => other !== term && term.includes(other)), | ||
| ) | ||
| .join('|'), | ||
| ); | ||
|
jgerigmeyer marked this conversation as resolved.
|
||
| } | ||
| return inlineAnchorStylesRegex.test(el.getAttribute('style') ?? ''); | ||
| } | ||
| // 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 | ||
| // style tags. | ||
|
jamesnw marked this conversation as resolved.
Outdated
|
||
| function fetchInlineStyles(elements?: HTMLElement[]) { | ||
| const elementsWithInlineAnchorStyles: HTMLElement[] = elements | ||
| ? elements.filter( | ||
| (el) => | ||
| el instanceof HTMLElement && | ||
| (el.matches(ELEMENTS_WITH_INLINE_ANCHOR_STYLES_QUERY) || | ||
| el.matches(ELEMENTS_WITH_INLINE_POSITION_AREA)), | ||
| ) | ||
| : Array.from( | ||
| document.querySelectorAll( | ||
| [ | ||
| ELEMENTS_WITH_INLINE_ANCHOR_STYLES_QUERY, | ||
| ELEMENTS_WITH_INLINE_POSITION_AREA, | ||
| ].join(','), | ||
| ), | ||
| ); | ||
| const elementsWithInlineAnchorStyles: HTMLElement[] = ( | ||
| elements ?? Array.from(document.querySelectorAll<HTMLElement>('[style]')) | ||
| ).filter((el) => el instanceof HTMLElement && hasInlineAnchorStyles(el)); | ||
| const inlineStyles: Partial<StyleData>[] = []; | ||
|
|
||
| elementsWithInlineAnchorStyles | ||
| .filter((el) => el instanceof HTMLElement) | ||
| .forEach((el) => { | ||
| const dataAttribute = 'data-has-inline-styles'; | ||
| // Reuse an existing id rather than minting a new one each run: a | ||
| // concurrent run (e.g. another shadow root being polyfilled) may already | ||
| // be relying on this element's id in an anchor selector, and re-stamping | ||
| // it would invalidate that selector. | ||
| const selector = el.getAttribute(dataAttribute) ?? nanoid(12); | ||
| el.setAttribute(dataAttribute, selector); | ||
| const styles = el.getAttribute('style'); | ||
| const css = `[${dataAttribute}="${selector}"] { ${styles} }`; | ||
| inlineStyles.push({ el, css }); | ||
| }); | ||
| elementsWithInlineAnchorStyles.forEach((el) => { | ||
| const dataAttribute = 'data-has-inline-styles'; | ||
| // Reuse an existing id rather than minting a new one each run: a | ||
| // concurrent run (e.g. another shadow root being polyfilled) may already | ||
| // be relying on this element's id in an anchor selector, and re-stamping | ||
| // it would invalidate that selector. | ||
| const selector = el.getAttribute(dataAttribute) ?? nanoid(12); | ||
| el.setAttribute(dataAttribute, selector); | ||
| const styles = el.getAttribute('style'); | ||
| const css = `[${dataAttribute}="${selector}"] { ${styles} }`; | ||
| inlineStyles.push({ el, css }); | ||
| }); | ||
|
|
||
| return inlineStyles; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.