Closed Bug 284776 Opened 15 years ago Closed 15 years ago

Need support in dialog.xml for setting a default button

Categories

(Toolkit :: XUL Widgets, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.8beta3

People

(Reporter: mconnor, Assigned: mano)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Status: NEW → ASSIGNED
Assignee: gavin.sharp → jag
Status: ASSIGNED → NEW
Component: General → XP Toolkit/Widgets
Priority: -- → P1
Product: Firefox → Core
QA Contact: general → jrgmorrison
Version: unspecified → Trunk
Taking
Assignee: jag → bugs.mano
Target Milestone: --- → mozilla1.8beta2
Blocks: 286197
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #178624 - Flags: review?(mconnor)
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?
(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 on attachment 178624 [details] [diff] [review]
patch v1

needs update
Attachment #178624 - Attachment is obsolete: true
Attachment #178624 - Flags: review?(mconnor)
Attached patch v2 (obsolete) — Splinter Review
Attachment #179033 - Flags: review?(mconnor)
Attached patch v2 (xpfe) (obsolete) — Splinter Review
Attachment #179093 - Flags: superreview?(neil.parkwaycc.co.uk)
+              if(!this.getButton("accept") || this.getButton("accept").hidden)
+                aNewDefault = "accept";
+              else
+                aNewDefault = "none";

in the opposite way of course... thanks Gavin.
Attached patch v2.1 (obsolete) — Splinter Review
fix oops
Attachment #179033 - Attachment is obsolete: true
Attachment #179093 - Attachment is obsolete: true
Attachment #180348 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #180348 - Flags: review?(mconnor)
Attachment #179093 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #180348 - Flags: review?(mconnor) → review+
Comment on attachment 180348 [details] [diff] [review]
v2.1

(I'll reseparate the xpfe part)
Attachment #180348 - Flags: superreview?(neil.parkwaycc.co.uk) → approval-aviary1.1a?
Comment on attachment 180348 [details] [diff] [review]
v2.1

a=asa
Attachment #180348 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
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 on attachment 180348 [details] [diff] [review]
v2.1

->obsolete until i get this working again...
Attachment #180348 - Attachment is obsolete: true
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.
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.
defaulting to hidden buttons seems like a bad plan, period.

I think the getComputedStyle is fine here, its not frequent or repeating.
Attachment #180348 - Flags: review-
Attachment #180348 - Flags: review+
Attachment #180348 - Flags: approval-aviary1.1a+
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.
Target Milestone: mozilla1.8beta2 → mozilla1.8beta3
Since accesskeys do fire hidden buttons, I'm not going to handle this case here.
Component: XP Toolkit/Widgets → XUL Widgets
Flags: review-
Product: Core → Toolkit
QA Contact: jrgmorrison → xul.widgets
Version: Trunk → unspecified
Attached patch v3 (toolkit) (obsolete) — Splinter Review
Neil, Mike, what do you think?
Attachment #185557 - Flags: first-review?(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";
Attachment #185557 - Flags: first-review?(mconnor) → first-review+
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.
(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.
(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.
Attached patch v3.1 (xpfe)Splinter Review
Attachment #185927 - Flags: second-review?(neil.parkwaycc.co.uk)
Attachment #185928 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Comment on attachment 185928 [details] [diff] [review]
[checked in] v3.1 (toolkit)

checked this in, leaving open for xpfe sr and landing...
Attachment #185928 - Attachment description: v3.1 (toolkit) → [checked in] v3.1 (toolkit)
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.
Attachment #185927 - Flags: second-review?(neil.parkwaycc.co.uk) → second-review+
Attachment #185927 - Flags: approval1.8b3? → approval1.8b3+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.