Tab order is broken when a xul popup is open

RESOLVED FIXED in mozilla1.9alpha8

Status

()

RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: mano, Assigned: enndeakin)

Tracking

unspecified
mozilla1.9alpha8
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

One can easily tab out of an open xul panel, but not back to it.
Flags: blocking1.9?
(Assignee)

Updated

12 years ago
Assignee: jag → enndeakin
Target Milestone: --- → mozilla1.9beta1
(Assignee)

Comment 1

12 years ago
Created attachment 271714 [details] [diff] [review]
focus in panels better

This patch prevents Tab from navigating outside of a panel. Mano, is this what you need?
Prevents as in "pressing tab when the last element within the popup is focused focuses the first element within the popup"?
(Assignee)

Comment 3

12 years ago
(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
(Assignee)

Comment 5

12 years ago
OK, I'll create some tests for this then. 
(Assignee)

Comment 7

12 years ago
Created attachment 272793 [details] [diff] [review]
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.
Attachment #271714 - Attachment is obsolete: true
Attachment #272793 - Flags: superreview?(roc)
Attachment #272793 - Flags: review?(roc)
Who wants to traverse across popup boundaries?
(Assignee)

Comment 9

12 years ago
Created attachment 275178 [details] [diff] [review]
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
Attachment #272793 - Attachment is obsolete: true
Attachment #275178 - Flags: superreview?(roc)
Attachment #275178 - Flags: review?(roc)
Attachment #272793 - Flags: superreview?(roc)
Attachment #272793 - Flags: review?(roc)
+  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.
(Assignee)

Comment 11

12 years ago
(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
(Assignee)

Comment 12

12 years ago
Created attachment 276682 [details] [diff] [review]
updated patch
Attachment #275178 - Attachment is obsolete: true
Attachment #276682 - Flags: superreview?(roc)
Attachment #276682 - Flags: review?(roc)
Attachment #275178 - Flags: superreview?(roc)
Attachment #275178 - Flags: review?(roc)
Attachment #276682 - Flags: superreview?(roc)
Attachment #276682 - Flags: superreview+
Attachment #276682 - Flags: review?(roc)
Attachment #276682 - Flags: review+
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Flags: in-testsuite+
(Assignee)

Comment 13

12 years ago
Created attachment 276968 [details] [diff] [review]
fix the test on Mac by clicking the document first
Attachment #276968 - Flags: review?(gavin.sharp)

Comment 14

12 years ago
This bug has created a problem with --disable-xul builds. Prior to this patch, nsEventStateManager.cpp didn't have a XUL component.
Depends on: 393704
Attachment #276968 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 15

12 years ago
Checked in the Mac test fix
(Assignee)

Comment 16

12 years ago
Still not working, so had to disable the test again on Mac
Depends on: 461981
(Assignee)

Comment 17

10 years ago
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

a year ago
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/abceeaf385f8
handle tab navigation in popups properly, r+sr=roc
You need to log in before you can comment on or make changes to this bug.