513186 - Button in alert() dialog no longer gets initial focus

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Jesse Ruderman]

I assume this was caused by bug 494345 (see bug 503829 comment 7).

In general, non-editable text labels should be skipped by the "focus the first focusable thing" heuristic.

Comment 1

[Neil Deakin]

Attachment: possible patch

This patch just steps past the description when opening the dialog. Not a great fix I think, but maybe someone has a better idea?

Comment 2

[Neil Deakin]

Maybe people from bug 335089 would have an idea.

Comment 3

[[e-mail]]

I take it there's no way to emulate the old "focusable, but not tabbable" behaviour that the <description> used to have?

Comment 4

[Neil Deakin]

(In reply to comment #3)
> I take it there's no way to emulate the old "focusable, but not tabbable"
> behaviour that the <description> used to have?

Not currently, unless we add support for tabindex to all xul elements and not just nsIDOMXULControlElements. Or ideally, just fix bug 474440.

Comment 5

[Neil Deakin]

Attachment: another approach

This approach adds an initialfocus attribute which may be used to specify the initial focus in a dialog. This patch also fixes bug 517819.

Thoughts?

Comment 6

[[e-mail]]

Comment on attachment 402418 [details] [diff] [review]
another approach

I don't think this patch is correct as written. For commonDialog.xul, the focus priority is the first text field (if any) otherwise the default button (except for the Mac), and it looks like this patch would break that.

Maybe you should have an ignoreinitialfocus attribute, and dialog.xml should look for non-tab elements without that attribute to find something to focus?

>+            var initialFocus = dialog.initialFocus;
>+            initialFocus = document.getElementById(dialog.initialFocus);
???

>+              initialFocus.focus();
What if it's a textbox?

Comment 7

[[e-mail]]

And you could set ignoreinitialfocus on commonDialog's checkbox too, and then dialog.xml would handle focus correctly without needing commonDialog.js help.

Comment 8

[Neil Deakin]

Ah yes, I had meant to make the commonDialog changes mac specific behaviour. I'll think about how an ignoreinitialfocus attribute might work.

Comment 9

[Neil Deakin]

Attachment: what about this

This doesn't fix bug 517819 due to it focusing the inner input, although I can't get that bug to be reproduced any more.

Comment 10

[[e-mail]]

(In reply to comment #7)
> And you could set ignoreinitialfocus on commonDialog's checkbox too, and then
> dialog.xml would handle focus correctly without needing commonDialog.js help.
Hmm, that's not quite true, since the desired outcome is
* If textfield exists, focus it
* If non-Mac, focus default button
* If checkbox exists, focus it
* Otherwise, focus leftmost button
So the checkbox would have to #ifdef the noinitialfocus attribute, which is not worth it just to avoid one (excluding #ifdef) line of code in commonDialog.js

Comment 11

[[e-mail]]

Comment on attachment 403337 [details] [diff] [review]
what about this

>+<script type="application/javascript" 
>+        src="chrome://mochikit/content/MochiKit/packed.js"/>
Nit: trailing space after "application/javascript"

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this ready to land?

Comment 13

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/b8aa319fa2dd

This bug adds a boolean attribute for all xul elements 'noinitialfocus' which, when true, indicates that when the element is in a dialog, the element is skipped when determining which element should be initially focused in the dialog. For example, generally the first focusable element will be focused (varies on each platform). This attribute may be used to skip those elements when checking for that first element.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

I had to back this out because test_dialogfocus.xul was failing on OS X with:

Error 0 (Unknown error: 0).6153 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_dialogfocus.xul | Test timed out.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1254865752.1254867383.20632.gz

Comment 15

[Neil Deakin]

Bug is still fixed on trunk though.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Indeed, sorry about that.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Can this land on mozilla-1.9.2? It blocks the beta ...

Comment 18

[Neil Deakin]

Woops, I checked it earlier today, with the test disabled on Mac.

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> Woops, I checked it earlier today, with the test disabled on Mac.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/812c49feab66

Comment 20

[Eric Shepherd [:sheppy]]

To what elements does this apply? At first blush, this seems more an implementation detail of how alerts work internally than something that necessarily cries out for documenting.

Comment 21

[Neil Deakin]

Normally, a platform-specific algorithm is used to determine what element is first focused when a dialog opens. For instance, this might select the first element in the dialog. This bug adds a 'noinitialfocus' attribute to all xul elements which when set to true, indicates that that element should be skipped when determining what should be first focused. This is useful for focusable descriptions as in the alert box. If noinitialfocus is not set or false, that element may be focused first, assuming the algorithm determines that it should be initially focused. The attribute only has any effect in dialogs (or wizards).

Comment 22

[zug_treno]

The fix for this bug seems to have caused bug 550539.

Comment 23

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/en/XUL/Attribute/noinitialfocus