112999 - Built-in "Accept" button closes dialog even when not used in dialog because "default" attribute is still true

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Charles Manske]

In converting the Composer's Spell checking dialog to use the new <dialog> tag,
we set our own buttons using:
  <dialog buttons="cancel"...
In this case, the built-in 'accept' button is hidden, and the Spellcheck code 
manages which button has the "default" attribute and whether or not the dialog
is closed when the default button is fired using Return or Enter key.
This doesn't work now because the anonymous "accept" button fires and causes the
dialog to close.
Test: 
1. Start Composer and type a bunch of garbage words.
2. Click on the spellcheck button. 
3. You should see either the "Ignore" or "Replace" buttons set as the default 
button (thicker border). Press Enter/Return key.
4. The dialog should find the next misspelled word and stay open. This should 
be true for all successive presses of Enter/Return until all words are checked
and the "Close" button is the default button when spell checking is completed.

The early terminitation behavior will only be seen when the changes in 
bug 104113 are checked in (conversion of the Spelling dialog to use "<dialog>").

Comment 1

[Charles Manske]

Attachment: Fix: Remove "default" attribute from built-in buttons that are not used

Comment 2

[Joe Hewitt (gone)]

Attachment: try this one

I do try to support dynamic changing of the buttons property, so we need to be
able to add default back if we remove it.

Comment 3

[Charles Manske]

Comment on attachment 59980 [details] [diff] [review]
Fix: Remove "default" attribute from built-in buttons that are not used

The newer patch works fine.

Comment 4

[Joe Hewitt (gone)]

fixed

Comment 5

[jag (Peter Annema)]

@@ -158,8 +158,13 @@
             if (anonBtn) {
               if (this._useAnonButton[dlgtype] && shown[dlgtype])
                 anonBtn.removeAttribute("hidden");
-              else 
+                if (dlgtype == "accept")
+                  anonBtn.setAttribute("default", "true");
+              else {
                 anonBtn.setAttribute("hidden", "true");
+                if (dlgtype == "accept")
+                  anonBtn.removeAttribute("default");
+              }
             }

I was going to comment on either incorrect indentation or lack of curly braces
for the outer if case, but the curly braces were added before this was checked
in, so now this is just a "for the record" :-)