Closed Bug 250609 Opened 18 years ago Closed 11 years ago

Need a way to create a <dialog> with no buttons

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: Stefan.Borggraefe, Assigned: iannbugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Currently there is no way to create a dialog without any buttons. This is needed
for dialogs like 

chrome://navigator/content/metadata.xul or
chrome://global/content/printPreviewProgress.xul

They can't be ported from the (deprecated) dialogOverlay.xul to the (new)
<dialog> element because of this.

I suggest adding "none" to the possible values of the buttons attribute.
Attached patch Patch (obsolete) — Splinter Review
Adds buttons="none" to dialog.xml.

This patch also directly includes the conversions of the Image/Link
Properties-dialog and the Print Preview-dialog to <dialog> using the new value
for the buttons attribute. I thought this would be nice to have as a test-case
for reviewing and I wanted to do this conversion in a later patch anyway.
Assignee: jag → Stefan.Borggraefe
Status: NEW → ASSIGNED
Keywords: helpwanted
Attachment #159630 - Flags: superreview?(neil.parkwaycc.co.uk)
(In reply to comment #1)
> Properties-dialog and the Print Preview-dialog to <dialog> using the new value
                                         ^-- Progress
Mhh, it's probably better when the Properties dialog doesn't react on pressing
the Enter key. I can add onbuttonaccept="return false;" to the attributes of the
<dialog> tag.
Comment on attachment 159630 [details] [diff] [review]
Patch

I think this button configuration code is getting too complicated - instead of
splitting up the list and remembering which ones were in the list, why not just
loop though the button element mapping and show or hide whichever ones are or
aren't in the configuration?

As for the Enter action, I think it needs to check to see if the accept button
is hidden.
Attachment #159630 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attached patch Patch V2 (obsolete) — Splinter Review
Addressed review comments. I think the code to show/hide the buttons is much
simpler now. :-) Furthermore this patch now also includes pageInfo.xul from
Firefox which is also a dialog with no buttons.

Tested with current Seamonkey and Firefox trunk builds.
Attachment #159630 - Attachment is obsolete: true
Attachment #160189 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 160189 [details] [diff] [review]
Patch V2

>+          // show all buttons listed in the button configuration
>+          var list = aButtons.split(",");
>+          for (item in list)
>+            buttons[list[item]].removeAttribute("hidden");
Actually this isn't what I meant either, I was thinking you could use for
(dlgtype in buttons) buttons[dlgtype].hidden = /*aButtons does not contain the
substring dlgtype*/; then it wouldn't matter if there were nonsense words in
aButtons.
(In reply to comment #6)
> (From update of attachment 160189 [details] [diff] [review])
> >+          // show all buttons listed in the button configuration
> >+          var list = aButtons.split(",");
> >+          for (item in list)
> >+            buttons[list[item]].removeAttribute("hidden");
> Actually this isn't what I meant either, I was thinking you could use for
> (dlgtype in buttons) buttons[dlgtype].hidden = /*aButtons does not contain the
> substring dlgtype*/; then it wouldn't matter if there were nonsense words in
> aButtons.

I consider a big fat error in the JavaScript console when there is a nonsense
word in aButtons a good thing. The current <dialog>-implementation also shows an
error in this case. Maybe a developer notices earlier he made a typo because of
such an error message.
Ok, then what about
<constructor>
<![CDATA
  if (this.hasAttribute(buttons))
    this._configureButtons(this.getAttribute(buttons));
  else
    this._configureButtons("accept,cancel");
(In reply to comment #8)
> Ok, then what about
> <constructor>
> <![CDATA
>   if (this.hasAttribute(buttons))
>     this._configureButtons(this.getAttribute(buttons));
>   else
>     this._configureButtons("accept,cancel");

Hmm, I already have a similar fallback in attachment 160189 [details] [diff] [review]:

+          if (!aButtons)
+            // default button configuration
+            aButtons = "accept,cancel";

I'm not sure I understand your comment correctly. Do you want me to move it to
the constructor? Why would this be better?
Attached patch Patch V2.1 (obsolete) — Splinter Review
After thinking a bit more about this, I believe it is just a little bit cleaner
to have this check in the constructor. Having this in the constructor makes the
comment

+	     // default button configuration

from Patch V2 obsolete, because it is now clear from the placement of the code.
I guess, this is what you meant, right?
Attachment #160189 - Attachment is obsolete: true
Attachment #160189 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #160201 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 160201 [details] [diff] [review]
Patch V2.1

This patch doesn't apply anymore. :-(
Attachment #160201 - Attachment is obsolete: true
Attachment #160201 - Flags: superreview?(neil.parkwaycc.co.uk)
Blocks: 226955
Attached patch Patch v2.2 (obsolete) — Splinter Review
Changes since v2.2
* Just the .xml changes (unbitrotted I hope).
* Made necessary adjustments to #ifdef MOZ_THUNDERBIRD line.
* Made necessary adjustments to #ifdef XP_WIN spacer line.

I count about 36 files that could be updated once this had landed.
Saying that not all 36 are buttonless windows with class="dialog"
Hmm... this looks as if this change will further break the .buttons setter.
Perhaps we should just get rid of the setter altogether?
Neil, I don't see much of a reason to support the setter.  I know that we use it
in Firefox, but there's other ways to skin that cat, so if its broken already,
lets just remove it instead of letting people rely on something flawed.
Comment on attachment 174249 [details] [diff] [review]
Patch v2.2

OK, so assuming we can remove the buttons property completely, then:

>-        <xul:button dlgtype="accept" class="dialog-button"
We won't need to change these

>-        this._configureButtons(this.getAttribute("buttons"));
This will become this._configureButtons()

>-          // ensure that hitting enter triggers ondialogaccept
>-          buttons["accept"].setAttribute("default", "true");
This shouldn't have moved anyway.

>-          // if there is a special button configuration, use it
>-          if (aButtons) {
This will become if (this.hasAttribute("buttons"))

>-            // expect a comma delimited list of dlgtype values
>-            var list = aButtons.split(",");
This will become this.getAttribute("buttons").split
Note: although specifying buttons="" doesn't produce this effect, specifying buttons="," does, in Gecko 1.9/Firefox 3 anyway.
Neil, is this what you meant?
I've tried metadata.xul and not put a buttons attribute in the <dialog> but still get OK and Cancel buttons, perhaps still need to hide them somehow?
Assignee: Stefan.Borggraefe → iann_bugzilla
Attachment #174249 - Attachment is obsolete: true
Attachment #519827 - Flags: review?(neil)
Comment on attachment 519827 [details] [diff] [review]
Removing the buttons patch v3.0

As far as I can tell my previous comments were related to the patch, while this seems to be a completely separate patch aimed at removing the ability to change the displayed buttons dynamically.

And it's been so long that I've forgotten what, if anything, we ever agreed to as how we should indicate that we don't want any buttons. In fact it seems that both <dialog buttons="none"> and <dialog buttons=","> already work.
Attachment #519827 - Flags: review?(neil)
(In reply to comment #19)
> Comment on attachment 519827 [details] [diff] [review]
> Removing the buttons patch v3.0
> 
> As far as I can tell my previous comments were related to the patch, while this
> seems to be a completely separate patch aimed at removing the ability to change
> the displayed buttons dynamically.
> 
> And it's been so long that I've forgotten what, if anything, we ever agreed to
> as how we should indicate that we don't want any buttons. In fact it seems that
> both <dialog buttons="none"> and <dialog buttons=","> already work.

Hmmm, indeed they do, never thought to try, so this bug should be closed then.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.