DevTools: Navigate TabbedPane with arrow keys

DevTools: Navigate TabbedPane with arrow keys

BUG=706699
Review-Url: https://codereview.chromium.org/2805593002
Cr-Commit-Position: refs/heads/master@{#473710}
Committed: https://chromium.googlesource.com/chromium/src/+/7edef8fcbb3ed857191ff14296527fe918ade889

[einbinder, 2017-04-06 03:46:53]

einbinder@chromium.org changed reviewers:
+ aboxhall@chromium.org, hans.hillen@gmail.com

[einbinder, 2017-04-06 03:46:54]

ptal

[einbinder, 2017-04-07 00:27:03]

einbinder@chromium.org changed reviewers:
+ pfeldman@chromium.org

[aboxhall, 2017-04-12 07:25:06]

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:547:
dropDownContainer.addEventListener('click',
this._onDropDownMouseDown.bind(this));
Do we need to make sure we don't run all this code again when a mouse user
releases the mouse button and the click event fires?

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:560: // This
wasn't a mouse event, manually set the position
Can we just do this normally? It doesn't seem like there's much to gain from
getting the exact mouse position within that tiny click target anyway.

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:795:
tab.tabElement.tabIndex = -1;
Can just remove the tabIndex attribute instead (preferable, since that makes it
completely unfocusable - moot in this case since it's detached but better
practice in general)

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:864: var nextTab =
null;
s/nextTab/nextTabElement/ ?

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:882:
this._dropDownButton.click();
This is pretty counter-intuitive - maybe extract the code in
_onDropDownMouseDown() into something like "showRemainingTabs()" (or whatever)
and just call that?

[einbinder, 2017-04-17 21:46:39]

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:547:
dropDownContainer.addEventListener('click',
this._onDropDownMouseDown.bind(this));
On 2017/04/12 at 07:25:06, aboxhall wrote:
> Do we need to make sure we don't run all this code again when a mouse user
releases the mouse button and the click event fires?

The ContextMenu consumes the mousedown event if it is shown, so the click event
isn't fired.

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:560: // This
wasn't a mouse event, manually set the position
On 2017/04/12 at 07:25:06, aboxhall wrote:
> Can we just do this normally? It doesn't seem like there's much to gain from
getting the exact mouse position within that tiny click target anyway.

Done.

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:795:
tab.tabElement.tabIndex = -1;
On 2017/04/12 at 07:25:06, aboxhall wrote:
> Can just remove the tabIndex attribute instead (preferable, since that makes
it completely unfocusable - moot in this case since it's detached but better
practice in general)

Done.

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:864: var nextTab =
null;
On 2017/04/12 at 07:25:06, aboxhall wrote:
> s/nextTab/nextTabElement/ ?

Done.

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:882:
this._dropDownButton.click();
On 2017/04/12 at 07:25:06, aboxhall wrote:
> This is pretty counter-intuitive - maybe extract the code in
_onDropDownMouseDown() into something like "showRemainingTabs()" (or whatever)
and just call that?

I need the event to construct the ContextMenu. I tried to untangle Events and
ContextMenus but it proved difficult.

[aboxhall, 2017-04-17 23:03:36]

Saw some things I didn't notice first time around, apologies for the back and
forth.

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:547:
dropDownContainer.addEventListener('click',
this._onDropDownMouseDown.bind(this));
On 2017/04/17 21:46:39, einbinder wrote:
> On 2017/04/12 at 07:25:06, aboxhall wrote:
> > Do we need to make sure we don't run all this code again when a mouse user
> releases the mouse button and the click event fires?
> 
> The ContextMenu consumes the mousedown event if it is shown, so the click
event
> isn't fired.

Oh right :)

https://codereview.chromium.org/2805593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:882:
this._dropDownButton.click();
On 2017/04/17 21:46:39, einbinder wrote:
> On 2017/04/12 at 07:25:06, aboxhall wrote:
> > This is pretty counter-intuitive - maybe extract the code in
> _onDropDownMouseDown() into something like "showRemainingTabs()" (or whatever)
> and just call that?
> 
> I need the event to construct the ContextMenu. I tried to untangle Events and
> ContextMenus but it proved difficult.

