The default bug view has changed. See this FAQ.

Need support in dialog.xml for setting a default button

RESOLVED FIXED in mozilla1.8beta3

Status

()

Toolkit
XUL Widgets
P1
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: mconnor, Assigned: mano)

Tracking

unspecified
mozilla1.8beta3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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
Created attachment 178624 [details] [diff] [review]
patch v1
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)
Created attachment 179033 [details] [diff] [review]
v2
Attachment #179033 - Flags: review?(mconnor)
Created attachment 179093 [details] [diff] [review]
v2 (xpfe)
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.
Created attachment 180348 [details] [diff] [review]
v2.1

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 #179033 - Flags: review?(mconnor)
Attachment #179093 - Flags: superreview?(neil.parkwaycc.co.uk)
(Reporter)

Comment 10

12 years ago
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 12

12 years ago
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.
(Reporter)

Comment 17

12 years ago
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 18

12 years ago
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
Created attachment 185557 [details] [diff] [review]
v3 (toolkit)

Neil, Mike, what do you think?
Attachment #185557 - Flags: first-review?(mconnor)
(Reporter)

Comment 21

12 years ago
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 22

12 years ago
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.

Comment 24

12 years ago
(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.
Created attachment 185927 [details] [diff] [review]
v3.1 (xpfe)
Attachment #185927 - Flags: second-review?(neil.parkwaycc.co.uk)
Created attachment 185928 [details] [diff] [review]
[checked in] v3.1 (toolkit)
Attachment #185557 - Attachment is obsolete: true
Attachment #185928 - Flags: first-review+
Attachment #185928 - Flags: approval-aviary1.1a2?

Updated

12 years ago
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 28

12 years ago
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?
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 304879
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.