385275 - Tab order is broken when a xul popup is open

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Asaf Romano (gone)]

One can easily tab out of an open xul panel, but not back to it.

Comment 1

[Neil Deakin]

Attachment: focus in panels better

This patch prevents Tab from navigating outside of a panel. Mano, is this what you need?

Comment 2

[Asaf Romano (gone)]

Prevents as in "pressing tab when the last element within the popup is focused focuses the first element within the popup"?

Comment 3

[Neil Deakin]

(In reply to comment #2)
> Prevents as in "pressing tab when the last element within the popup is focused
> focuses the first element within the popup"?
> 

Yes

Comment 4

[Asaf Romano (gone)]

Awesome, thanks.

Comment 5

[Neil Deakin]

OK, I'll create some tests for this then. 

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

roc and I aren't the right people to triage the blocking1.9? flag here.  Not sure how to fix that...

Comment 7

[Neil Deakin]

Attachment: focus tab navigation shouldn't step out of a popup

Don't like having to add a flag everywhere. Maybe we can remove the xpcom from nsIFrameTraversal in some way in a follow up bug.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Who wants to traverse across popup boundaries?

Comment 9

[Neil Deakin]

Attachment: better patch

Better patch:
 - tab within a popup works
 - when opening and closing a panel, clear the focus if necessary
 - add a keepfocus attribute to make sure the autocomplete widget which has special focus behaviour where the textbox keeps focus while the popup is open, still works.
 - the changes in layout/xul/base/src are mostly to propagate the tri-state popup type around rather than just the flag if the menu state is set

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (curFocusFrame) {
+    // check if the focus is currently inside a popup
+    popupFrame = curFocusFrame;
+    while (popupFrame && popupFrame->GetType() != nsGkAtoms::menuPopupFrame) {
+      popupFrame = popupFrame->GetParent();
+    }
+  }
+  else {

Use nsLayoutUtils::GetClosestFrameOfType here and comment that some widgets like autocomplete allow focus to be outside the currently topmost popup.

+  if (popupFrame && !nextFocus) {
+    // if no content was found to focus, yet we are inside a popup, try again
+    // from the beginning or end of the popup. Set the current tab index to
+    // the beginning or end.

I don't know the focus code very well, but why is this necessary? Focus already cycles around within the current document, why can't we reuse that cycling?

It might make more sense to define a new IsFrameOfType flag, ePopup or something, and use that instead of these GetType() checks and nsFrameIterator::IsPopupFrame.

+  nsPopupType mPopupType; // true if the popup is a menu, false for a panel

fix comment

'keepfocus' doesn't seem like a great attribute name; it sounds like setting it will give the popup focus, but the reverse is true. Maybe 'autofocus', which defaults to 'true' and can be set to 'false'?

+  // when a panel is closed, blur whatever has focus

"... inside the popup"

+nsXULPopupManager::GetTopMostOpenPanel()

Should this just return the topmost popup, and allow callers to check the type? That would be more flexible for potential other users.

Comment 11

[Neil Deakin]

(In reply to comment #10)
> 
> I don't know the focus code very well, but why is this necessary? Focus already
> cycles around within the current document, why can't we reuse that cycling?
> 

GetNextTabbableContent scans only to the end of document and through later tabindex values. It returns after this and the later code in ShiftFocusInternal either cycles on to the next document, or back to the beginning and calls itself recursively to try again.

For focus in a popup, we need to cycle to the beginning of the popup here instead of moving to the next document.

> It might make more sense to define a new IsFrameOfType flag, ePopup or
> something, and use that instead of these GetType() checks and
> nsFrameIterator::IsPopupFrame.

I'll do this as a follow up bug as there are other places where the popup type is checked.

> 'keepfocus' doesn't seem like a great attribute name; it sounds like setting it
> will give the popup focus, but the reverse is true. Maybe 'autofocus', which
> defaults to 'true' and can be set to 'false'?

I'm going to use 'noautofocus' as I don't like attributes that default to true.
> +nsXULPopupManager::GetTopMostOpenPanel()
> 
> Should this just return the topmost popup, and allow callers to check the type?
> That would be more flexible for potential other users.
> 

OK

Comment 12

[Neil Deakin]

Attachment: updated patch

Comment 13

[Neil Deakin]

Attachment: fix the test on Mac by clicking the document first

Comment 14

[David G King]

This bug has created a problem with --disable-xul builds. Prior to this patch, nsEventStateManager.cpp didn't have a XUL component.

Comment 15

[Neil Deakin]

Checked in the Mac test fix

Comment 16

[Neil Deakin]

Still not working, so had to disable the test again on Mac

Comment 17

[Neil Deakin]

The Mac failure is caused because of the Full Keyboard Access preference. So I'm going to just add a comment to this effect and leave the test disabled.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/abceeaf385f8
handle tab navigation in popups properly, r+sr=roc