Bug 181769

Summary: Web Inspector: Canvas Tab: Scroll into view / Inspect element if Canvas has DOM node
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, mattbaker, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 175485    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
[Image] After Patch is applied
none
[Image] Markup2.svg
none
[Image] Markup icons - 2 up
none
[Image] Markup icons - 2 up (Markup3.svg)
none
[Icon] Markup3.svg
none
Patch
none
[Image] After Patch is applied
none
[Image] New canvas buttons - close up none

Description Joseph Pecoraro 2018-01-17 16:12:49 PST
ER: Canvas Tab: Scroll into view / Inspect element if Canvas has DOM node

There are already a bunch of quick actions in the Overview to record / refresh. I'd like to see an action to try to scroll the canvas into view on the real page / highlight the element on the page if possible. These kinds of things.
Comment 1 Devin Rousso 2018-01-17 21:03:26 PST
Created attachment 331587 [details]
Patch

I'm gonna put it out there and say that I don't like the icons I used for Log and the Canvas Element menu, but I'm not sure what else to use.  This is more of a proof of concept.  Feedback would be greatly appreciated.
Comment 2 Devin Rousso 2018-01-17 21:05:00 PST
Created attachment 331588 [details]
[Image] After Patch is applied

The image for "Log Canvas Context" is the same icon we use for `console.log()` results.
The image for "Canvas Element" is the icon we use for elements in the DOM tree, and the context menu that appears when you click on it is the same as what you would get in the DOM tree.
Comment 3 Matt Baker 2018-01-18 11:47:28 PST
(In reply to Devin Rousso from comment #1)
> Created attachment 331587 [details]
> Patch
> 
> I'm gonna put it out there and say that I don't like the icons I used for
> Log and the Canvas Element menu, but I'm not sure what else to use.  This is
> more of a proof of concept.  Feedback would be greatly appreciated.

What about a simple monochrome "</>" icon for the element menu? I think that would look better alongside the other two buttons. Also, let's drop the Log button since Log Element can be accessed from the context menu.
Comment 4 Devin Rousso 2018-02-19 18:10:57 PST
Created attachment 334219 [details]
Patch

The Markup.svg icon is what I was able to come up with in Sketch.  Definitely could use a better icon -.-
Comment 5 Devin Rousso 2018-02-19 18:11:09 PST
Created attachment 334220 [details]
[Image] After Patch is applied
Comment 6 Matt Baker 2018-02-19 20:31:27 PST
Comment on attachment 334219 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:339
> +                const text = WI.UIString("Selected Canvas Context");

Nit: I don't find this const particularly helpful, but I'll leave it up to you.
Comment 7 Matt Baker 2018-02-19 20:34:23 PST
Created attachment 334232 [details]
[Image] Markup2.svg

Here's a polished </> icon. I used the Inspector's markup font as a guide.
Comment 8 Matt Baker 2018-02-19 20:47:05 PST
Created attachment 334234 [details]
[Image] Markup icons - 2 up

I'm going to tweak the new icon a bit. I think it looks too compact.
Comment 9 Timothy Hatcher 2018-02-20 09:29:17 PST
Matt, I do like the second icon better, but it seems a little too bold. (Maybe it is the screenshot not being viewed as HiDPI.) Compared to the reload icon weight, it is just a hair more it seems.
Comment 10 Matt Baker 2018-02-20 12:33:51 PST
(In reply to Devin Rousso from comment #5)
> Created attachment 334220 [details]
> [Image] After Patch is applied

My only other comment is that I think the '</>' button should be last. Having to maneuver the cursor between two buttons to hit Record seems bad.
Comment 11 Matt Baker 2018-02-20 13:12:51 PST
Created attachment 334298 [details]
[Image] Markup icons - 2 up (Markup3.svg)

- Fixed SVG path color
- Increased height of brackets and slash
- Increased angle of brackets
Comment 12 Matt Baker 2018-02-20 13:13:45 PST
Created attachment 334299 [details]
[Icon] Markup3.svg
Comment 13 Matt Baker 2018-02-20 13:14:49 PST
(In reply to Timothy Hatcher from comment #9)
> Matt, I do like the second icon better, but it seems a little too bold.
> (Maybe it is the screenshot not being viewed as HiDPI.) Compared to the
> reload icon weight, it is just a hair more it seems.

I had forgotten to set paths to use fill="currentColor".
Comment 14 Devin Rousso 2018-02-20 17:46:36 PST
Created attachment 334325 [details]
Patch
Comment 15 Devin Rousso 2018-02-20 17:46:53 PST
Created attachment 334326 [details]
[Image] After Patch is applied
Comment 16 Timothy Hatcher 2018-02-20 17:48:37 PST
Comment on attachment 334325 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Images/Markup.svg:6
> +    <path fill="currentColor" d="M 15.5 8 a 0.5 0.5 0 0 1 -0.24 0.43 L 11 10.99 V 9.82 L 14.03 8 11 6.18 V 5.01 l 4.26 2.56 A 0.5 0.5 0 0 1 15.5 8 Z"/>
> +    <polygon fill="currentColor" points="10.77 3 6.32 13 5.23 13 9.68 3 10.77 3"/>
> +    <path fill="currentColor" d="M 5 6.18 1.97 8 5 9.82 v 1.17 L 0.74 8.43 a 0.505 0.505 0 0 1 0 -0.86 L 5 5.01 Z"/>

Is it me, or is the color not matching the reload icon? The reload icon looks like a grey, where the arrows look black.
Comment 17 Matt Baker 2018-02-26 15:25:04 PST
(In reply to Timothy Hatcher from comment #16)
> Comment on attachment 334325 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334325&action=review
> 
> > Source/WebInspectorUI/UserInterface/Images/Markup.svg:6
> > +    <path fill="currentColor" d="M 15.5 8 a 0.5 0.5 0 0 1 -0.24 0.43 L 11 10.99 V 9.82 L 14.03 8 11 6.18 V 5.01 l 4.26 2.56 A 0.5 0.5 0 0 1 15.5 8 Z"/>
> > +    <polygon fill="currentColor" points="10.77 3 6.32 13 5.23 13 9.68 3 10.77 3"/>
> > +    <path fill="currentColor" d="M 5 6.18 1.97 8 5 9.82 v 1.17 L 0.74 8.43 a 0.505 0.505 0 0 1 0 -0.86 L 5 5.01 Z"/>
> 
> Is it me, or is the color not matching the reload icon? The reload icon
> looks like a grey, where the arrows look black.

Looks okay to me.
Comment 18 Matt Baker 2018-02-26 15:25:32 PST
Created attachment 334653 [details]
[Image] New canvas buttons - close up
Comment 19 Timothy Hatcher 2018-02-26 15:26:12 PST
(In reply to Matt Baker from comment #18)
> Created attachment 334653 [details]
> [Image] New canvas buttons - close up

That looks right. Thanks!
Comment 20 Matt Baker 2018-02-26 15:27:32 PST
Comment on attachment 334325 [details]
Patch

r=me
Comment 21 WebKit Commit Bot 2018-02-26 15:52:14 PST
Comment on attachment 334325 [details]
Patch

Clearing flags on attachment: 334325

Committed r229044: <https://trac.webkit.org/changeset/229044>
Comment 22 WebKit Commit Bot 2018-02-26 15:52:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2018-02-26 15:53:20 PST
<rdar://problem/37920747>