Closed Bug 1444300 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
Summary: Cannot activate a tab in the developer toolbar after highlighting it → Cannot activate a tab in the DevTools tab bar after highlighting it
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.
See Also: → 1242852
Duplicate of this bug: 1443090
Attached file tab-key-handle (obsolete) —
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 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.
(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!
Attachment #8962580 - Attachment is obsolete: true
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.
Attachment #8962966 - Flags: review?(odvarko)
Attachment #8962966 - Attachment is obsolete: true
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
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?
Attachment #8962980 - Flags: review?(odvarko) → review-
Attachment #8962980 - Attachment is obsolete: true
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 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
Attachment #8964224 - Flags: review?(odvarko) → review+
Attachment #8964224 - Attachment is obsolete: true
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.
Attachment #8964283 - Flags: review?(odvarko)
(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
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.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efce674123ac
Make tab button to be able to handle the key event. r=honza
https://hg.mozilla.org/mozilla-central/rev/efce674123ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.