-
-
Notifications
You must be signed in to change notification settings - Fork 80
perf: Skip redundant border shorthand expansion during cssText updates #295
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?
Conversation
When cssText is set, prepareProperties() already expands border shorthands into all their longhand properties. Previously, calling setProperty() on these shorthands would then trigger their property setters, which called _borderSetter() to expand them again - redundant work. This makes border property setters check the _updating flag (set during cssText operations) and skip _borderSetter() when true. The setters still call parse() to validate and canonicalize the value, then store it directly via _setProperty(). Affected: border, border-width, border-style, border-color, border-top, border-right, border-bottom, border-left 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
9d5d906 to
a9ed7d0
Compare
|
Looks like there is a regression in test/web-platform-tests/tests/css/cssom/shorthand-values.html |
CSS borders have two shorthands: line (border-color, border-style, border-width) and position (border-top, border-right, border-bottom, border-left). At this point, I have no idea how to improve on what is already implemented in normalize.js and parsers.js. If we can remove this redundancy, that would be very welcome. |
Can you explain a bit more, since you understand this project better than me? In particular, how normalize.js and parsers.js map to the handler files? For example... Do we expect eventually every CSS property to have its own handler file? Or will we be able to use some generic patterns, like a line handler and a position handler? Or will we maybe generate handlers from the grammars in |
|
There are at least two shorthands that require special handling in CSS. One is the background, which allows multiple backgrounds. To make things even more confusing, border specifies all four sides with a single value, but border-style and border-top etc. allow you to specify any of sides 1 to 4 individually. So, If we have the following for example: We need to follow these steps in the order the CSS is specified:
|
It would be ideal if that were to happen eventually. |
|
Got it, thanks so much for the explanation! So my takeaways are:
|
Hmm, not quite. Currently, no browsers support stripes(), and there is no test for it in wpt. While |
When cssText is set, prepareProperties() already expands border shorthands into all their longhand properties. Previously, calling setProperty() on these shorthands would then trigger their property setters, which called _borderSetter() to expand them again - redundant work.
This makes border property setters check the _updating flag (set during cssText operations) and skip _borderSetter() when true. The setters still call parse() to validate and canonicalize the value, then store it directly via _setProperty().
Affected: border, border-width, border-style, border-color, border-top, border-right, border-bottom, border-left
🤖 Generated with Claude Code
@asamuzaK I am unsure on this one. It adds a good amount of repetitive code, and is subtle and hard to understand. I guess we can probably factor a bit out to reduce the repetition (I might push some commits to try to do that). But I'm still unsure.
The main thing I'm curious about is how this fits with your larger plans in #255. Do you plan to overhaul these handlers anyway, onto a newer/better architecture? Should we just wait for that? Or would this sort of optimization be valuable longer term?
I also tried to see if this generalizes to other properties besides the border ones, but am not having too much luck so far.