Summary: | Web Inspector: change style of RecordingNavigationSidebarPanel | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||||||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | agomez, bburg, buildbot, commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 174484 | ||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 173807, 178428 | ||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2017-08-10 15:21:17 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
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. |