Summary: | Web Inspector: conic-gradient color picker doesn't accurately show color when saturation value is not 100% | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 189485 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2018-12-14 16:58:11 PST
Created attachment 357489 [details]
Patch
Created attachment 357491 [details]
[Image] After Patch is applied
This is a pretty large change. Is this fixing a regression (and is therefore very important)? (In reply to Joseph Pecoraro from comment #4) > This is a pretty large change. Is this fixing a regression (and is therefore very important)? Yes and no. It does fix the regression (In reply to Joseph Pecoraro from comment #4) > This is a pretty large change. Is this fixing a regression (and is therefore very important)? Yes and no. It does fix the regression introduced by <https://webkit.org/b/189485> (the description/title of this bug explains the issue), but it is also just as viable to roll out <https://webkit.org/b/189485> instead of trying to "fix the fix". This patch is an attempt to get the best of both, in that it uses some of the logic of the old color picker (<canvas> drawing vs conic-gradient), but has the look of the new color picker (hsl vs rgb). Comment on attachment 357489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357489&action=review This is a prettyOverall this seems fine. When I applied locally I encountered one issue: Steps to Reproduce: 1. Trigger color picker for hsl() color 2. Modify the S value manually (up/down arrows) => Graph always snaps current value pointer to the edge even though S is not 100% Same when trying to modify any RGB value manually. Everything else seems fine. > Source/WebInspectorUI/UserInterface/Models/Color.js:522 > - let r = WI.Color._eightBitChannel(this.rgb[0]); > - let g = WI.Color._eightBitChannel(this.rgb[1]); > - let b = WI.Color._eightBitChannel(this.rgb[2]); > - return `rgb(${r}, ${g}, ${b})`; > + let rgb = this.rgb.map((value) => WI.Color._eightBitChannel(Math.round(value))); > + return `rgb(${rgb[0]}, ${rgb[1]}, ${rgb[2]})`; I find the earlier code much easier to read. Also it didn't round but your new code does, is that a good idea? > Source/WebInspectorUI/UserInterface/Models/Color.js:528 > - let r = WI.Color._eightBitChannel(this.rgb[0]); > - let g = WI.Color._eightBitChannel(this.rgb[1]); > - let b = WI.Color._eightBitChannel(this.rgb[2]); > - return `rgba(${r}, ${g}, ${b}, ${this.alpha})`; > + let rgb = this.rgb.map((value) => WI.Color._eightBitChannel(Math.round(value))); > + return `rgba(${rgb[0]}, ${rgb[1]}, ${rgb[2]}, ${this.alpha})`; Ditto. Comment on attachment 357489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357489&action=review >> Source/WebInspectorUI/UserInterface/Models/Color.js:522 >> + return `rgb(${rgb[0]}, ${rgb[1]}, ${rgb[2]})`; > > I find the earlier code much easier to read. Also it didn't round but your new code does, is that a good idea? I believe so, since RGB values are supposed to be integers between [0, 255]. Having a long decimal string suddenly appear doesn't make a lot of sense, especially for a CSS string. I personally find this easier to read, not to mention it avoids code duplication. Created attachment 358000 [details]
Patch
Comment on attachment 358000 [details] Patch Attachment 358000 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10513780 New failing tests: inspector/model/color.html Created attachment 358005 [details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358000 [details] Patch Attachment 358000 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10514089 New failing tests: inspector/model/color.html Created attachment 358006 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 358013 [details]
Patch
webkit-patch doesn't include anything outside of the current directory -.-
Comment on attachment 358013 [details]
Patch
r=me
Comment on attachment 358013 [details] Patch Clearing flags on attachment: 358013 Committed r239597: <https://trac.webkit.org/changeset/239597> All reviewed patches have been landed. Closing bug. |