172691 - Prompt with checkbox and one, three or four buttons initally focuses wrong button

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[[e-mail]]

Using Build ID: 2002100317
Steps to reproduce problem:
Paste this as a single line into the JS console and evaluate it:
Components.classes[ "@mozilla.org/embedcomp/prompt-service;1" ].getService(
Components.interfaces.nsIPromptService ).confirmEx( top, "Preview document not
saved", "Do you want to save for a more accurate preview?", 0x40203, null, null,
null, "Remember this decision", {value: 0} )

Actual result: "No" is focused

Expected result: "Yes" is focused

Comment 1

[[e-mail]]

Attachment: Proposed patch

Only accept and cancel are candidates for the focus.

Comment 2

[Aaron Leventhal]

The initial focus should always be on the leftmost button.

Comment 3

[[e-mail]]

My point exactly, the Yes button should be focused, not the No button.

Comment 4

[Aaron Leventhal]

> Only accept and cancel are candidates for the focus.

Are one of those always the leftmost button?

Comment 5

[[e-mail]]

Using Common Dialog numbers:
Windows/Linux: button2 button3 button4 button1
Mac: button4 button3 button1 button2 (if it makes any difference)
Using dlgtype names:
Windows/Linux: accept extra1 extra2 cancel (help disclosure)
Mac: (disclosure help) extra2 extra1 cancel accept
My understanding is that the different order on the Mac shouldn't affect the
problem that the extra buttons should never claim focus.

Comment 6

[Aaron Leventhal]

I don't understand something, why should the extra buttons never claim focus?

I put brackets around the button I think should have initial focus.

Windows/Linux: [button2] button3 button4 button1
Mac: [button4] button3 button1 button2 (if it makes any difference)
Using dlgtype names:
Windows/Linux: [accept] extra1 extra2 cancel (help disclosure)
Mac: (disclosure help) [extra2] extra1 cancel accept

Comment 7

[[e-mail]]

Sorry to confuse you about claiming focus, I meant initially focus, my JS skills
are obviously better than my English skills :-/

So, to rephrase, prompts should initially focus the accept button (button 2),
except when there is a text field, or trivially where there is only one button.

> Windows/Linux: [button2] button3 button4 button1

I agree, but currently if it exists button3 (or 4) will initially focus.

Comment 8

[[e-mail]]

Attachment: Better patch

After rereading the code and examining DOM inspector I realized that the
"accept" button is always visibly the leftmost and therefore initially focused
button (except when textfields are present).

Unfortunately the existing code updates the label on the hidden "cancel" button
when only one button is visible, so I've fixed that problem too.

Comment 9

[[e-mail]]

Steps to reproduce single button problem:
Paste this as a single line into the JS console and evaluate it:
Components.classes[ "@mozilla.org/embedcomp/prompt-service;1" ].getService(
Components.interfaces.nsIPromptService ).confirmEx( top, "Single Button Test",
"This tests the dynamic label on a single button", 127, "&Done", null, null,
null, {value: 0} )

Comment 10

[:deanis74]

Comment on attachment 102389 [details] [diff] [review]
Better patch

