653230 - Panel unexpectedly receives focus when noautofocus="true"

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[:Margaret Leibovic]

While investigating bug 616136, I found that setting noautofocus="true" doesn't stop popup notification panels from receiving focus when they are opened, but that seems to only happen when a chrome element has focus at the time the
panel is opened. I'm not familiar with the way focus works, so this is a guess by me. Nevertheless, we need a way to prevent a panel from getting focus when it opens.

Comment 1

[Neil Deakin]

Is there some steps I can use to test this? Which notifications are causing this problem?

Comment 2

[:Margaret Leibovic]

This problem used to apply to popup notifications until the noautofocus="true" was removed in bug 594586. We removed that so that these notifications will just always get focus for the sake of consistency. You can test this by reverting the change from that bug.

I think we would like to put back the noautofocus="true" if we can get it to work as expected.

Comment 3

[Neil Deakin]

I tested this and don't see any bug, and I can't follow the discussion in bug 594586 enough to understand what prompted this bug to be filed.

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to Neil Deakin from comment #3)
> I tested this and don't see any bug, and I can't follow the discussion in
> bug 594586 enough to understand what prompted this bug to be filed.

Neil, the problem is that, as soon as you get a doorhanger notification, keyboard focus is no longer inside the currently loaded page. If you press Tab after such a password prompt (for example) appears, you are thrown into this panel rather than staying in the context of the currently loaded page. In Firefox 3.6 and earlier, where we had the simpler notification popup, focus would always stay within the page even when the notification bar appeared. This is no longer the case. So to reproduce:

1. Provoke a doorhanger, like a password remembrance prompt.
2. Press tab once this appears.

Expected: You'd go to the first focusable item on the newly loaded page.
Actual: You land in the doorhanger panel on the "Save password" button menu. And there, you're bitten, incidentally, by bug 653226.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> 1. Provoke a doorhanger, like a password remembrance prompt.
> 2. Press tab once this appears.
> 
> Expected: You'd go to the first focusable item on the newly loaded page.

Wouldn't this break keyboard accessibility of the doorhanger? How would you access the doorhanger using the keyboard otherwise?

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #5)
> Wouldn't this break keyboard accessibility of the doorhanger? How would you
> access the doorhanger using the keyboard otherwise?

