Last Comment Bug 284776 - Need support in dialog.xml for setting a default button
: Need support in dialog.xml for setting a default button
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: unspecified
: All All
: P1 enhancement with 1 vote (vote)
: mozilla1.8beta3
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
: Neil Deakin
Mentors:
Depends on:
Blocks: 286197 304879
  Show dependency treegraph
 
Reported: 2005-03-04 08:19 PST by Mike Connor [:mconnor]
Modified: 2006-11-28 12:56 PST (History)
7 users (show)
benjamin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (4.38 KB, patch)
2005-03-25 14:40 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
v2 (5.01 KB, patch)
2005-03-30 01:05 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
v2 (xpfe) (5.04 KB, patch)
2005-03-30 12:21 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
v2.1 (10.06 KB, patch)
2005-04-10 21:30 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
v3 (toolkit) (3.93 KB, patch)
2005-06-07 05:43 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: first‑review+
Details | Diff | Splinter Review
v3.1 (xpfe) (2.96 KB, patch)
2005-06-11 01:28 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
neil: second‑review+
benjamin: approval1.8b3+
Details | Diff | Splinter Review
[checked in] v3.1 (toolkit) (4.05 KB, patch)
2005-06-11 01:29 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: first‑review+
asa: approval‑aviary1.1a2+
Details | Diff | Splinter Review

Description Mike Connor [:mconnor] 2005-03-04 08:19:39 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-03-22 23:55:25 PST
Taking
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-03-25 14:40:26 PST
Created attachment 178624 [details] [diff] [review]
patch v1
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-03-25 15:15:11 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-03-25 15:34:05 PST
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-03-27 02:42:30 PST
Comment on attachment 178624 [details] [diff] [review]
patch v1

needs update
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-03-30 01:05:05 PST
Created attachment 179033 [details] [diff] [review]
v2
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-03-30 12:21:43 PST
Created attachment 179093 [details] [diff] [review]
v2 (xpfe)
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-04-10 21:20:36 PDT
+              if(!this.getButton("accept") || this.getButton("accept").hidden)
+                aNewDefault = "accept";
+              else
+                aNewDefault = "none";

in the opposite way of course... thanks Gavin.
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-04-10 21:30:02 PDT
Created attachment 180348 [details] [diff] [review]
v2.1

fix oops
Comment 10 Mike Connor [:mconnor] 2005-04-21 03:22:06 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-04-21 04:23:59 PDT
Comment on attachment 180348 [details] [diff] [review]
v2.1

(I'll reseparate the xpfe part)
Comment 12 Asa Dotzler [:asa] 2005-04-21 13:35:25 PDT
Comment on attachment 180348 [details] [diff] [review]
v2.1

a=asa
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-04-21 15:01:25 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-04-21 15:02:29 PDT
Comment on attachment 180348 [details] [diff] [review]
v2.1

->obsolete until i get this working again...
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-05-02 23:21:39 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-05-02 23:32:20 PDT
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] 2005-05-05 06:11:55 PDT
defaulting to hidden buttons seems like a bad plan, period.

I think the getComputedStyle is fine here, its not frequent or repeating.
Comment 18 neil@parkwaycc.co.uk 2005-05-05 08:52:23 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-07 05:41:46 PDT
Since accesskeys do fire hidden buttons, I'm not going to handle this case here.
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-07 05:43:02 PDT
Created attachment 185557 [details] [diff] [review]
v3 (toolkit)

Neil, Mike, what do you think?
Comment 21 Mike Connor [:mconnor] 2005-06-07 23:31:45 PDT
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 neil@parkwaycc.co.uk 2005-06-08 03:07:17 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-08 03:32:40 PDT
(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 neil@parkwaycc.co.uk 2005-06-08 04:00:35 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-11 01:28:16 PDT
Created attachment 185927 [details] [diff] [review]
v3.1 (xpfe)
Comment 26 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-11 01:29:12 PDT
Created attachment 185928 [details] [diff] [review]
[checked in] v3.1 (toolkit)
Comment 27 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-18 03:22:04 PDT
Comment on attachment 185928 [details] [diff] [review]
[checked in] v3.1 (toolkit)

checked this in, leaving open for xpfe sr and landing...
Comment 28 neil@parkwaycc.co.uk 2005-06-24 06:46:47 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-06-29 13:06:30 PDT
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

Note You need to log in before you can comment on or make changes to this bug.