Open Bug 1541787 Opened 5 years ago Updated 2 years ago

PanelMultiView Keyboard navigation gets out of sync if a control is focused via other means

Categories

(Firefox :: Toolbars and Customization, defect, P3)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect

Tracking

()

People

(Reporter: Jamie, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Note that at time of writing, you won't see this in Nightly because PanelMultiView keyboard navigation is disabled for the main overflow and Site Identity panels. That causes a bunch of problems, though, so it's being re-enabled in bug 1477673. This bug is not specific to the new implementation in bug 1477673; it would also have occurred, for example, in the Firefox 65 Site Identity panel (before we disabled key nav).

STR:

  1. Ensure that disablekeynav is not set on the Site identity panel.
  2. Open http://www.popuptest.com/popuptest1.html
  3. Open the Site Identity panel.
  4. Click (with the mouse) the allow/block selector for Open Pop-up Windows.
  5. Press tab.
    • Expected: Focus should move to the "Show 6 blocked pop-ups…" link.
    • Actual: Focus moves to the "Show Connection Details" button.
  • This occurs because PanelMultiView maintains a separate "selectedElement" for keyboard navigation. It sets this during keyboard navigation and always navigates relative to that. That is, it doesn't use the real DOM focus. When you focus by clicking with the mouse, this doesn't get set.
  • We do need to keep track of the last focused control because we want to restore focus when going back from a subview.
  • A big gotcha is that clicking a button to open a subview will focus it, but we don't want to restore focus in that case. That means setting selectedElement using the focusin event doesn't work as expected.

The way I think we can fix this is to:

  1. Get rid of the selectedElement property:
    https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/browser/components/customizableui/PanelMultiView.jsm#1591
  2. For most cases where that selectedElement property was previously used, use DOM focus. That is, to retrieve it, use this.document.activeElement. To set it, modify focusSelectedElement to take an element argument and pass the appropriate element wherever focusSelectedElement is called.
  3. When activating a button with the keyboard, set a new property (restoreFocusElement?) to that button. Only use this when activating the view again (going back from a sub-view).

Gijs, thoughts appreciated.

This is the test we'll add to browser_PanelMultiView_keyboard.js for this (currently failing):

// Test that keyboard navigation behaves as expected after focusing a control
// without the keyboard.
add_task(async function testTabAfterNonKeyboardFocus() {
  await openPopup();
  gMainTabOrder[1].focus();
  is(document.activeElement, gMainTabOrder[1],
     gMainTabOrder[1].id + " focused");
  await expectFocusAfterKey("Tab", gMainTabOrder[2]);
  await hidePopup();
});

(This test file is introduced in bug 1477673.)

IMO, this is low priority because I envisage that users who click a menulist or the like with the mouse are not likely to press the tab key after that; i.e. if they used the mouse to get to a control, they'll probably use the mouse to get to the next control in the same panel. Still, it's weird and it's annoying for tests.

(In reply to James Teh [:Jamie] from comment #0)

The way I think we can fix this is to:

  1. Get rid of selectedElement.
  2. For most keyboard navigation, use DOM focus.
  3. When activating a button with the keyboard, set a new property (restoreFocusElement?) to that button. Only use this when activating the view again (going back from a sub-view).

This seems sane to me, yep.

Priority: -- → P3
Mentor: jteh
Whiteboard: [lang=js]

Hi!
I am interested in taking this up. I am new to Open Source Development ( haven't contributed yet ) and would really appreciate some help in getting started. I am skilled in JavaScript, ReactJS, Node.JS, HTML and CSS.

I found out about this bug through https://codetribute.mozilla.org. Could someone please tell me how to begin? Thanks a lot!

Flags: needinfo?(jteh)

Sure! Thanks for your willingness to help!
With regard to how to contribute, these documents are a good place to start:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
There's a bunch of other related documentation here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

With regard to this specific bug, I've provided some info about how to fix it in comment 0 (see "The way I think we can fix this is"). (I've just updated that comment with some additional info and links.)

Flags: needinfo?(jteh)

Your explanation of the process to fix the bug is very helpful and extensive, however to understand what is going on better I tried to search for some documentation on PanelMultiView ( I searched here : https://firefox-source-docs.mozilla.org ) but was unable to find anything.

Is there anywhere I can find information about what the file PanelMultiView.jsm does and how it works ? Thanks a lot!

Flags: needinfo?(jteh)

There's no separate documentation that I'm aware of for PanelMultiView. There's a large code comment at the top of PanelMultiView.jsm which broadly explains what it does.

For the purposes of this bug, you only need to be concerned with the keyboard navigation portions of panelMultiView. General concepts:

  1. PanelMultiView is used for things like the Site Information panel, the Firefox menu, the Protections panel, etc.
  2. There can be multiple views in a PanelMultiView. Pressing a button might open a subview, after which you can press the Back button to return to the parent view.
  3. Because these views have special keyboard navigation requirements (e.g. we want the up and down arrow keys to move through some controls so they feel like menus), PanelMultiView manages keyboard navigation itself, rather than relying on standard document keyboard navigation provided by the browser.
  4. The core of the keyboard navigation code for a view is in the keyNavigation method. You shouldn't have to change most of this, though, except for the code which focuses a newly selected button for up arrow/down arrow/tab keys, as explained in comment 0.
  5. The other area you need to be concerned with is the code which restores focus when returning to a previous view, also noted in comment 0.
Flags: needinfo?(jteh)

(In reply to :Gijs (he/him) from comment #2)

(In reply to James Teh [:Jamie] from comment #0)

The way I think we can fix this is to:

  1. Get rid of selectedElement.
  2. For most keyboard navigation, use DOM focus.
  3. When activating a button with the keyboard, set a new property (restoreFocusElement?) to that button. Only use this when activating the view again (going back from a sub-view).

This seems sane to me, yep.

I accidentally ran across this bug again. I've just realized, this will break using cut/copy/paste buttons inside the hamburger panel, because they only work if "real" focus stays where it is.

A simpler solution (I think?) would be to add a focusin event listener that notices when something in the area governed by the panel receives "real" focus, and updating selectedElement based on that. Jamie, how does that sound?

Flags: needinfo?(jteh)

(In reply to :Gijs (he/him) from comment #7)

I've just realized, this will break using cut/copy/paste buttons inside the hamburger panel, because they only work if "real" focus stays where it is.

I don't follow why anything in the solution I suggested would change the current behaviour. We already focus the selected element whenever we change the selectedElement property; see here, here and here. I'm just suggesting that we just stop tracking it separately (so the two can't get out of sync) and just use DOM focus alone.

A simpler solution (I think?) would be to add a focusin event listener that notices when something in the area governed by the panel receives "real" focus, and updating selectedElement based on that. Jamie, how does that sound?

I already explored that option; see comment 0:

A big gotcha is that clicking a button to open a subview will focus it, but we don't want to restore focus in that case. That means setting selectedElement using the focusin event doesn't work as expected.

The scenario is this:

  1. You click a button to open a sub-view.
  2. The button gets focus. focusin gets fired, so we update selectedElement.
  3. The sub-view opens.
  4. You click Back, moving to the previous sub-view.
  5. The focus gets restored to selectedElement, which was set to the button in (2)... but mouse users apparently don't want that.

Let me know if I'm missing something here. :)

Flags: needinfo?(jteh)

(In reply to James Teh [:Jamie] from comment #8)

(In reply to :Gijs (he/him) from comment #7)

I've just realized, this will break using cut/copy/paste buttons inside the hamburger panel, because they only work if "real" focus stays where it is.

I don't follow why anything in the solution I suggested would change the current behaviour. We already focus the selected element whenever we change the selectedElement property; see here, here and here. I'm just suggesting that we just stop tracking it separately (so the two can't get out of sync) and just use DOM focus alone.

Oh right - this is already broken. That's kind of sad... though I guess for cut/copy/paste, there are shortcuts so it shouldn't bother people too much.

A simpler solution (I think?) would be to add a focusin event listener that notices when something in the area governed by the panel receives "real" focus, and updating selectedElement based on that. Jamie, how does that sound?

I already explored that option; see comment 0:

Right - I just shouldn't comment on bugs in haste, clearly!

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.