322155 - if the only focusable elements in a dialog are the buttons, focus the default button on load

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement, P2)

Description

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

Many users of the dialog binding want the default button focused, mostly to ensure that on Windows/Linux, hitting enter after the dialog appears triggers the button. Currently this requires a .focus() onLoad, since the dialog currently focuses the first focusable element, which for many cases is another button. I propose changing the behavior so that dialogs whose only focusable elements are the buttons default to focusing the default button. This would eliminate the need for onload "getButton(XX).focus()" calls in many dialogs (see URL for some examples).

Toolkit patch coming, I'll patch xpfe too if there's agreement that this is desired.

Comment 1

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

Attachment: patch

Comment 2

[[e-mail]]

Shouldn't the URL just search for the "accept" button? When I do that, there are only nine entries:
 /calendar/sunbird/base/content/aboutDialog.js
 /toolkit/mozapps/downloads/content/downloadProperties.js
 /browser/base/content/aboutDialog.js
 /browser/base/content/pageReport.js
In these cases a non-button appears to be taking the focus.
 /xpinstall/res/content/xpistatus.js
The focus is set after XPI install completes.
 /security/manager/pki/resources/content/newserver.js
This looks like a badly designed dialog, and the View Certificate non-dialog button should be put on extra2 instead, which would resolve the issue.
 /security/manager/pki/resources/content/serverCrlNextupdate.js
I guess this one depends on the platform - it probably works fine on Windows.
 /security/manager/pki/resources/content/serverCertExpired.js
 /security/manager/pki/resources/content/domainMismatch.js
OK, so these two appear to be valid on all platforms.

Comment 3

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

Neil and I discussed this on IRC, I mentioned that there are other cases that aren't caught by that regexp. Neil mentioned that maybe having an attribute to indicate the button to focus would be useful, but then I can't think of a situation where that button would not be the defaultButton. Bug 213207 is an example of a bug that would be easier to fix with this change.

Comment 4

[[e-mail]]

Comment on attachment 207376 [details] [diff] [review]
patch

>-          var focusInit = 
>-            function() {
>+          function dialog_postLoadInit(aDialog) {
No need to rename this, plus aDialog is unused, so the above becomes "function focusInit() {" and the change below is reverted:
>-          setTimeout(focusInit, 0);
>+          setTimeout(dialog_postLoadInit, 0, 0);

Comment 5

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

Attachment: updated patch

Comment 6

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

mozilla/toolkit/content/widgets/dialog.xml 	1.28
mozilla/xpfe/global/resources/content/bindings/dialog.xml 	1.38

Comment 7

[Håkan Waara]

Gavin, wanna land this on branch?   Thanks.

Comment 8

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

(In reply to comment #7)
> Gavin, wanna land this on branch?   Thanks.

The branch is still closed for Firefox alpha 2. I will land this once it opens, which will probably be later today.

Comment 9

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

mozilla/xpfe/global/resources/content/bindings/dialog.xml 	1.34.4.3
mozilla/toolkit/content/widgets/dialog.xml 	1.25.2.7