Ah, I see. Yeah it's a bit of a hack using a context menu here really but it
would be a ton of work to unwind.

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/elements-tab-stops-expected.txt
(right):

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/elements-tab-stops-expected.txt:3:
DIV#tab-Styles:Styles
I love this test, but is there any way to test the arrow key functionality as
well?

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:868:
nextTabElement = this._currentTab.tabElement.previousElementSibling;
Sorry for not noticing this the first time around, but this looks potentially
brittle. Obviously though, I don't know the code as well as you. Would it make
sense to do this in terms of `this._tabs` instead? i.e. wrap around the array,
check if the "next" tab is showing, if not, click the drop down?

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:891:
nextTabElement.focus();
Why not make focusing the tab part of selectTab()? If it got selected from a
mouse click it'll be focused anyway, right?

[einbinder, 2017-04-18 18:45:22]

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/elements-tab-stops-expected.txt
(right):

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/elements-tab-stops-expected.txt:3:
DIV#tab-Styles:Styles
On 2017/04/17 at 23:03:36, aboxhall wrote:
> I love this test, but is there any way to test the arrow key functionality as
well?

Added a test.

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:868:
nextTabElement = this._currentTab.tabElement.previousElementSibling;
On 2017/04/17 at 23:03:36, aboxhall wrote:
> Sorry for not noticing this the first time around, but this looks potentially
brittle. Obviously though, I don't know the code as well as you. Would it make
sense to do this in terms of `this._tabs` instead? i.e. wrap around the array,
check if the "next" tab is showing, if not, click the drop down?

It would be nicer to use this._tabs, but this._tabs doesn't correspond to the
DOM order. Depending on the last tab shown, or if the user drags and drops tab
elements, the DOM order can be out of sync with this._tabs.

https://codereview.chromium.org/2805593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:891:
nextTabElement.focus();
On 2017/04/17 at 23:03:36, aboxhall wrote:
> Why not make focusing the tab part of selectTab()? If it got selected from a
mouse click it'll be focused anyway, right?

selectTab is also called for things like Ctrl+] shortcut, when the current tab
is closed, or when a view is revealed.

[aboxhall, 2017-04-19 00:32:47]

lgtm

Test looks great, and thanks for explaining those things.

[einbinder, 2017-04-28 23:42:23]

@pfeldman ptal

[einbinder, 2017-05-19 22:45:10]

On 2017/04/28 at 23:42:23, einbinder wrote:
> @pfeldman ptal

Gentle ping

[pfeldman, 2017-05-19 23:38:41]

lgtm

[einbinder, 2017-05-19 23:46:52]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-05-19 23:47:05]

The CQ bit was unchecked by einbinder@chromium.org

[einbinder, 2017-05-20 00:06:51]

The CQ bit was checked by einbinder@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 00:07:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-20 01:53:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-20 01:53:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[einbinder, 2017-05-22 19:51:18]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-05-22 19:51:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2805593002/#ps60001
(title: "Fix stray space in test")

[commit-bot: I haz the power, 2017-05-22 19:51:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 21:42:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495482678221630,
"parent_rev": "320d7f295bf6705998c40241630638f40a6e7d17", "commit_rev":
"7edef8fcbb3ed857191ff14296527fe918ade889"}

[commit-bot: I haz the power, 2017-05-22 21:42:54]

Description was changed from

==========
DevTools: Navigate TabbedPane with arrow keys

BUG=706699
==========

to

==========
DevTools: Navigate TabbedPane with arrow keys

BUG=706699

Review-Url: https://codereview.chromium.org/2805593002
Cr-Commit-Position: refs/heads/master@{#473710}
Committed:
https://chromium.googlesource.com/chromium/src/+/7edef8fcbb3ed857191ff1429652...
==========

[commit-bot: I haz the power, 2017-05-22 21:42:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/7edef8fcbb3ed857191ff1429652...