Bug 178800 - Web Inspector: Canvas Tab: canvases overview should support navigation via keyboard
Summary: Web Inspector: Canvas Tab: canvases overview should support navigation via ke...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2017-10-25 09:52 PDT by Brian Burg
Modified: 2017-12-04 17:31 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.27 KB, patch)
2017-10-26 01:25 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (11.38 KB, patch)
2017-10-26 15:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-10-25 09:52:52 PDT
Expected:

- left/up moves to previous canvas
- right/down moves to next canvas
- space bar starts/stops a recording

Actual:

Nothing works
Comment 1 Radar WebKit Bug Importer 2017-10-25 09:53:15 PDT
<rdar://problem/35175856>
Comment 2 Devin Rousso 2017-10-26 01:25:18 PDT
Created attachment 324974 [details]
Patch
Comment 3 Brian Burg 2017-10-26 10:29:32 PDT
Comment on attachment 324974 [details]
Patch

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

r=me 

General comments:

Whew, that's way hairier than I thought.
- What do you think about pushing these shortcuts down into CollectionContentView? I guess that might be kind of messy if collection elements are not sized uniformly, but it should still be possible to observe visual rows and columns regardless of the flex direction if you creep forward and backward enough from the current selection to find the next/prev row. (We can shelve this for later, just a thought.)
- RTL would change visual order but not the logical order. Can you make sure that these shortcuts work as intended?

> Source/WebInspectorUI/ChangeLog:9
> +        Create a KeyboardShorcut for each of the following:

Nit: KeyboardShortcut

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:271
> +WI.CanvasOverviewContentView._itemMargin = 10;

I wish there was a sane way to share --variables between CSS and JS.. :|
Comment 4 Devin Rousso 2017-10-26 14:43:37 PDT
(In reply to Brian Burg from comment #3)
> Comment on attachment 324974 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324974&action=review
> 
> r=me 
> 
> General comments:
> 
> Whew, that's way hairier than I thought.
> - What do you think about pushing these shortcuts down into
> CollectionContentView? I guess that might be kind of messy if collection
> elements are not sized uniformly, but it should still be possible to observe
> visual rows and columns regardless of the flex direction if you creep
> forward and backward enough from the current selection to find the next/prev
> row. (We can shelve this for later, just a thought.)
I think it might be useful, but at the same time there is no real reason to support selection for a general CollectionContentView unless you can do something with that collection.  As an example, selecting an item for the Images folder in Resources doesn't really do anything since there is no recording (or anything really) feature there.

> - RTL would change visual order but not the logical order. Can you make sure
> that these shortcuts work as intended?
I'll have to check on that and make sure it works.

> > Source/WebInspectorUI/ChangeLog:9
> > +        Create a KeyboardShorcut for each of the following:
> 
> Nit: KeyboardShortcut
> 
> > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:271
> > +WI.CanvasOverviewContentView._itemMargin = 10;
> 
> I wish there was a sane way to share --variables between CSS and JS.. :|
I think I have a way to do this :)
Comment 5 Nikita Vasilyev 2017-10-26 15:32:22 PDT
(In reply to Devin Rousso from comment #4)
> > > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:271
> > > +WI.CanvasOverviewContentView._itemMargin = 10;
> > 
> > I wish there was a sane way to share --variables between CSS and JS.. :|
> I think I have a way to do this :)

This is how it's usually done:
https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables#Values_in_Javascript
Comment 6 Devin Rousso 2017-10-26 15:48:04 PDT
Created attachment 325066 [details]
Patch
Comment 7 Brian Burg 2017-10-26 16:21:42 PDT
Comment on attachment 325066 [details]
Patch

r=me, thanks Devin!
Comment 8 WebKit Commit Bot 2017-10-26 16:41:26 PDT
Comment on attachment 325066 [details]
Patch

Clearing flags on attachment: 325066

Committed r224067: <https://trac.webkit.org/changeset/224067>
Comment 9 WebKit Commit Bot 2017-10-26 16:41:27 PDT
All reviewed patches have been landed.  Closing bug.