Bug 175451 – Web Inspector: change style of RecordingNavigationSidebarPanel

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

[Devin Rousso]

- bold action names
- switch icons to be based on category (e.g., path, drawing, style, object creation, attribute getter)
- somehow make action index smaller (take up less space)
- swap the "visual"/draw highlight for `--value-changed-highlight` (the one used by the DOM tree)
- fix the zebra striping colors
- make Import/Export icon buttons

[Radar WebKit Bug Importer]

<rdar://problem/34040769>

[Matt Baker]

Created attachment 319624 [details]
[Image] WIP sidebar redesign

Changes:
 - Removed zebra striping
 - Changed "visual" highlight to green
 - New icons based on operation types
 - Inline color swatches (read-only) for fill/stroke style

[Matt Baker]

Created attachment 319633 [details]
[Image] WIP sidebar redesign - Solar System

Data recorded from MDN's solar system demo: https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Basic_animations.

[Devin Rousso]

😮😮😮 this looks incredible!!!

[Matt Baker]

Created attachment 320166 [details]
Patch

[Matt Baker]

Created attachment 320167 [details]
Patch

[Matt Baker]

Created attachment 320168 [details]
[Image] Full icon set

Icon set for 2D canvas operations. Some may apply to WebGL as well, but generally icons for WebGL actions will be added in a follow-up:

https://bugs.webkit.org/show_bug.cgi?id=176560

[Devin Rousso]

Comment on attachment 320167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320167&action=review

> Source/WebInspectorUI/ChangeLog:9
> +        THis patch adds UI polish to the Canvas recording navigation sidebar,

Style: s/THis/This

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:84
> +    static classNameForAction(recordingAction)

