fix(colorarea): remove layout thrashing and cache selectors #5550
Conversation
🦋 Changeset detectedLatest commit: 06d7213 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsChromecolor-area permalinkbasic-test
Firefoxcolor-area permalinkbasic-test
|
packages/color-area/src/ColorArea.ts
Outdated
| if (this.x !== this.inputX.valueAsNumber) { | ||
| this.colorController.color.set( | ||
| 's', | ||
| this.inputX.valueAsNumber * 100 | ||
| ); | ||
| } | ||
| if (this.y !== this.inputY.valueAsNumber) { | ||
| this.colorController.color.set( | ||
| 'v', | ||
| (1 - this.inputY.valueAsNumber) * 100 | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm not entirely clear on why we're doing this here again, as we already set these in the various setters above. Also, the y value is setting the value differently than above (line 574), which seems incorrect. I don't see any side effects to removing this altogether.
There was a problem hiding this comment.
Removing these lines delegates full responsibility for color state updates to the x and y property setters which introduces a tighter coupling between property updates and state synchronisation.
If, for any reason, the input elements and the internal color state become unsynchronised, the component may exhibit inconsistent behavior—potentially leading to visual regressions.
This constitutes a breaking change in the component's internal state management logic and can have both visual and functional regressions.Your tests are failing bcz of the removal of this block.
We would need to a thorough audit of the all the state updates to ensure we are covering every aspects. We can add this as a enhancement but I would not block this PR due to this optimisation.
There was a problem hiding this comment.
I removed this part of the change for now, but as we started to discuss separately, I think it's worth considering whether it makes sense to have roughly the same state stored and updated in multiple places (and accidentally also in multiple ways).
In my opinion it's better to have only one representation of the color value stored in one piece of reactive state, and one way to update it. This state should be used to render the handle position (rather than querying the DOM) for best performance. The same state could also be used to derive the x/y values for the a11y range inputs.
I suspect (only a theory as of yet) that the fact that the color is represented both as a color in the ColorController, and sort of also as x and y coordinates in this ColorArea component (and that they're updated multiple times in different ways) are a source of some strange behavior, especially on slower browsers/devices or when the browser is under some additional load. See the end of this video for what I mean...
Screen.Recording.2025-06-19.at.10.01.19.AM.mov
|
@tjgupta can you please include detailed testing instructions? Also should this cause a visual change at all, I'm seeing a large number of VRT failures? caveat: please don't update the golden hash until a second reviewer gives a thumbs up if visual changes are expected |
|
Also can you added an automated test if possible to cover this code? |
|
Thanks for your contribution @tjgupta . Can you please add a script to set x and y rapidly like below. const colorArea = document.querySelector('sp-color-area');
for (let i = 0; i <= 1; i += 0.01) {
colorArea.x = i;
colorArea.y = 1 - i;
}Let's observe for layout (reflow/paint) spikes. |
packages/color-area/src/ColorArea.ts
Outdated
| if (this.x !== this.inputX.valueAsNumber) { | ||
| this.colorController.color.set( | ||
| 's', | ||
| this.inputX.valueAsNumber * 100 | ||
| ); | ||
| } | ||
| if (this.y !== this.inputY.valueAsNumber) { | ||
| this.colorController.color.set( | ||
| 'v', | ||
| (1 - this.inputY.valueAsNumber) * 100 | ||
| ); | ||
| } |
There was a problem hiding this comment.
Removing these lines delegates full responsibility for color state updates to the x and y property setters which introduces a tighter coupling between property updates and state synchronisation.
If, for any reason, the input elements and the internal color state become unsynchronised, the component may exhibit inconsistent behavior—potentially leading to visual regressions.
This constitutes a breaking change in the component's internal state management logic and can have both visual and functional regressions.Your tests are failing bcz of the removal of this block.
We would need to a thorough audit of the all the state updates to ensure we are covering every aspects. We can add this as a enhancement but I would not block this PR due to this optimisation.
Remove layout invalidation from ColorArea and cache selectors
Description
This change removes layout invalidation/recalculation from the ColorArea when adjusting the current color, either with the mouse or with keyboard input. The change also caches the selectors for the queries so they don't need to be recalculated every time they're accessed.
Motivation and context
The layout thrashing had some negative performance impact, most noticeable in Safari on Intel based Macs.
Related issue(s)
Screenshots (if appropriate)
Before:
After:
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review