Button in alert() dialog no longer gets initial focus

RESOLVED FIXED

Status

()

Core
XUL
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: Neil Deakin)

Tracking

({dev-doc-complete})

Trunk
x86
Mac OS X
dev-doc-complete
Points:
---
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

7.92 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
Flags: blocking1.9.2?
(Assignee)

Comment 1

8 years ago
Created attachment 397667 [details] [diff] [review]
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?
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Maybe people from bug 335089 would have an idea.

Comment 3

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

Comment 4

8 years ago
(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.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
(Assignee)

Comment 5

8 years ago
Created attachment 402418 [details] [diff] [review]
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?
Attachment #402418 - Flags: review?(neil)

Comment 6

8 years ago
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?
Attachment #402418 - Flags: review?(neil) → review-

Comment 7

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

Comment 8

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

Comment 9

8 years ago
Created attachment 403337 [details] [diff] [review]
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.
Attachment #397667 - Attachment is obsolete: true
Attachment #402418 - Attachment is obsolete: true
Attachment #403337 - Flags: review?(neil)
(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 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"
Attachment #403337 - Flags: review?(neil) → review+
Whiteboard: [needs landing]
Whiteboard: [needs landing]
Is this ready to land?
(Assignee)

Comment 13

8 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [doc-waiting-1.9.3]
(Assignee)

Updated

8 years ago
status1.9.2: --- → beta1-fixed
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

8 years ago
Bug is still fixed on trunk though.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
status1.9.2: beta1-fixed → ---
Resolution: --- → FIXED
Indeed, sorry about that.
Can this land on mozilla-1.9.2? It blocks the beta ...
(Assignee)

Comment 18

8 years ago
Woops, I checked it earlier today, with the test disabled on Mac.
status1.9.2: --- → beta1-fixed
(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
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.
Whiteboard: [doc-waiting-1.9.3]
(Assignee)

Comment 21

8 years ago
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

8 years ago
The fix for this bug seems to have caused bug 550539.
Documented here:

https://developer.mozilla.org/en/XUL/Attribute/noinitialfocus
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.