1073551 - When posting login form that triggers password saving prompt, focus ends up in limbo

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Gijs (out for now; he/him)]

We should focus (something in) the document (ideally) or the URL bar. Now it's not clear where focus goes.

(In reply to Marco Zehe (:MarcoZ) from bug 990045 comment #19)
> One thing I did notice is that, when the password prompt appears, and the
> new page is loaded, focus doesn't seem to go to the document. Instead, when
> now pressing tab, it again lands on the password doorhanger, as if that's
> the first thing in the document's tab order. So when removing the auto
> focus, it should be made sure that focus is on the new document when it
> loads and not somewhere in limbo.

Comment 1

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8669071 [details]
MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

https://reviewboard.mozilla.org/r/21119/#review19011

::: toolkit/modules/PopupNotifications.jsm
(Diff revision 1)
> -      popupnotification.setAttribute("noautofocus", "true");

How is adding the noautofocus attribute to the markup different than when it was added through the JS? Does it just come down to timing and the focus during binding creation or something like that?

::: toolkit/modules/PopupNotifications.jsm:872
(Diff revision 1)
> +    this.panel.removeAttribute("noautofocus", "true");

From this patch, it looks like the old code never removed the noautofocus attribute, so how was the focus getting placed in the notification before?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8669071 [details]
> MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless
> explicitly opened using mouse or keyboard, r?jaws
> 
> https://reviewboard.mozilla.org/r/21119/#review19011
> 
> ::: toolkit/modules/PopupNotifications.jsm
> (Diff revision 1)
> > -      popupnotification.setAttribute("noautofocus", "true");
> 
> How is adding the noautofocus attribute to the markup different than when it
> was added through the JS? Does it just come down to timing and the focus
> during binding creation or something like that?
> 
> ::: toolkit/modules/PopupNotifications.jsm:872
> (Diff revision 1)
> > +    this.panel.removeAttribute("noautofocus", "true");
> 
> From this patch, it looks like the old code never removed the noautofocus
> attribute, so how was the focus getting placed in the notification before?

The old code set the attribute on the popupnotification element, which is inside the panel. That has 0 effect.

The new code sets and removes it on the element depending on whether focus needs to go into it (effectively only allowing that to happen if the user explicitly makes the doorhanger appear). Because of this being dynamic, I didn't add it to the markup, although come to think of it, it probably needs to be on the element from the start so it works the first time (as the patch as-is adds it when the popup is hidden).

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8669071 [details]
MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

(In reply to :Gijs Kruitbosch from comment #3)
> Because of this being dynamic, I
> didn't add it to the markup, although come to think of it, it probably needs
> to be on the element from the start so it works the first time (as the patch
> as-is adds it when the popup is hidden).

Oh, I did add it to the markup already. Nevermind, then I just re-request review...

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8669071 [details]
MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

https://reviewboard.mozilla.org/r/21119/#review19021

Ok, I see that it is now being set on the panel instead of the popupnotification. Thanks for the clarification!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/310287f0e8f6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/310287f0e8f6

Comment 8

[:Gijs (out for now; he/him)]

Jared, can you give me a second opinion on how safe you think this change is for uplift? I think it's safe enough to request aurora+beta uplift, but I also feel like I might be biased. Thoughts?

Comment 9

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this issue with Firefox 43 beta 4, build ID: 20151116155110 on Windows 10 x64.

I can confirm this issue being fixed on latest Aurora 44 (build ID: 20151117004023).

Tested on:
*Windows 10 x64
*Ubuntu 14.04 x64
*Mac OS X 10.8.5

Comment 10

[:Gijs (out for now; he/him)]

Oops, seems like the beta consideration here got dropped. That's a shame, but it's not too late. Considering this has baked for literally 1.5 month on nightly + aurora and I am unaware of regressions, going to request uplift for beta.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8669071 [details]
MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

Approval Request Comment
[Feature/regressing bug #]: setting focus appropriately when password doorhanger pops up
[User impact if declined]: we do not set focus appropriately. This is problematic for keyboard and accessibility tool users
[Describe test coverage new/current, TreeHerder]: nope!
[Risks and why]: low, considering baking time (1.5 month now), lack of reported regressions, the size of the fix (ie very small), and that this is essentially removing no-op, broken code that we already landed to try to do exactly what this bug was about, and replaced it with code that, err, actually does it. :-)
[String/UUID change made/needed]: nope!

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Sigh, sorry for missing this needinfo.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I agree with the beta uplift request.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8669071 [details]
MozReview Request: Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r?jaws

Accessibility fix for recent regression, looks good to me.
Please uplift to beta.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this sounds like a regression, marking affected for 43.

Comment 16

[Carsten Book [:Tomcat]]

This cause problems on beta:

grafting 306688:310287f0e8f6 "Bug 1073551 - fix doorhangers to not steal focus unless explicitly opened using mouse or keyboard, r=jaws"
merging toolkit/modules/PopupNotifications.jsm
warning: conflicts during merge.
merging toolkit/modules/PopupNotifications.jsm incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

can you take a look, thanks!

Comment 17

[:Gijs (out for now; he/him)]

Landed on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/7434475a5d02

Comment 18

[Mihai Boldan, Desktop QA [:mboldan]]

I was able to reproduce this issue with Firefox 43 beta 4, build ID: 20151116155110 on Windows 8 x86.
This issue is no longer reproducible on Firefox 43 beta 8, build ID: 20151201152349 across platforms[1].

[1]Windows 8 x86, Mac OS X 10.11 and Ubuntu 14.04 x86.