284776 - Need support in dialog.xml for setting a default button

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement, P1)

Description

[Mike Connor [:mconnor]]

Add a new attribute to the dialog widget, defaultButton, which if set makes that
button the default in that dialog.  This should also support "none" in which
case no button should be set to default.  If not present, default to "accept" as
is currently the case.

This parallels changes to nsIPromptService to allow specifying a default.

Comment 1

[Asaf Romano (gone)]

Taking

Comment 2

[Asaf Romano (gone)]

Attachment: patch v1

Comment 3

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

Comment on attachment 178624 [details] [diff] [review]
patch v1

>Index: toolkit/content/widgets/dialog.xml

>+        <setter>
>+        <![CDATA[
>+           this._setDefaultButton(val);
>+        ]]>
>+        </setter>

Setters this small usually use "onset", no? Like "buttons" right above it.
(if you leave it, fix indent)

>+             if (aNewDefault != "none") // set the new default button
>+                this.getButton(aNewDefault).setAttribute("default", "true");

nit: fix indent

>+             dump("setDefaultButton: " + aNewDefault + "no such button; assuming 'Accept'\n");

nit: space before "no such button"
maybe something like: "setDefaultButton: Button "foo" does not exist, using
'Accept'"
(not that it's all that important)

Also, do you plan on doing the xpfe one as well?

Comment 4

[Asaf Romano (gone)]

(In reply to comment #3)
> (From update of attachment 178624 [details] [diff] [review] [edit])
> >Index: toolkit/content/widgets/dialog.xml
> 
> >+        <setter>
> >+        <![CDATA[
> >+           this._setDefaultButton(val);
> >+        ]]>
> >+        </setter>
> 
> Setters this small usually use "onset", no? Like "buttons" right above it.

Not when the getter is in the other syntax.


> Also, do you plan on doing the xpfe one as well?

Probably.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 178624 [details] [diff] [review]
patch v1

needs update

Comment 6

[Asaf Romano (gone)]

Attachment: v2

Comment 7

[Asaf Romano (gone)]

Attachment: v2 (xpfe)

Comment 8

[Asaf Romano (gone)]

+              if(!this.getButton("accept") || this.getButton("accept").hidden)
+                aNewDefault = "accept";
+              else
+                aNewDefault = "none";

in the opposite way of course... thanks Gavin.

Comment 9

[Asaf Romano (gone)]

Attachment: v2.1

fix oops

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 180348 [details] [diff] [review]
v2.1


>+          else // default to the accept button 
>+            if (!this.getButton("accept").hidden)
>+              rv = "accept";
>+            else  // but if it's hidden, default to none.
>+              rv = "none";

nit, use {} here just for code readability, it works without it, but this is
kinda inconsistent with other places in toolkit.  Also, move the comment down a
line, since the comment seems to refer to the action in the else statment, but
that's not quite right.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 180348 [details] [diff] [review]
v2.1

(I'll reseparate the xpfe part)

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 180348 [details] [diff] [review]
v2.1

a=asa

Comment 13

[Asaf Romano (gone)]

Neil, has anything changed in the way the "hidden" property works since i posted
this patch? buttons which are very hidden, return "false" as their hidden property.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 180348 [details] [diff] [review]
v2.1

->obsolete until i get this working again...

Comment 15

[Asaf Romano (gone)]

I was wrong, the reason it hasn't worked for me was that the buttons box was
hidden, not the button itself... which means i'm not handling some cases.

Comment 16

[Asaf Romano (gone)]

I'm not sure what would be the Right Way to handle it:
  1. Do allow defaultng to hidden buttons
  2. Catch those cases by checking the computed display style property.

Comment 17

[Mike Connor [:mconnor]]

defaulting to hidden buttons seems like a bad plan, period.

I think the getComputedStyle is fine here, its not frequent or repeating.

Comment 18

[[e-mail]]

Comment on attachment 179093 [details] [diff] [review]
v2 (xpfe)

>+      <property name="defaultButton">
>+        <getter>
>+        <![CDATA[
>+          if (this.hasAttribute("defaultButton")) {
>+            var rv = this.getAttribute("defaultButton");
>+            // Check (again) for the hidden property, in case
>+            // it was changed after the default button has been set
>+            if (rv != "none" && this.getButton(rv).hidden)
>+              rv = "none";
>+          }
>+          else // default to the accept button 
>+            if (!this.getButton("accept").hidden)
>+              rv = "accept";
>+            else  // but if it's hidden, default to none.
>+              rv = "none";
>+
>+          return rv;
>+        ]]>
>+        </getter>
I don't see the point of checking for hiddenness here. In fact, I'm not
completely convinced that you should return "accept" if the attribute is not
set, but it makes my rewrite of the setter nicer ;-)

>+        <setter>
>+        <![CDATA[
>+          this._setDefaultButton(val);
>+        ]]>
>+        </setter>
You forgot to return val;

>+          // ensure that hitting enter triggers the default button command
>+          this._setDefaultButton(this.defaultButton);
I'd write this.defaultButton = this.defaultButton; here - after all, we've
commented why the line is being added... you then get to inline
_setDefaultButton() into set defaultButton().

>+          var oldDefaultButtonTitle = this.defaultButton;
>+          if (oldDefaultButtonTitle != "none") {
>+            var oldDefaultButton = this.getButton(oldDefaultButtonTitle);
>+            if (oldDefaultButton && oldDefaultButton.hasAttribute("default")) 
>+              oldDefaultButton.removeAttribute("default");
>+          }
Note that this.getButton will return undefined unless you pass it one of the
six known buttons, so there's no pressing need to special-case "none", as any
random attribute value will have the same effect.

>+            if (!this.getButton(aNewDefault) || this.getButton(aNewDefault).hidden) {
>+              dump("_setDefaultButton: invalid new default button: '" + aNewDefault + "', assuming: ");
>+
>+              if(!this.getButton("accept") || this.getButton("accept").hidden)
>+                aNewDefault = "accept";
>+              else
>+                aNewDefault = "none";
Again, I don't see the point of checking for hidden here - in fact, I don't
even think your test is correct.

>+          var btn = this.getButton(this.defaultButton);
>+          if (btn)
>+            this._doButtonCommand(this.defaultButton);
I think this is the best place, if you really want to check for hidden. But I
think that anyone wanting enter not to fire a button should just set the
default to null.

Comment 19

[Asaf Romano (gone)]

Since accesskeys do fire hidden buttons, I'm not going to handle this case here.

Comment 20

[Asaf Romano (gone)]

Attachment: v3 (toolkit)

Neil, Mike, what do you think?

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 185557 [details] [diff] [review]
v3 (toolkit)

>           // ensure that hitting enter triggers ondialogaccept
>-          buttons["accept"].setAttribute("default", "true");
>+          this.defaultButton = this.defaultButton;

The comment is wrong, you had this fixed in an earlier patch

>+      <method name="_setDefaultButton">
>+        <parameter name="aNewDefault"/>
>+        <body>
>+        <![CDATA[
>+          // remove the default attribute from the previous default button, if any
>+          var oldDefaultButton = this.getButton(this.defaultButton);
>+          if (oldDefaultButton)
>+              oldDefaultButton.removeAttribute("default");

nit: wacky indentation for no reason

>+          var newDefaultButton = this.getButton(aNewDefault);
>+          if (newDefaultButton) {
>+            this.setAttribute("defaultButton", aNewDefault);
>+            newDefaultButton.setAttribute("default", "true");
>+          }
>+          else {
>+            if (aNewDefault == "none")
>+              this.setAttribute("defaultButton", aNewDefault);
>+            else
>+              throw "either an exist button or no button should be picked";
>+          }

nit: the nested else is ugly, convention is to do the following instead, I
should have pointed this out the first time I reviewed this!  Also, I think you
mean existing...

	 var newDefaultButton = this.getButton(aNewDefault);
	 if (newDefaultButton) {
	   this.setAttribute("defaultButton", aNewDefault);
	   newDefaultButton.setAttribute("default", "true");
	 }
	 else if (aNewDefault == "none")
	   this.setAttribute("defaultButton", aNewDefault);
	 else
	   throw "either an exist button or no button should be picked";

Comment 22

[[e-mail]]

Comment on attachment 185557 [details] [diff] [review]
v3 (toolkit)

>+          if (this.hasAttribute("defaultButton"))
>+            return this.getAttribute("defaultButton");
>+          else // default to the accept button
>+            return "accept";
Possibly could be // default to the accept button
return this.getAttribute("defaultButton") || "accept";

>+              throw "either an exist button or no button should be picked";
I don't like this. It will mean that <dialog defaultButton="invalid"> will not
get constructed correctly.

Comment 23

[Asaf Romano (gone)]

(In reply to comment #22)

> >+              throw "either an exist button or no button should be picked";
> I don't like this. It will mean that <dialog defaultButton="invalid"> will not
> get constructed correctly.

Should it be? The list of possible dialog buttons is static, if an invalid
button has been picked, it should be well notified IMHO.

Comment 24

[[e-mail]]

(In reply to comment #22)
>(From update of attachment 185557 [details] [diff] [review] [edit])
>>+          if (this.hasAttribute("defaultButton"))
>>+            return this.getAttribute("defaultButton");
>>+          else // default to the accept button
>>+            return "accept";
>Possibly could be // default to the accept button
>return this.getAttribute("defaultButton") || "accept";
Forget that, I've decided I'd prefer <dialog defaultButton=""> for no default.

Comment 25

[Asaf Romano (gone)]

Attachment: v3.1 (xpfe)

Comment 26

[Asaf Romano (gone)]

Attachment: [checked in] v3.1 (toolkit)

Comment 27

[Asaf Romano (gone)]

Comment on attachment 185928 [details] [diff] [review]
[checked in] v3.1 (toolkit)

checked this in, leaving open for xpfe sr and landing...

Comment 28

[[e-mail]]

Comment on attachment 185927 [details] [diff] [review]
v3.1 (xpfe)

>+          var newDefaultButton = this.getButton(aNewDefault);
>+          if (newDefaultButton) {
>+            this.setAttribute("defaultButton", aNewDefault);
>+            newDefaultButton.setAttribute("default", "true");
>+          }
>+          else {
>+            this.setAttribute("defaultButton", "none");
>+            if (aNewDefault != "none")
>+              dump("invalid new default button: " +  aNewDefault + ", assuming: none\n");
>+          }
I don't really like the dump there either. Please remove that, and I'd also
prefer if you always set the attribute to aNewDefault before checking this in.

Comment 29

[Asaf Romano (gone)]

Checking in dialog.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/dialog.xml,v  <-- 
dialog.xml
new revision: 1.34; previous revision: 1.33
done