1444300 - Cannot activate a tab in the DevTools tab bar after highlighting it

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Brian Birtles (:birtles)]

STR:

1. Click a tab in the developer toolbar and observe that it has a focus ring on it
2. Press left or right to highlight another tab.
3. Press Enter or Space to activate the highlighted tab.

Expected results:

The highlighted tab becomes the active tab.

Actual results:

Nothing happens.

Comment 1

[Brian Birtles (:birtles)]

Apparently this is just a bug, most likely from the React rewrite. It involves re-doing the work done in bug 1242852.

Yura is happy to review.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: tab-key-handle

The command button which(e.g. splitconsole / paintflashing / screenshot..etc) listens the onKeyDown event[1]. However, tool tab button (i.e. ToolboxTab) doesn't have this listener.

[1] https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/devtools/client/framework/toolbox.js#776

This wip patch will handle keydown event in tab button.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 8962580 [details]
tab-key-handle

>+        onKeyDown: (evt) => {
>+          if (evt.key === "Enter") selectTool(id);
>+        },

Please make these react to the SPACE key as well. These are buttons, or toggle buttons, so Space is also expected to activate them.

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: wip-handle-keyevent-on-toolbox

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> Comment on attachment 8962580 [details]
> tab-key-handle
> 
> >+        onKeyDown: (evt) => {
> >+          if (evt.key === "Enter") selectTool(id);
> >+        },
> 
> Please make these react to the SPACE key as well. These are buttons, or
> toggle buttons, so Space is also expected to activate them.

OK. I modified the patch to be able to detect the space key.

Thanks!

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8962643 [details] [diff] [review]
wip-handle-keyevent-on-toolbox

