fix: inline styles were not being shifted to custom properties - #448
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!
| // `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?
jgerigmeyer
left a comment
jgerigmeyer
left a comment
There was a problem hiding this comment.
Looks good! I left one comment for a potential efficiency gain, and I think there's another related improvement we could make in this PR...
Repeat polyfill() runs grow the inline style attributes, because we keep adding them. That isn't caused by this PR, but we've now amplified it since we're matching against more declarations.
I think we could fix this by removing stale attributes that we've added on a previous run before we re-shift on a subsequent run. Something like this in cascade.ts (I haven't tested this):
diff --git a/src/cascade.ts b/src/cascade.ts
--- a/src/cascade.ts
+++ b/src/cascade.ts
@@ -132,12 +132,31 @@ export function registerShiftedProperties(
}
}
+/**
+ * Remove every declaration of `property` from `block`.
+ *
+ * Used before appending a generated declaration, so that re-processing a block
+ * we already wrote doesn't accumulate a duplicate each time. Inline styles are
+ * re-parsed from the `style` attribute a previous run wrote back, so this is
+ * the difference between an idempotent run and one that grows the attribute
+ * without bound. Removing rather than skipping keeps last-wins order when a
+ * block declares the same property twice.
+ */
+function dropExistingDeclarations(block: Block, property: string) {
+ block.children.forEach((child, item) => {
+ if (isDeclaration(child) && child.property === property) {
+ block.children.remove(item);
+ }
+ });
+}
+
/**
* Shift property declarations for properties that are not yet natively
* supported into custom properties.
*/
function shiftUnsupportedProperties(node: CssNode, block?: Block) {
if (isDeclaration(node) && SHIFTED_PROPERTIES[node.property] && block) {
+ dropExistingDeclarations(block, SHIFTED_PROPERTIES[node.property]);
block.children.appendData({
...node,
property: SHIFTED_PROPERTIES[node.property],
@@ -162,6 +181,7 @@ function expandInsetShorthands(node: CssNode, block?: Block) {
const appendProperty = (property: string, value?: CssNode) => {
if (!value) return;
+ dropExistingDeclarations(block, property);
block.children.appendData({
...node,
property,Or @jpzwarte and @jamesnw if you think this deserves its own issue and PR, that's fine too!
I have some questions (do we drop all, or just overwritten, for example) so I opened #461 |
3 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:
