1166825 - GTK port has broken keyboard navigation in the menubar

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Samuel Bronson]

Reproduction steps:

 1. Press F10
 2. Press the right arrow key several times

Actual results:

 1. This selects the first item on the File menu, File > New Tab.
 2. This ends up selecting View > Toolbars > Menubar.

Expected results:

 1. This should
     a. select the File menu on the bar (as it is the first menu on the menubar)
     b. [ideally] display the menu
     c. NOT selected any item on the File menu
 2. This should have selected each of the other menus in turn, eventually either
     a. staying on Help
    or
     b. cycling back around to File.

(#2 should just fall out of #1 for free, but I included it here to illustrate the problem.)

Currently, you get 1a and 1b right, but the GTK port fails on 1c. For comparison, the Windows port gets 1a and 1c right, but misses on the "nice to have" 1b; xfce4-terminal (GTK 2) and wireshark (GTK 3), whose menubars are hopefully typical of menubars in GTK apps, do all three.  Each of these except for the GTK port of Firefox ends up on 2b.

Or, in tabular form:

1a  1b  1c  2b  Testcase
--  --  --  --  --------
 X   X          Firefox (GTK port)     <--- Subject of this bug report
 X       X   X  Firefox (Windows port)
 X   X   X   X  xfce4-terminal (GTK 2)
 X   X   X   X  Wireshark      (GTK 3)

Comment 1

[Colomban Wendling]

Attachment: GTK: Do not focus the first item in the menu when open with F10

This has accessibility impact as it makes keyboard navigation in the menus both harder and inconsistent with other apps.

Attached a patch to fix this by not focusing the first elements initially when open with F10 (but keeps the current and expected behavior when triggered e.g. with Alf+F).

Could someone please assign me?  Also, product should be Core, and it doesn't only affect Firefox but at least Thunderbird too.

Comment 2

[Colomban Wendling]

Any news on this?

Comment 3

[Alex ARNAUD]

Dear all,

On Firefox Nightly from today I can reproduce this issue. It would be great to have your patch integrated in the next Firefox and Thunderbird releases.

Best regards.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8844851 [details] [diff] [review]
GTK: Do not focus the first item in the menu when open with F10

Neil is probably a better match, and for a regular review (a11y-review is specific to a11y usage verification).

Neil please redirect if you are the wrong reviewer.

Comment 5

[Neil Deakin]

Comment on attachment 8844851 [details] [diff] [review]
GTK: Do not focus the first item in the menu when open with F10

>+  nsMenuFrame* currentItem = nullptr;
>+  if (item && mActiveMenuBar && NS_DIRECTION_IS_INLINE(theDirection)) {
>+    currentItem = item->Frame()->GetCurrentMenuItem();
>+    // if nothing is selected in the menu and we have a menubar, let it
>+    // handle the movement not to steal focus from it.
>+    if (!currentItem) {
>+      item = nullptr;
>+      itemFrame = mActiveMenuBar;

itemFrame isn't used after this line so no point is setting it.

All of the changes need to be GTK-specific as the current behaviour is correct on Windows.

The behaviour implemented by the patch (or for that matter our current behaviour) doesn't match what I see on Ubuntu where cursor left or right always switches menus and never highlights the item, regardless of what was selected in the previous menu. I'd be willing to assume though that Ubuntu may be different here, but it would be good to confirm this.

Did you run the test test_menubar.xul ? It will likely need updating for this change.

Comment 6

[Samuel Thibault]

Hello,

> The behaviour implemented by the patch (or for that matter our current behaviour) doesn't match what I see on Ubuntu where cursor left or right always switches menus and never highlights the item, regardless of what was selected in the previous menu.

I guess you are using the menus integrated at the top of the screen as provided by the Ubuntu desktop? (they don't seem to even support the F10 shortcut)

Here the bug is about menus non integrated at the top of the screen, so I guess it's not surprising that Ubuntu does not have the issue.

Comment 7

[Samuel Thibault]

This is confirmed by e.g. installing the ubuntu-mate-desktop package and running mate-session instead of the Ubuntu session. The menus are then not integrated at the top of the screen, and the behavior is as described in this bug.

Comment 8

[Samuel Thibault]

Attachment: GTK: Do not focus the first item in the menu when open with F10

Hello,

Here is a reworked patch, which makes the changes GTK-only and drops the useless itemFrame part.

About test_menubar.xul, window_menubar.xul probably needs to be updated indeed (it contains DOMMenuItemActive item1), but I don't know how to run a single xul test? (firefox refuses to execute remote xul, and I didn't find the former option to bypass that)

Comment 9

[Samuel Thibault]

(ok, found my way to running 
/var/tmp/firefox-55.0.3/build-browser/_virtualenv/bin/python    ./_tests/testing/mochitest/runtests.py  --flavor chrome ../toolkit/content/tests/widgets/
)

Comment 10

[Samuel Thibault]

Attachment: F10

Hello,

Here is a reworked patch, which makes the changes GTK-only, drops the useless itemFrame part, and updates test_menubar.xul (I ran the whole toolkit/content/tests/widgets testsuite to be sure)

Samuel

Comment 11

[Neil Deakin]

Comment on attachment 8934513 [details] [diff] [review]
F10

>+    // if nothing is selected in the menu and we have a menubar, let it
>+    // handle the movement not to steal focus from it.
>+    if (!currentItem)
>+      item = nullptr;

Use braces around single line conditions.

Also I know the file is inconsistent here, but new comments should start sentences with a capital letter.

Comment 12

[Samuel Thibault]

Attachment: F10

Here a patch fixed according to review.

Comment 13

[Samuel Thibault]

Attachment: F10

This should fix the patch formatting for commit.

Comment 14

[Samuel Thibault]

FTR, I ran the whole testsuite with the patch applied, brings the same results as without the patch, so can somebody add a checkin-needed tag? (it seems I can't do it myself)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef30bd55328
do not focus the first item in the menu when open with F10, helped by Samuel Thibault, r=enndeakin

Comment 16

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5ef30bd55328