Bug 176822 - Web Inspector: Canvas: recording parameters that include colors should show an InlineSwatch (2D canvas)
Summary: Web Inspector: Canvas: recording parameters that include colors should show a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on: 176811
Blocks: WebInspectorCanvasRecording
  Show dependency treegraph
 
Reported: 2017-09-12 19:24 PDT by Matt Baker
Modified: 2017-09-15 22:38 PDT (History)
5 users (show)

See Also:


Attachments
[Image] Inline color swatches (578.79 KB, image/png)
2017-09-12 19:33 PDT, Matt Baker
no flags Details
Patch (11.02 KB, patch)
2017-09-12 19:34 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] State sidebar swatches (539.88 KB, image/png)
2017-09-13 13:37 PDT, Matt Baker
no flags Details
[Image] Color strings sans quotes (755.17 KB, image/png)
2017-09-13 13:52 PDT, Matt Baker
no flags Details
Patch (22.20 KB, patch)
2017-09-14 12:46 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (17.05 KB, patch)
2017-09-14 14:11 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (22.31 KB, patch)
2017-09-14 15:48 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (22.25 KB, patch)
2017-09-14 17:17 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-09-12 19:24:44 PDT
Summary:
Recording parameters that include colors should show an InlineSwatch (2D canvas).

Apply to the following CanvasRenderingContext2D properties/methods:

- fillStyle
- strokeStyle
- shadowColor
- setFillColor (4 overloads)
- setStrokeColor (4 overloads)
- setShadow (4 overloads)

Note:
The overloaded methods are legacy, and only supported by WebKit/Blink.
Comment 1 Radar WebKit Bug Importer 2017-09-12 19:25:18 PDT
<rdar://problem/34402170>
Comment 2 Matt Baker 2017-09-12 19:33:53 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.
Comment 3 Matt Baker 2017-09-12 19:34:25 PDT
Created attachment 320605 [details]
Patch
Comment 4 Joseph Pecoraro 2017-09-12 19:52:10 PDT
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?
Comment 5 Joseph Pecoraro 2017-09-12 19:53:03 PDT
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 6 Joseph Pecoraro 2017-09-12 20:02:18 PDT
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 7 Matt Baker 2017-09-13 13:30:46 PDT
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.
Comment 8 Matt Baker 2017-09-13 13:35:56 PDT
(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.
Comment 9 Matt Baker 2017-09-13 13:37:37 PDT
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.
Comment 10 Matt Baker 2017-09-13 13:52:24 PDT
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.
Comment 11 Devin Rousso 2017-09-13 15:25:50 PDT
(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 12 Devin Rousso 2017-09-13 15:26:47 PDT
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 13 Matt Baker 2017-09-13 16:01:17 PDT
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.
Comment 14 Matt Baker 2017-09-14 12:46:53 PDT
Created attachment 320803 [details]
Patch
Comment 15 Matt Baker 2017-09-14 12:49:52 PDT
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 16 Devin Rousso 2017-09-14 13:07:06 PDT
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 17 Matt Baker 2017-09-14 13:37:11 PDT
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 18 Matt Baker 2017-09-14 13:42:26 PDT
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. :)
Comment 19 Matt Baker 2017-09-14 14:11:30 PDT
Created attachment 320820 [details]
Patch
Comment 20 Devin Rousso 2017-09-14 15:15:45 PDT
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
Comment 21 Matt Baker 2017-09-14 15:48:17 PDT
Created attachment 320837 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2017-09-14 15:51:22 PDT
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
Comment 23 Matt Baker 2017-09-14 17:17:43 PDT
Created attachment 320848 [details]
Patch for landing
Comment 24 WebKit Commit Bot 2017-09-15 12:50:43 PDT
Comment on attachment 320848 [details]
Patch for landing

Clearing flags on attachment: 320848

Committed r222102: <http://trac.webkit.org/changeset/222102>
Comment 25 WebKit Commit Bot 2017-09-15 12:50:45 PDT
All reviewed patches have been landed.  Closing bug.