Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes several ColorPicker interaction bugs by preserving hue across achromatic (gray) colors, correctly pairing typed-input “drag” start/end events, and removing reliance on a non-standard array equality helper.
Changes:
- Preserve the last known hue when the selected color becomes achromatic (
s === 0) so the UI doesn’t snap back to red. - Fix typed RGB input leaving
_draggingstucktrueby ensuring a corresponding “end” event is emitted after throttled updates. - Replace use of non-existent
Array.prototype.equalsin thevaluessetter with an inline element-wise comparison.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #playcanvas/supersplat#892
Summary
Fixes three related bugs in
ColorPicker:Hue resets to red when the selected color becomes achromatic. Dragging the saturation/value rect to white (top-left), black, or any gray, or typing an
R=G=Bvalue, would silently overwrite the stored hue with0because_rgb2hsvreturnsh=0, s=0for achromatic input. The user's hue selection was lost on the next interaction. Fixes [playcanvas/supersplat#818](Selecting pure white resets hue in color picker supersplat#818) (supersplat's [PR #892](Preserve color picker hue when selecting white supersplat#892) is a downstream workaround that can be dropped once this lands). The fix gates the_colorHSV[0]assignment onhsv[1] > 0in_setPickerColor,_callPicker, and_onChangeRgb. The partialsum !== 765 && sum !== 0guard in_onChangeRgb(which only covered pure white / pure black) is replaced by the same general check, so mid-grays like(128,128,128)are now handled too._draggingflag stucktrueafter typed input._onChangeRgbsets_dragging = truewhen the user types into an R/G/B field, but nothing ever reset it. This made_setPickerColora permanent no-op for the rest of the picker's lifetime (so externalvalueupdates stopped syncing to the overlay UI) and left the binding'shistoryCombineflag stuck on, breaking undo grouping.callbackHandlenow resets_draggingand emits the matchingpicker:color:endafter the throttledpicker:coloremit, and_onChangeRgbonly emitspicker:color:starton the leading edge so rapid typing produces properly paired start/end events.valuessetter called non-existentArray.prototype.equals. Any consumer that hit the array-of-arrays branch (e.g. multi-select bindings) would get aTypeError. Replaced with an inline length + element-wise comparison.Test plan
npm run build:es6cleannpm run lint:jscleannpm test— all 50 tests pass (no existing ColorPicker tests)255,255,255and#808080directly into the fields — hue should also stay put. Sanity: typing#ff8800should update the hue normally.picker.valuefrom the console — overlay UI should now reflect the change (previously it would stay frozen).editorandsupersplatconsumers still work — neither subclassesColorPickeror touches its protected internals (audited).