Bug 175451 - Web Inspector: change style of RecordingNavigationSidebarPanel
Summary: Web Inspector: change style of RecordingNavigationSidebarPanel
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: 174484
Blocks: WebInspectorCanvasRecording 178428
  Show dependency treegraph
 
Reported: 2017-08-10 15:21 PDT by Devin Rousso
Modified: 2017-10-17 23:17 PDT (History)
7 users (show)

See Also:


Attachments
[Image] WIP sidebar redesign (190.53 KB, image/png)
2017-09-01 11:38 PDT, Matt Baker
no flags Details
[Image] WIP sidebar redesign - Solar System (486.16 KB, image/png)
2017-09-01 12:45 PDT, Matt Baker
no flags Details
Patch (2.02 KB, patch)
2017-09-07 13:19 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (49.01 KB, patch)
2017-09-07 13:21 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] Full icon set (85.26 KB, image/png)
2017-09-07 13:32 PDT, Matt Baker
no flags Details
[Recording] All actions (48.45 KB, application/json)
2017-09-07 13:51 PDT, Devin Rousso
no flags Details
[Image] Getter vs Setter (57.10 KB, image/png)
2017-09-07 14:04 PDT, Devin Rousso
no flags Details
Patch (49.97 KB, patch)
2017-09-07 17:45 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] drawFocusIfNeeded (error icon) (243.90 KB, image/png)
2017-09-07 17:49 PDT, Matt Baker
no flags Details
Patch (49.98 KB, patch)
2017-09-07 17:54 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] closePath icon (7.43 KB, image/png)
2017-09-07 22:41 PDT, Devin Rousso
no flags Details
[Image] beginPath icon (6.31 KB, image/png)
2017-09-07 22:42 PDT, Devin Rousso
no flags Details
[Image] drawFocusIfNeeded error icon (21.24 KB, image/png)
2017-09-07 22:44 PDT, Devin Rousso
no flags Details
[Image] save/restore icons (15.36 KB, image/png)
2017-09-07 22:46 PDT, Devin Rousso
no flags Details
[Image] arc icon (13.35 KB, image/png)
2017-09-08 14:19 PDT, Devin Rousso
no flags Details
[Image] ellipse icon (12.18 KB, image/png)
2017-09-08 14:19 PDT, Devin Rousso
no flags Details
[Image] bezierCurveTo icon (9.85 KB, image/png)
2017-09-08 14:20 PDT, Devin Rousso
no flags Details
[Image] New icons for "image" and "point in stroke" (158.93 KB, image/png)
2017-09-08 17:45 PDT, Matt Baker
no flags Details
Patch (50.45 KB, patch)
2017-09-08 18:11 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (49.35 KB, patch)
2017-09-10 20:16 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 Devin Rousso 2017-08-10 15:21:17 PDT
- 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
Comment 1 Radar WebKit Bug Importer 2017-08-23 12:12:32 PDT
<rdar://problem/34040769>
Comment 2 Matt Baker 2017-09-01 11:38:39 PDT
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
Comment 3 Matt Baker 2017-09-01 12:45:38 PDT
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.
Comment 4 Devin Rousso 2017-09-01 13:19:27 PDT
😮😮😮 this looks incredible!!!
Comment 5 Matt Baker 2017-09-07 13:19:51 PDT
Created attachment 320166 [details]
Patch
Comment 6 Matt Baker 2017-09-07 13:21:16 PDT
Created attachment 320167 [details]
Patch
Comment 7 Matt Baker 2017-09-07 13:32:40 PDT
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
Comment 8 Devin Rousso 2017-09-07 13:43:58 PDT
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`
Comment 9 Devin Rousso 2017-09-07 13:51:23 PDT
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.
Comment 10 Devin Rousso 2017-09-07 14:04:11 PDT
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.
Comment 11 Matt Baker 2017-09-07 14:14:00 PDT
(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.
Comment 12 Matt Baker 2017-09-07 14:22:51 PDT
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!
Comment 13 Matt Baker 2017-09-07 15:53:37 PDT
(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.
Comment 14 Matt Baker 2017-09-07 17:45:31 PDT
Created attachment 320213 [details]
Patch
Comment 15 Matt Baker 2017-09-07 17:49:16 PDT
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.
Comment 16 Matt Baker 2017-09-07 17:54:14 PDT
Created attachment 320216 [details]
Patch
Comment 17 Matt Baker 2017-09-07 17:54:51 PDT
Last patch fixed the horizontal alignment of the error icon.
Comment 18 Devin Rousso 2017-09-07 22:41:31 PDT
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.
Comment 19 Devin Rousso 2017-09-07 22:42:50 PDT
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.
Comment 20 Devin Rousso 2017-09-07 22:44:55 PDT
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.
Comment 21 Devin Rousso 2017-09-07 22:46:06 PDT
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.
Comment 22 Devin Rousso 2017-09-08 14:19:23 PDT
Created attachment 320302 [details]
[Image] arc icon

The arc icon seems a bit thin on non-retina.
Comment 23 Devin Rousso 2017-09-08 14:19:53 PDT
Created attachment 320304 [details]
[Image] ellipse icon

The ellipse icon seems a bit thin on non-retina.
Comment 24 Devin Rousso 2017-09-08 14:20:24 PDT
Created attachment 320305 [details]
[Image] bezierCurveTo icon

The bezierCurveTo icon seems a bit thin on non-retina.
Comment 25 Matt Baker 2017-09-08 17:45:43 PDT
Created attachment 320320 [details]
[Image] New icons for "image" and "point in stroke"
Comment 26 Matt Baker 2017-09-08 18:11:46 PDT
Created attachment 320324 [details]
Patch
Comment 27 Matt Baker 2017-09-08 18:14:31 PDT
(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
Comment 28 Devin Rousso 2017-09-10 16:13:49 PDT
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).
Comment 29 Matt Baker 2017-09-10 20:16:17 PDT
Created attachment 320415 [details]
Patch
Comment 30 WebKit Commit Bot 2017-09-10 21:49:32 PDT
Comment on attachment 320415 [details]
Patch

Clearing flags on attachment: 320415

Committed r221844: <http://trac.webkit.org/changeset/221844>
Comment 31 WebKit Commit Bot 2017-09-10 21:49:34 PDT
All reviewed patches have been landed.  Closing bug.