oddbird / oddbird/css-anchor-positioning

[BUG] Repeat `polyfill()` runs grow the inline `style` attributes

Open Beginner friendly
#461 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
493
Forks
18
Avg merge
12h 37m
Merged PRs (30d)
5

Description

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!

Originally posted by @jgerigmeyer in https://github.com/oddbird/css-anchor-positioning/pull/448#pullrequestreview-4928827142

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/cascade.ts, especially registerShiftedProperties, shiftUnsupportedProperties, and expandInsetShorthands. Reproduce repeated polyfill() processing with shifted properties and inset shorthands, then verify that previously generated inline declarations are removed before new ones are appended and repeated runs no longer grow the style attribute.

Written by the indexing model from the issue text.

Assessment

Tech stack
css, typescript
Domain
frontend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.