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.

Taking

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

+ if(!this.getButton("accept") || this.getButton("accept").hidden) + aNewDefault = "accept"; + else + aNewDefault = "none"; in the opposite way of course... thanks Gavin.

fix oops

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 on attachment 180348 [details] [diff] [review] v2.1 (I'll reseparate the xpfe part)

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

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...

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.

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.

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

Neil, Mike, what do you think?

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 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.

Comment on attachment 185928 [details] [diff] [review] [checked in] v3.1 (toolkit) checked this in, leaving open for xpfe sr and landing...

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.

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