Skip to content

[cssom] "Serialize a CSS declaration block" does redundant work collecting longhands #12193

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

Open
AtkinsSJ opened this issue May 14, 2025 · 2 comments
Labels
cssom-1 Current Work

Comments

@AtkinsSJ
Copy link
Contributor

Spun out from something I noticed in #12187: The "shorthand loop" of this algorithm does redundant work each loop iteration:

  1. Let longhands be an array consisting of all CSS declarations in declaration block’s declarations that that are not in already serialized and have a property name that maps to one of the shorthand properties in shorthands.

It's then only used to populate current longhands with a subset of its contents:

  1. Append all CSS declarations in longhands that have a property name that maps to shorthand to current longhands.

(It's also used in step 2 to reject shorthand if it doesn't have what it needs - but that could easily use current longhands instead.)

longhands itself doesn't serve any purpose as-is, it just is an extra array to allocate and add items to. I suspect the intention was as an optimization: If we only populate longhands once, before the loop, then it gives us a shorter list to iterate on each loop iteration, instead of having to look at every declaration each time. (I'm not convinced that would always be faster, fwiw, and it's unusual for a spec algorithm to attempt to optimize things.)

@cdoublev
Copy link
Collaborator

In my opinion, populating longhands in the shorthand loop is appropriate because most properties are not mapped by a shorthand, and even fewer are mapped by more than one shorthand.

You can populate longhands before the shorthand loop and skip doing it when shorthands is empty, but I am not convinced that would be clearer in the spec, or faster in implementations.

@cdoublev cdoublev added the cssom-1 Current Work label May 15, 2025
@AtkinsSJ
Copy link
Contributor Author

I disagree, I think creating longhands inside the loop has no benefits over creating current longhands directly:

  • It's more complicated to populate longhands matching a list of shorthands.
  • You get a list of declarations that may or may not match shorthand.
  • You only care about shorthand so the rest are wasted work and thrown away immediately.

Unless I'm missing something, it's only useful if done once, outside the loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cssom-1 Current Work
Projects
None yet
Development

No branches or pull requests

2 participants