Closed Bug 385275 Opened 13 years ago Closed 13 years ago

Tab order is broken when a xul popup is open

Categories

(Core :: XUL, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: mano, Assigned: enndeakin)

References

Details

Attachments

(2 files, 3 obsolete files)

One can easily tab out of an open xul panel, but not back to it.
Flags: blocking1.9?
Assignee: jag → enndeakin
Target Milestone: --- → mozilla1.9beta1
Attached patch focus in panels better (obsolete) — Splinter Review
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"?
(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
OK, I'll create some tests for this then. 
roc and I aren't the right people to triage the blocking1.9? flag here.  Not sure how to fix that...
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)
Flags: blocking1.9? → blocking1.9+
Who wants to traverse across popup boundaries?
Attached patch better patch (obsolete) — Splinter Review
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.
(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
Attached patch updated patchSplinter Review
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+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Attachment #276968 - Flags: review?(gavin.sharp)
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+
Checked in the Mac test fix
Still not working, so had to disable the test again on Mac
Depends on: 461981
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.
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.