># HG changeset patch
># Parent  de32269720d056972b85f4eec5f0a8286de6e3af
>Bug 1444300 - Part 1. Make tab button to be able to handle the key event.
>
>diff --git a/devtools/client/framework/components/toolbox-tab.js b/devtools/client/framework/components/toolbox-tab.js
>--- a/devtools/client/framework/components/toolbox-tab.js
>+++ b/devtools/client/framework/components/toolbox-tab.js
>@@ -62,6 +62,11 @@ class ToolboxTab extends Component {
>         tabIndex: focusedButton === id ? "0" : "-1",
>         onFocus: () => focusButton(id),
>         onMouseDown: () => selectTool(id),
>+        onKeyDown: (evt) => {
>+          if (evt.key === "Enter" || evt.key === "Space") {
>+            selectTool(id);
>+          }
>+        },
>       },
>       span(
>         {
>diff --git a/devtools/client/framework/test/browser_toolbox_keyboard_navigation.js b/devtools/client/framework/test/browser_toolbox_keyboard_navigation.js
>--- a/devtools/client/framework/test/browser_toolbox_keyboard_navigation.js
>+++ b/devtools/client/framework/test/browser_toolbox_keyboard_navigation.js
>@@ -81,3 +81,42 @@ add_task(async function () {
>   EventUtils.synthesizeKey("KEY_Tab", {shiftKey: true});
>   is(doc.activeElement.id, expectedFocusedControl.id, "New control is focused");
> });
>+
>+// Test that moving the focus of tab button and selecting it.
>+add_task(async function () {
>+  info("Create a test tab and open the toolbox");
>+  let toolbox = await openNewTabAndToolbox(TEST_URL, "inspector");
>+  let doc = toolbox.doc;
>+
>+  let toolbar = doc.querySelector(".toolbox-tabs");
>+  let tabButtons = toolbar.querySelectorAll(".devtools-tab, button");
>+  let win = tabButtons[0].ownerDocument.defaultView;
>+
>+  // Put the keyboard focus onto the first tab button.
>+  tabButtons[0].focus();
>+  ok(containsFocus(doc, toolbar), "Focus is within the toolbox");
>+  is(doc.activeElement.id, tabButtons[0].id, "First tab button is focused.");
>+
>+  // Move the focused tab and select it by using enter key.
>+  let onKeyEvent = once(win, "keydown");
>+  EventUtils.synthesizeKey("KEY_ArrowRight");
>+  await onKeyEvent;
>+
>+  let onceSelected = toolbox.once("webconsole-selected");
>+  EventUtils.synthesizeKey("Enter");
>+  await onceSelected;
>+  ok(doc.activeElement.id, tabButtons[1].id, "Changed tab button is focuesd.");
>+
>+  tabButtons[1].focus();
>+
>+  // Return the focused tab with space key.
>+  onKeyEvent = once(win, "keydown");
>+  EventUtils.synthesizeKey("KEY_ArrowLeft");
>+  await onKeyEvent;
>+
>+  onceSelected = toolbox.once("inspector-selected");
>+  EventUtils.synthesizeKey("Space");
>+  await onceSelected;
>+
>+  ok(doc.activeElement.id, tabButtons[0].id, "Changed tab button is focuesd.");
>+});

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444300 - Make tab button to be able to handle the key event.

A command button like ruler has keydown event handler, this patch will
add same event handler to tab button.

Review commit: https://reviewboard.mozilla.org/r/231810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231810/

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8962966 [details]
Bug 1444300 - Make tab button to be able to handle the key event.

Sorry,

I cleared the r? flag since I submitted the wrong patch.

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444300 - Make tab button to be able to handle the key event.

A command button like ruler has keydown event handler, this patch will
add same event handler to tab button.

Review commit: https://reviewboard.mozilla.org/r/231826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231826/

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8962980 [details]
Bug 1444300 - Make tab button to be able to handle the key event.

https://reviewboard.mozilla.org/r/231826/#review238172

Thanks for working on this!

Please see my inline comments.

One more note, after selecting a tab using Enter/Space key,
the focus moves to the panel. Shouldn't the tab stay focused?

Honza

::: devtools/client/framework/components/toolbox-tab.js:66
(Diff revision 1)
>          "aria-pressed": currentToolId === id ? "true" : "false",
>          tabIndex: focusedButton === id ? "0" : "-1",
>          onFocus: () => focusButton(id),
>          onMouseDown: () => selectTool(id),
> +        onKeyDown: (evt) => {
> +          if (evt.key === "Enter" || evt.key === "Space") {

You can't compare to "Space" the correct value is " " (one space character).

Another option would be using evt.keyCode == 32, but it's nice to be consistent and use evt.code (or eve.keyCode) for both Enter and Space.

::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:121
(Diff revision 1)
> +  EventUtils.synthesizeKey("KEY_ArrowLeft");
> +  await onKeyEvent;
> +
> +  onceSelected = toolbox.once("inspector-selected");
> +  EventUtils.synthesizeKey("Space");
> +  await onceSelected;

How come the test is passing even if the key-handler doesn't properly handle pressing Space key?

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444300 - Make tab button to be able to handle the key event.

A command button like ruler has keydown event handler, this patch will
add same event handler to tab button.

Review commit: https://reviewboard.mozilla.org/r/232962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232962/

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8962980 [details]
Bug 1444300 - Make tab button to be able to handle the key event.

https://reviewboard.mozilla.org/r/231826/#review238172

Thanks.
I thought that focus should remain selected tab, but some tool panel will steal the focus. For example webconsole will get focus after opening own panel at first time. I filed the bug for it. (bug 1450599)

> You can't compare to "Space" the correct value is " " (one space character).
> 
> Another option would be using evt.keyCode == 32, but it's nice to be consistent and use evt.code (or eve.keyCode) for both Enter and Space.

Sorry, I misunderstood key event value. I addressed it.

> How come the test is passing even if the key-handler doesn't properly handle pressing Space key?

If test specifies the "SPACE" to the key, EventUtil will send the key event as "SPACE" key property.[1]
However, this test is clearly wrong.
[1] https://searchfox.org/mozilla-central/rev/b80994a43e5d92c2f79160ece176127eed85dcc9/testing/mochitest/tests/SimpleTest/EventUtils.js#1813

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8964224 [details]
Bug 1444300 - Make tab button to be able to handle the key event.

https://reviewboard.mozilla.org/r/232962/#review238438

Looks good to me now, just a couple of minor comments.

R+, assuming that Try is green

Thanks for working on this!
Honza

::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:108
(Diff revision 1)
> +  await onKeyEvent;
> +
> +  let onceSelected = toolbox.once("webconsole-selected");
> +  EventUtils.synthesizeKey("Enter");
> +  await onceSelected;
> +  ok(doc.activeElement.id, tabButtons[1].id, "Changed tab button is focuesd.");

typo: focuesd -> focused

::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:123
(Diff revision 1)
> +
> +  onceSelected = toolbox.once("inspector-selected");
> +  EventUtils.synthesizeKey(" ");
> +  await onceSelected;
> +
> +  ok(doc.activeElement.id, tabButtons[0].id, "Changed tab button is focuesd.");

typo

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1444300 - Make tab button to be able to handle the key event.

A command button like ruler has keydown event handler, this patch will
add same event handler to tab button.

Review commit: https://reviewboard.mozilla.org/r/233006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233006/

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8964283 [details]
Bug 1444300 - Make tab button to be able to handle the key event.

Oh, Sorry.
This request is my mistake. I cleared r? flag due to already granted this patch.

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> Comment on attachment 8964283 [details]
> Bug 1444300 - Make tab button to be able to handle the key event.
> 
> Oh, Sorry.
> This request is my mistake. I cleared r? flag due to already granted this
> patch.

I pushed the patch to mozreview without MozReview-Commit-ID and honza's r+ is cleared. 
So I'll push this patch to m-i after finishing the following try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8708a178dba3cbcfe059aeda93bc6a3a85de64

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

OK. Try result is green except for JSDCov, this jsdcov failure is not related to this patch and it is already filed the bug. So I'll land this patch soon.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efce674123ac
Make tab button to be able to handle the key event. r=honza

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/efce674123ac

Comment 20

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  60.0a1 (2018.03.08) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 61.0b9 on Win 8.1 x64, Mac OS X 10.13 and Ubuntu 14.04 x64.