>-  if (aLabel.charAt(0) == '&') {
>+  if (/^\&[^\&]/.test(aLabel)) {

I'm pretty poor at expression matching.  What does this do?

Can't the whole block of code for checking for checking for an accesskey be
replaced by:

  accessKeyIndex = aLabel.search(/[^\&]\&[^\&]/);

>     case 2:
>-      newButton = document.documentElement.getButton("accept");
>-      firstButton = firstButton || newButton;
>-      var string = gCommonDialogParam.GetString(8);
>+      string = gCommonDialogParam.GetString(9);

Will this cause a js strict warning?

>   // initialize the checkbox
>-  if (setCheckbox(gCommonDialogParam.GetString(1), gCommonDialogParam.GetInt(1)) &&
>-      gCommonDialogParam.GetInt(3) == 0)  // if no text fields
>+  setCheckbox(gCommonDialogParam.GetString(1), gCommonDialogParam.GetInt(1));

By taking setCheckbox out of the test, setCheckbox no longer needs to return a
value.

>+  if (gCommonDialogParam.GetInt(3) == 0)

Can you maintain the comment of what this means?

>     firstButton.focus(); // Don't focus checkbox, focus first button instead

I know the comment isn't yours, but is it wrong?  Should it say 'text box'
instead of 'checkbox'?	Or am I just reading the code wrong?  According to the
comments, it checks for whether there are text fields, but then sets focus on
the check box or button.

This seems to be pretty much the same thing I was doing in attachment 103700 [details] [diff] [review]
for bug 175683, with a few extras.  If you can clear up my questions, it looks
good to me.

(/me breathes a huge sigh of relief as he realizes that Redo Edit As Comment
saves the text I'd already typed!)

Comment 11

[[e-mail]]

Attachment: Addressed Dean's comments (!)

Dean Tessman wrote:
>
>(From update of attachment 102389 [details] [diff] [review])
>>-  if (aLabel.charAt(0) == '&') {
>>+  if (/^\&[^\&]/.test(aLabel)) {
>
>I'm pretty poor at expression matching.  What does this do?

^ - match at beginning
\& - match literal symbol (\ possibly not needed for &)
[^\&] - match anything except & (yes, different meaning of ^)

So, it matches a & at the beginning, but not && at the beginning.

>Can't the whole block of code for checking for checking for an accesskey be
>replaced by:
>
>  accessKeyIndex = aLabel.search(/[^\&]\&[^\&]/);

Unfortunately that won't match &No (for instance); while /(^|[^\&])\&[^\&]/
would match, it would return 0 for both &No and N&o.

>>     case 2:
>>-	 newButton = document.documentElement.getButton("accept");
>>-	 firstButton = firstButton || newButton;
>>-	 var string = gCommonDialogParam.GetString(8);
>>+	 string = gCommonDialogParam.GetString(9);
>
>Will this cause a js strict warning?

My fault, I put the "var" in the wrong place (it's in case 1:-)

>>   // initialize the checkbox
>>-  if (setCheckbox(gCommonDialogParam.GetString(1),
>gCommonDialogParam.GetInt(1)) &&
>>-	 gCommonDialogParam.GetInt(3) == 0)  // if no text fields
>>+  setCheckbox(gCommonDialogParam.GetString(1),
gCommonDialogParam.GetInt(1));
>
>By taking setCheckbox out of the test, setCheckbox no longer needs to return a

>value.

OK, I'll remove that return value.

>>+  if (gCommonDialogParam.GetInt(3) == 0)
>
>Can you maintain the comment of what this means?
>
>>     firstButton.focus(); // Don't focus checkbox, focus first button instead

>
>I know the comment isn't yours, but is it wrong?  Should it say 'text box'
>instead of 'checkbox'? Or am I just reading the code wrong?  According to the
>comments, it checks for whether there are text fields, but then sets focus on
>the check box or button.

No, because if (gCommonDialogParam.GetInt(3) == 0) then there are no text boxes
to focus, but left to itself the dialog will focus the checkbox. Restoring the
comment on the previous line should help here.

Comment 12

[:deanis74]

Comment on attachment 104791 [details] [diff] [review]
Addressed Dean's comments (!)

> >Can't the whole block of code for checking for checking for an accesskey be
> >replaced by:
> >
> >  accessKeyIndex = aLabel.search(/[^\&]\&[^\&]/);

> Unfortunately that won't match &No (for instance); while /(^|[^\&])\&[^\&]/
> would match, it would return 0 for both &No and N&o.

Oh .search isn't 0-based?  That sucks.	I thought that it would return 0 if it
matched the first character, and -1 if no match at all.

Given that, everything else looks good.  r=me

Comment 13

[[e-mail]]

Dean Tessman wrote:

> Oh .search isn't 0-based?

It is, but it won't tell you which alternative matched, which is why the
separate test is necessary.

Comment 14

[:deanis74]

Huh?  If it was 0-based, it would return 0 for &No and 1 for N&o.

Comment 15

[[e-mail]]

Dean, the problem is that search doesn't return the index of the & that we want.
Normally search returns the index of the previous character, but that's not very
useful when there is no previous character...

Comment 16

[[e-mail]]

Changing summary to reflect problem with security warnings (bug 175683).

Comment 17

[[e-mail]]

bz, if you meant to give this sr=, then can you check it in as well?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Um, sure.  But so far I've just marked this "take the time to figure out why the
changes to the arguments being passed to GetString() were made"...  Since I've
not that familiar with the code in question, this will probably take me a few
hours in a block that I need to find somewhere...

Comment 19

[Boris Zbarsky [:bzbarsky]]

-    var text = document.createTextNode(messageParagraphs[i]);
-    descriptionNode.appendChild(text);
     messageParent.appendChild(descriptionNode);
+    descriptionNode.splitText(messageParagraphs[i]);

What's with this change?

Other than that, looks good.

Comment 20

[[e-mail]]

Comment on attachment 104791 [details] [diff] [review]
Addressed Dean's comments (!)

bz: good catch, that's from another patch and doesn't belong here.

Comment 21

[[e-mail]]

Attachment: Removed extraneous change

Comment 22

[:deanis74]

Comment on attachment 106081 [details] [diff] [review]
Removed extraneous change

r=me

Comment 23

[[e-mail]]

*** Bug 180072 has been marked as a duplicate of this bug. ***

Comment 24

[Adam Becevello]

*** Bug 180037 has been marked as a duplicate of this bug. ***

Comment 25

[Jun MUTO]

*** Bug 173246 has been marked as a duplicate of this bug. ***

Comment 26

[[e-mail]]

Fix was checked in by timeless.

Comment 27

[Christian Schweer]

I still see this bug in the 2002-11-21 build of the 1.2 trunk (WinNT4 SP6).

Will the fix make it to the final 1.2 release??

Comment 28

[Boris Zbarsky [:bzbarsky]]

There is no 1.2 trunk, there is only a 1.2 branch.  And no, this fix will not
make it on the 1.2 branch.  It was fixed about 2-3 weeks too late for that.

Comment 29

[[e-mail]]

Note that you can still manually apply this fix to 1.2 using e.g. PatchMaker.

Comment 30

[[e-mail]]

*** Bug 183264 has been marked as a duplicate of this bug. ***

Comment 31

[u60234]

*** Bug 182714 has been marked as a duplicate of this bug. ***

Comment 32

[Boris Zbarsky [:bzbarsky]]

*** Bug 183916 has been marked as a duplicate of this bug. ***

Comment 33

[sairuh (rarely reading bugmail)]

vrfy'd fixed (tested with secure page dlg cited in dups) with 2002.12.10.07
mach-o mozilla (OS X 10.2.2), 2002.12.09.08 comm trunk on linux rh8, and
2002.12.10.08 comm trunk on win2k.

Comment 34

[Kevin Chen]

*** Bug 186823 has been marked as a duplicate of this bug. ***

Comment 35

[Boris Zbarsky [:bzbarsky]]

*** Bug 186959 has been marked as a duplicate of this bug. ***

Comment 36

[Boris Zbarsky [:bzbarsky]]

*** Bug 190198 has been marked as a duplicate of this bug. ***

Comment 37

[Boris Zbarsky [:bzbarsky]]

*** Bug 193518 has been marked as a duplicate of this bug. ***

Comment 38

[KIRAN SHIVARAMA SUNDAR]

We have found out that the patch (attachment 106081 [details] [diff] [review] of bug172691) has resulted
in bug243920.
Making the following two changes in the patch (attachment 106081 [details] [diff] [review] of bug172691)
fixes the problem.

+ var firstButton = null;
- var firstButton = document.documentElement.getButton("accept");
  switch (nButtons) {
     case 4:

We want this to be reviewed by someone and also want neil to check the code
change. We have also verified that the code change does not affect the given
fix. Kindly have someone review this fix above.

Comment 39

[KIRAN SHIVARAMA SUNDAR]

Neil, We would like to know what problems could changing the code to "var
firstButton = null;" give rise to.