Bug 176822 – Web Inspector: Canvas: recording parameters that include colors should show an InlineSwatch (2D canvas)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/34402170>

[Matt Baker]

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.

[Matt Baker]

Created attachment 320605 [details]
Patch

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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 =)

[Joseph Pecoraro]

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);

[Matt Baker]

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.

[Matt Baker]

(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.

[Matt Baker]

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.

[Matt Baker]

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.

[Devin Rousso]

(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.

[Devin Rousso]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 320803 [details]
Patch

[Matt Baker]

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

[Devin Rousso]

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

[Matt Baker]

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.

[Matt Baker]

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. :)

[Matt Baker]

Created attachment 320820 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

Created attachment 320837 [details]
Patch for landing

[WebKit Commit Bot]

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

[Matt Baker]

Created attachment 320848 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 320848 [details]
Patch for landing

Clearing flags on attachment: 320848

Committed r222102: <http://trac.webkit.org/changeset/222102>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.