With the Firefox 3.6 notification bar, the save password button had a mnemonic like Alt+P or something (I don't recall). This mnemonic is now also present inside the doorhanger, and because it is an accessibility alert type, the mnemonic key is spoken, too.
Alternatively, if the panel is present, it could be included in the F6 cycling order, directly preceeding the document, so that if a doorhanger appears, and the user *chooses* to interact with it, they can press Shift+F6 once to get there and then tab through the buttons or choices.
Of course, pressing the mnemonic key from inside the document would still work.

Comment 7

[Marco Zehe (:MarcoZ)]

Note that, in order to move this forward, I've collected all information and a proposed solution on this wiki page, using the doorhanger as the main example for this:
https://wiki.mozilla.org/Accessibility/Doorhanger_Keyboard_Navigation

Comment 8

[Neil Deakin]

Attachment: Patch, put panels in the document tab order

Comment 9

[Neil Deakin]

Note that this patch should adds panels with focusable element to the F6 document tab order. It doesn't change any other behaviour. Probably noautofocus should be set on the panel.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 591826 [details] [diff] [review]
Patch, put panels in the document tab order

This feels right to me. Focus is still circling around the password remembrance doorhanger, but I suspect that's a problem in the widget that needs to be solved separately.

Comment 11

[Neil Deakin]

Attachment: Improved patch and add a test

This fixes some tests that failed with the previous patch because the starting content was null.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 596746 [details] [diff] [review]
Improved patch and add a test

Coding style in general is:
if (expr) {
  stmt;
}

>+    if (aType == MOVEFOCUS_FORWARDDOC || aType == MOVEFOCUS_BACKWARDDOC) {
>+      // When moving between documents, make sure to get the right
>+      // starting content in a descendant.
>+      nsCOMPtr<nsPIDOMWindow> focusedWindow;
>+      startContent = GetFocusedDescendant(aWindow, true, getter_AddRefs(focusedWindow));
>+    }
>+    else {
} else {


>+nsIContent*
>+nsFocusManager::GetNextTabbableDocument(nsIContent* aStartContent, bool aForward)
>+{
>+  // If currentPopup is set, then the starting content is in a panel.
>+  nsIFrame* currentPopup = nsnull;
>+  nsCOMPtr<nsIDocument> doc;
>   nsCOMPtr<nsIDocShellTreeItem> startItem;
>-  if (mFocusedWindow) {
>+
>+  if (aStartContent) {
>+    doc = aStartContent->GetCurrentDoc();
>+    if (doc) {
>+      startItem = do_QueryInterface(doc->GetWindow()->GetDocShell());
>+    }
>+
>+    // Check if the starting content is inside a panel. Document navigation
>+    // must start from this panel instead of the document root.
>+    nsIContent* content = aStartContent;
>+    while (content) {
>+      if (content->Tag() == nsGkAtoms::panel) {
You want to check namespace here.

Please ask review also from some a11y developer.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 596746 [details] [diff] [review]
Improved patch and add a test

asking feedback from Marco per comment #12

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 596746 [details] [diff] [review]
Improved patch and add a test

I'm going to test this patch on a local build, but a question beforehand: Does it make sense to test tab key behavior here as well? Esp the focus behavor when the panel goes away, where focus goes, and where tab and shift+tab takes the user in such a situation. An example is the pressingof "space" on the "Close this message" button of the password doorhanger. Focus should be returned to the document, then. I think it would be good to test that focus does return to a defined position/element when a panel with a previously focused item goes away.

Comment 15

[Marco Zehe (:MarcoZ)]

Comment on attachment 596746 [details] [diff] [review]
Improved patch and add a test

So, the bug makes the focus handling more consistent when there are focusable items in panels, but it does not address the original problem this bug was filed for, that the doorhanger panel automatically receives focus whenever it appears. This still happens even with the patch. So, not sure if I should f+ or f- this, since it definitely improves things, but does not fix the original bug yet.

Comment 16

[Neil Deakin]

That just requires someone to add noautofocus="true" to the panel you're testing. (as the original bug describes)

Comment 17

[alexander :surkov (:asurkov)]

Neil, I guess Olli didn't meant review actually but feedback from a11y guy. You have one from Marco, that should be enough.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Well, ok, feedback+ should be enough :)

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 596746 [details] [diff] [review]
Improved patch and add a test

canceling review request then :)

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c97dcc479c62

Comment 21

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I think the fix for this bug introduced a regression in BlueGriffon:
if I launch BlueGriffon 1.4 and create a blank document, type
a few chars and open the CSS Properties panel (that is a XUL floating panel),
using the left arrow to move the caret works fine in the editor.
If I do the same with BlueGriffon 1.4.1 (fix for the current bug is in),
then the first menulist in my CSS Properties panel acquires the focus
when the left arrow key is pressed and the editor loses the focus! I have a
lot of users' feedback about this.

Should the code in this fix check if a panel is floating? We have a rather
strong behavioural difference here between floating panels and contextual
popups.

Comment 22

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to Daniel Glazman from comment #21)

> I think the fix for this bug introduced a regression in BlueGriffon:

Nevermind. I finally found the guilty commit and this is not the current bug but
the fix for bug 669026. Sorry for the spam.

Comment 23

[James Teh [:Jamie]]

(In reply to Marco Zehe (:MarcoZ) from comment #15)
> [The patch] does not address the original problem this
> bug was filed for, that the doorhanger panel automatically receives focus
> whenever it appears.
(In reply to Neil Deakin from comment #16)
> That just requires someone to add noautofocus="true" to the panel you're
> testing. (as the original bug describes)
Marco, was a bug ever filed to have this done for doorhangers? This still doesn't appear to be fixed. If not, I'll file one. :)

Comment 24

[Marco Zehe (:MarcoZ)]

Jamie, feel free to file it! :)

Comment 25

[James Teh [:Jamie]]

As i understand it, this patch was supposed to put doorhangers in the f6 tab order (for want of a better term). However, I can't f6 or shift+f6 to an "update saved password" doorhanger. Am I misunderstanding the purpose of this patch or has it regressed?

Comment 26

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #25)
> I can't f6 or shift+f6 to an
> "update saved password" doorhanger.
Filed bug 972117.