I feel like this should be on RecordingActionTreeElement, as it is entirely a UI concept.  RecordingAction is already very filled with these static lists of things :(

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:103
> +                return "arc";

Is our style to add a newline after break/return inside a switch?  I have seen both in our code.  Personally, I find that having an extra newline makes it clearer where things cascade/fall-through.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:123
> +            case "beginPath":

Remove this, as it exists below.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:471
> +    "webkitImageSmoothingEnabled" // Deprecated

What about the non-prefixed "imageSmoothingEnabled"?

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:481
> +    "webkitLineDashOffset" // Deprecated

What about the non-prefixed "lineDashOffset"?

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:519
> +    "translate"

You can also add "getTransform" to this list.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:26
> +.item.action.initial-state > .icon {

Is there a default icon, or have you covered every case?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:48
> +    --data-indent: calc(var(--tree-outline-icon-margin-start) - var(--tree-outline-icon-margin-end));

Nice!

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:137
> +/* Icon set A */

What does this mean?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:47
> +        function createParameterElement(parameter, swizzleType)
> +        {

Style: bracket on same line

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:48
> +            let parameterElement = titleFragment.appendChild(document.createElement("span"));

Since this function returns the element, you should change this to not call `appendChild`:

    let parameterElement = document.createElement("span");

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:145
> +        if (actionClassName !== "")

I think we usually determine this by checking `.length`

[Devin Rousso]

Created attachment 320172 [details]
[Recording] All actions

I just tried testing by importing the recording taken in LayoutTests/inspector/canvas/recording-2d.html, and saw that:

The following actions are missing an icon:

clip

The following actions are not given a class-name:

commit
direction
drawFocusIfNeeded
fillText
getLineDash
getTransform
imageSmoothingEnabled
imageSmoothingQuality
isPointInPath
isPointInStroke
lineDashOffset
setAlpha
setCompositeOperation
setLineCap
setLineDash
setLineJoin
setLineWidth
setMiterLimit
strokeText

Also, the error icons (e.g. `drawFocusIfNeeded`) seem a bit too large.

[Devin Rousso]

Created attachment 320176 [details]
[Image] Getter vs Setter

Also, are there any plans on visually distinguishing getters and setters?  I realize that it is probably rare that they appear side-by-side like in the attached screenshot, but it might happen.

[Matt Baker]

(In reply to Devin Rousso from comment #10)
> Created attachment 320176 [details]
> [Image] Getter vs Setter
> 
> Also, are there any plans on visually distinguishing getters and setters?  I
> realize that it is probably rare that they appear side-by-side like in the
> attached screenshot, but it might happen.

I still believe that instrumenting getters has limited value (is there any value?). IMO, we should drop them.

[Matt Baker]

Comment on attachment 320167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320167&action=review

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:84
>> +    static classNameForAction(recordingAction)
> 
> I feel like this should be on RecordingActionTreeElement, as it is entirely a UI concept.  RecordingAction is already very filled with these static lists of things :(

I'll relocate the static method and new lists.

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:103
>> +                return "arc";
> 
> Is our style to add a newline after break/return inside a switch?  I have seen both in our code.  Personally, I find that having an extra newline makes it clearer where things cascade/fall-through.

Newline makes it more readable. Will change.

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:123
>> +            case "beginPath":
> 
> Remove this, as it exists below.

Removing the second instance. "beginPath" should share the "moveTo" icon.

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:471
>> +    "webkitImageSmoothingEnabled" // Deprecated
> 
> What about the non-prefixed "imageSmoothingEnabled"?

I missed the [ImplementedAs=*] cases in the IDL, good catch!

>> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:26
>> +.item.action.initial-state > .icon {
> 
> Is there a default icon, or have you covered every case?

We should be able to cover every case.

>> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:137
>> +/* Icon set A */
> 
> What does this mean?

It means I left in junk from when I was playing with multiple icon sets. :)

>> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:48
>> +            let parameterElement = titleFragment.appendChild(document.createElement("span"));
> 
> Since this function returns the element, you should change this to not call `appendChild`:
> 
>     let parameterElement = document.createElement("span");

Oops, copy paste error!

[Matt Baker]

(In reply to Devin Rousso from comment #9)
> Created attachment 320172 [details]
> [Recording] All actions
> 
> I just tried testing by importing the recording taken in
> LayoutTests/inspector/canvas/recording-2d.html, and saw that:
> 
> The following actions are missing an icon:
> 
> clip
> 
> The following actions are not given a class-name:
> 
> commit

CanvasRenderingContext2D.idl:35:
// FIXME: This has been moved to OffscreenCanvasRenderingContext2D in the latest standard.
void commit();

> direction
> drawFocusIfNeeded

Will use classname "image".

> fillText

Will use classname "fill".

> getLineDash
> getTransform
> imageSmoothingEnabled
> imageSmoothingQuality
> isPointInPath
> isPointInStroke
> lineDashOffset
> setAlpha
> setCompositeOperation
> setLineCap
> setLineDash
> setLineJoin
> setLineWidth
> setMiterLimit
> strokeText

I missed a bunch of the implemented interfaces. Going to review all of

CanvasRenderingContext2D implements CanvasState;
CanvasRenderingContext2D implements CanvasTransform;
CanvasRenderingContext2D implements CanvasCompositing;
CanvasRenderingContext2D implements CanvasImageSmoothing;
CanvasRenderingContext2D implements CanvasFillStrokeStyles;
CanvasRenderingContext2D implements CanvasShadowStyles;
CanvasRenderingContext2D implements CanvasFilters;
CanvasRenderingContext2D implements CanvasRect;
CanvasRenderingContext2D implements CanvasDrawPath;
CanvasRenderingContext2D implements CanvasUserInterface;
CanvasRenderingContext2D implements CanvasText;
CanvasRenderingContext2D implements CanvasDrawImage;
CanvasRenderingContext2D implements CanvasImageData;
CanvasRenderingContext2D implements CanvasPathDrawingStyles;
CanvasRenderingContext2D implements CanvasTextDrawingStyles;
CanvasRenderingContext2D implements CanvasPath;

> Also, the error icons (e.g. `drawFocusIfNeeded`) seem a bit too large.

Will polish.

[Matt Baker]

Created attachment 320213 [details]
Patch

[Matt Baker]

Created attachment 320214 [details]
[Image] drawFocusIfNeeded (error icon)

Not sure why the swizzle types are coming through as `None`, but at least the error icon looks better.

Also removed red text styling, since the icon really stands out.

[Matt Baker]

Created attachment 320216 [details]
Patch

[Matt Baker]

Last patch fixed the horizontal alignment of the error icon.

[Devin Rousso]

Created attachment 320239 [details]
[Image] closePath icon

On a non-retina display, it looks like the close path icon is not rendering nicely.  There is an extra pixel on the right side of the icon, and it overall seems to be a lighter color than it should be.

[Devin Rousso]

Created attachment 320240 [details]
[Image] beginPath icon

On a non-retina display, it looks like the main line of begin path icon/arrow is a lighter color than it should be.

[Devin Rousso]

Created attachment 320241 [details]
[Image] drawFocusIfNeeded error icon

The error icon is offset to the right compared to the other icons.  I think it's due to the `left: 2px;` value:

    .item.action.invalid > .icon {
        left: 2px; /* THIS */
        top: 2px;
        width: 16px;
        height: 12px;
        content: url(../Images/Error.svg);
    }

This appears for me on both retina and non-retina.

[Devin Rousso]

Created attachment 320243 [details]
[Image] save/restore icons

On non-retina, the lines used by these icons appear in a lighter grey and are almost blurry.

[Devin Rousso]

Created attachment 320302 [details]
[Image] arc icon

The arc icon seems a bit thin on non-retina.

[Devin Rousso]

Created attachment 320304 [details]
[Image] ellipse icon

The ellipse icon seems a bit thin on non-retina.

[Devin Rousso]

Created attachment 320305 [details]
[Image] bezierCurveTo icon

The bezierCurveTo icon seems a bit thin on non-retina.

[Matt Baker]

Created attachment 320320 [details]
[Image] New icons for "image" and "point in stroke"

[Matt Baker]

Created attachment 320324 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #26)
> Created attachment 320324 [details]
> Patch

Fixed:

- Improved PathLineTo.svg and PathMoveTo.svg on non-retina
- Improved Pixels.svg, PointInPath.svg, and PointInStroke.svg
- Selected item has full opacity icon
- Added missing HitRegion.svg
- Cleaned up SVG syntax across all files

[Devin Rousso]

Comment on attachment 320324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320324&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:30
> +.item.action:not(.initial-state, .parent, .invalid) > .icon {

I think we can simplify the property cascade if we change this

    .tree-outline:not(:focus, .force-focus) .item.action:not(.initial-state, .parent, .invalid) > .icon

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:59
> +:matches(:focus, .force-focus) .item.action.selected:not(.initial-state, .invalid) > .icon {

Should `:matches(:focus, .force-focus)` only be applied to a parent `.tree-outline`?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:124
> +.item.action:not(.selected) > .titles .parameter .inline-swatch {
> +    vertical-align: -1px;
> +}

NIT: I realize that this will be added in a future patch, but this is not used right now.  I would personally prefer to have it removed from this patch and added in the other one.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:140
> +.item.action.hit-region > .icon {
> +    content: url(../Images/HitRegion.svg);
> +}

This doesn't appear to be used anywhere.  Do we need it for WebGL?

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:159
> +            if (WI.RecordingActionTreeElement._lineStyleNames.includes(name))

NIT: it seems odd to have some names inside these static lists while others are listed inline in the switch-case.  Is there a reason for this difference, or could all of them be listed inside the switch?  This way, you only need to do a single check for determining the class of an action.

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:298
> +WI.RecordingActionTreeElement._imageNames = [

For some speed/efficiency, you should make these `new Set([...])`.  This also follows the pattern in RecordingAction.js.  Or you could just inline them in the switch-case above (see above comment).

[Matt Baker]

Created attachment 320415 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320415 [details]
Patch

Clearing flags on attachment: 320415

Committed r221844: <http://trac.webkit.org/changeset/221844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.