Summary: | Web Inspector: Canvas: recording parameters that include colors should show an InlineSwatch (2D canvas) | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, thisiskatewinslet, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | 176811 | ||||||||||||||||||||
Bug Blocks: | 173807 | ||||||||||||||||||||
Attachments: |
|
Description
Matt Baker
2017-09-12 19:24:44 PDT
Created attachment 320604 [details]
[Image] Inline color swatches
Since some methods (setShadowColor) have color data spanning multiple parameters, swatches are inserted before the first color parameter.
Created attachment 320605 [details]
Patch
Cool! Can we go a step further and hide the numbers all-together? Maybe only show if the user clicks the swatch? Do color swatches also show up in the Context State sidebar? Do you have a screenshot of that? Devin and I discussed even drawing a CanvasPattern / CanvasGradient in a little Swatch (drawn in a mini <canvas>)! Not sure how feasible that is, but also not a high priority =) Comment on attachment 320605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320605&action=review > Source/WebInspectorUI/UserInterface/Models/Color.js:497 > + rgba = rgba.map((value) => Math.floor(value)); Since the slice is -1 lets name the variable `rgb` and we can drop a function in the map. let rgb = this.rgba.slice(0, -1); rgb = rgb.map(Math.floor); Comment on attachment 320605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320605&action=review >> Source/WebInspectorUI/UserInterface/Models/Color.js:497 >> + rgba = rgba.map((value) => Math.floor(value)); > > Since the slice is -1 lets name the variable `rgb` and we can drop a function in the map. > > let rgb = this.rgba.slice(0, -1); > rgb = rgb.map(Math.floor); I dropped slice/join in favor of destructuring `this.rgba` and passing the formatted parts to a template string. (In reply to Joseph Pecoraro from comment #4) > Cool! Can we go a step further and hide the numbers all-together? Maybe only > show if the user clicks the swatch? > > Do color swatches also show up in the Context State sidebar? Do you have a > screenshot of that? I think there is value in seeing original text, especially for the properties taking CanvasStyle objects that are actually standardized, and thus more likely to be used. For example, `ctx.fillStyle = "violet"`. Having the label "violet" next to the swatch is preferable to having just the swatch, IMO. It's also consistent with our display of colors in CSS property values. Where the swatch becomes awkward is when it represents multiple parameters, which is only an issue with the legacy functions. I'll remove the quotes from color keywords, and leave anything else for future enhancements. Created attachment 320684 [details]
[Image] State sidebar swatches
I think the uniformity/alignment of the grid makes these look even better than the swatches in the tree.
Created attachment 320689 [details]
[Image] Color strings sans quotes
Much less cluttered with *all* color string quotes removed. Also makes the two sidebars are visually consistent.
(In reply to Matt Baker from comment #10) > Created attachment 320689 [details] > [Image] Color strings sans quotes > > Much less cluttered with *all* color string quotes removed. Also makes the > two sidebars are visually consistent. My only concern with this is that it is no longer valid JavaScript, but if the text in "Copy Action" still has the quotes then I'm fine with it. Comment on attachment 320605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320605&action=review r- for tests of new Color functions > Source/WebInspectorUI/UserInterface/Models/Color.js:228 > + static cmyk2rgb(c, m, y, k) Can you add some tests for this? > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:129 > + WI.RecordingActionTreeElement._insertSwatchForColorParameters(recordingAction, parametersContainer); I think it might be clearer (and more flexible for other swatch types, such as image previews) if you inverted the logic. Based on the action name, you create a color swatch. Otherwise, you do nothing. It seems odd to me to always call this function for every action even though most of them don't do anything. > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:191 > + return null; NIT: you can just `return;` with no value. Comment on attachment 320605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320605&action=review >> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:191 >> + return null; > > NIT: you can just `return;` with no value. Oops, left over from a previous version of this function. Created attachment 320803 [details]
Patch
New in this patch: - Tests for the color conversion routines - Additional input massaging to WI.Color functions - InlineSwatch creation for action tree elements has been greatly simplified Comment on attachment 320803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320803&action=review > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:60 > + var color = WI.Color.fromString(parameter) I don't think that this is the right way to do this. If the user calls `fillText("rgb(0, 0, 0)", 0, 0)`, then it wouldn't add quotes around the string, even though it is meant to be displayed as a string. I think we should also check that the action expects a color string (possibly by calling `getColorParameters()`) and that the parameter is valid for `WI.Color.fromString`. > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:62 > + parameterElement.textContent = color.toString(); Does this still include the quotes when copying the action (context menu "Copy Action")? > Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:145 > + let alpha = parameters.length === 1 ? 1 : parameters.lastValue; Style: move this declaration to right before it's used Comment on attachment 320803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320803&action=review >> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:60 >> + var color = WI.Color.fromString(parameter) > > I don't think that this is the right way to do this. If the user calls `fillText("rgb(0, 0, 0)", 0, 0)`, then it wouldn't add quotes around the string, even though it is meant to be displayed as a string. I think we should also check that the action expects a color string (possibly by calling `getColorParameters()`) and that the parameter is valid for `WI.Color.fromString`. IMO we should favor clarity and simplicity of presentation, rather than being true to life. The actual data is preserved in copy text and tooltips. However, I was missing a check for `fillText` throughout, and I agree with your point about checking that the action even expects color parameters, rather than blinding attempting a conversion. >> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:62 >> + parameterElement.textContent = color.toString(); > > Does this still include the quotes when copying the action (context menu "Copy Action")? Good catch, will fix. Comment on attachment 320803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320803&action=review >>> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:60 >>> + var color = WI.Color.fromString(parameter) >> >> I don't think that this is the right way to do this. If the user calls `fillText("rgb(0, 0, 0)", 0, 0)`, then it wouldn't add quotes around the string, even though it is meant to be displayed as a string. I think we should also check that the action expects a color string (possibly by calling `getColorParameters()`) and that the parameter is valid for `WI.Color.fromString`. > > IMO we should favor clarity and simplicity of presentation, rather than being true to life. The actual data is preserved in copy text and tooltips. > > However, I was missing a check for `fillText` throughout, and I agree with your point about checking that the action even expects color parameters, rather than blinding attempting a conversion. Now I see why you mentioned fillText. :) Created attachment 320820 [details]
Patch
Comment on attachment 320820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320820&action=review r=me, but what happened to the tests from the previous patch? > Source/WebInspectorUI/UserInterface/Models/Color.js:246 > + WI.Color._eightBitChannel(b * 255) NIT: add trailing comma Created attachment 320837 [details]
Patch for landing
Comment on attachment 320837 [details] Patch for landing Rejecting attachment 320837 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 320837, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: g rejects to file Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/inspector/model/color-expected.txt Hunk #1 succeeded at 153 (offset 22 lines). patching file LayoutTests/inspector/model/color.html Hunk #1 succeeded at 298 (offset 32 lines). Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/4550872 Created attachment 320848 [details]
Patch for landing
Comment on attachment 320848 [details] Patch for landing Clearing flags on attachment: 320848 Committed r222102: <http://trac.webkit.org/changeset/222102> All reviewed patches have been landed. Closing bug. |