503829 - Username/password prompt dialog (or any using commonDialog.xul) no longer has username field focused

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Timothy Nikkel (:tnikkel)]

The username/password prompt dialog (titled Authentication Required) that you get when accessing websites that require a username/password (eg ldap.mozilla.org) no longer has the username field focused. This was caused by bug 494345.

Comment 1

[Marco Zehe (:MarcoZ)]

Are you sure that this is a result of bug 494345? I think I've seen this before and thought it was bug 178324 which is at fault here.

Comment 2

[Timothy Nikkel (:tnikkel)]

My first thought was bug 178324 too, but I bisected and then I reverted the patch in bug 494345 and that fixed the problem.

Comment 3

[alexander :surkov (:asurkov)]

Would be nice to bring someone here familiar with this code. I have no idea how I could break this.

Comment 4

[Timothy Nikkel (:tnikkel)]

Probably has something to do with the text-base binding no longer extending the basecontrol binding, so it no longer implements nsIDOMXULControlElement. And in nsXULElement::IsFocusable, implementing nsIDOMXULControlElement is key. I hope that helps.

Comment 5

[(not currently active) Ted Mielczarek]

Also affects the master password dialog, FWIW.

Comment 6

[Nochum Sossonko [:Natch]]

Same problem for prompt() too. Basically, anything using dialog.xul has this problem apparently, which is anything that uses nsIPrompt.

Comment 7

[Neil Deakin]

This is because the 'info.body' <description> has -moz-user-focus: normal. Neil told me this was to allow the text to be selected by the user, presumably by the keyboard. Dialogs always focus the first tabbable element in them when opened.

It worked before because the presence of tabindex="-1" was making the element so it wasn't part of the taborder. Now that <description> is not an nsIDOMXULControlElement, tabindex no longer applies to it, so it now appears in the tab order and thus becomes focused.

Comment 8

[(not currently active) Ted Mielczarek]

This makes my browser a lot less usable. I think we should back out the patch that broke this if we can't get a fix pre-alpha 1.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #7)
> This is because the 'info.body' <description> has -moz-user-focus: normal. Neil
> told me this was to allow the text to be selected by the user, presumably by
> the keyboard.

No, it should be selectable with the mouse as well. The bug for this was bug 60513.

Comment 11

[Neil Deakin]

(In reply to comment #10)
> No, it should be selectable with the mouse as well. The bug for this was bug
> 60513.

I meant that it was made focusable so that keyboard selection was allowed as well.

Comment 12

[philippe (part-time)]

Would bug 506822 -edit field in edit style panel not focussed anymore- be a dupe of this one ?

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #11)
I see... it seems to be keyboard-selectable without being focusable though, via cmd_selectAll.

Comment 16

[Neil Deakin]

Attachment: just focus the textbox when opened

Comment 17

[[e-mail]]

(In reply to comment #7)
> This is because the 'info.body' <description> has -moz-user-focus: normal. Neil
> told me this was to allow the text to be selected by the user, presumably by
> the keyboard.
Actually there's a context menu for this, but it seems that (perhaps due to your focus manager changes) it works even without -moz-user-focus?

Of course the now useless tabindex="-1" attribute should be removed either way.

Comment 19

[Neil Deakin]

Attachment: remove tabindex

> Actually there's a context menu for this, but it seems that (perhaps due to
> your focus manager changes) it works even without -moz-user-focus?

It doesn't work if the textbox has focus. If nothing has focus, the context menu will work since the window's command dispatcher will use the window's selection.

Comment 20

[[e-mail]]

Comment on attachment 394824 [details] [diff] [review]
remove tabindex

>+  else {
>+    if (gCommonDialogParam.GetInt(4) == 1)
>+      document.getElementById("password1Textbox").focus();
>+    else
>+      document.getElementById("loginTextbox").focus();
>+  }
Sorry for not noticing before, but this doesn't preselect the text in the textfield, although that was the previous behaviour.
For example, javascript:void(prompt('message','preselected'))

Comment 21

[[e-mail]]

Hmm, should the command dispatcher's advanceFocus select the focused textbox or are you supposed to use the focus manager's moveFocus these days?

Comment 22

[Neil Deakin]

Attachment: use select() instead of focus()

Comment 23

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/76581fc3f3ea

Comment 24

[Michael Newton]

This is now WFM

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090820 Minefield/3.7a1pre

Built from http://hg.mozilla.org/mozilla-central/rev/f04f32458068

Comment 25

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Comment 26

[Nick Thomas [:nthomas] (UTC+12)]

This bug is blocking1.9.2+ but the patch still needs to land on that branch. Could we get it landed so this isn't in 3.6b1, and dogfooding 3.6 nightlies is less painful ?

Comment 27

[Marco Zehe (:MarcoZ)]

Pushed this to 1.9.2 on enn's behalf so we can start dogfooding it:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/155c850ccb9d

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Verified fixed on 1.9.2 on all platforms with builds like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090909 Namoroka/3.6a2pre ID:20090909040930