Bug 175451

<rdar://problem/34040769>

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

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.

😮😮😮 this looks incredible!!!

Created attachment 320166 [details] Patch

Created attachment 320167 [details] Patch

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 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`

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.

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.

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

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

Created attachment 320213 [details] Patch

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.

Created attachment 320216 [details] Patch

Last patch fixed the horizontal alignment of the error icon.

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.

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.

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.

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.

Created attachment 320302 [details] [Image] arc icon The arc icon seems a bit thin on non-retina.

Created attachment 320304 [details] [Image] ellipse icon The ellipse icon seems a bit thin on non-retina.

Created attachment 320305 [details] [Image] bezierCurveTo icon The bezierCurveTo icon seems a bit thin on non-retina.

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

Created attachment 320324 [details] Patch

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

Created attachment 320415 [details] Patch

Comment on attachment 320415 [details] Patch Clearing flags on attachment: 320415 Committed r221844: <http://trac.webkit.org/changeset/221844>

All reviewed patches have been landed. Closing bug.