507622 - remove some usage of obsolete dialogOverlay.xul in mailnews/

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

dialogOverlay.xul is obsolete in favor of dialog.xml

Convert the few instances of using <window> as dialog. The upside is that a ok/cancel buttons get ordered correctly in a few places.

The selectAddressesWindow was pretty unhappy as a dialog. Dunno why, but the dialog is blank if that includes both bundle_composeMsgs and MsgComposeCommands.js. Fortunately, don't need the latter at all. Which means we don't need the former either!

abSelectAddressesDialog.xul doesn't belong in mailnews/ but i'll leave moving it to suite to another bug.

Comment 1

[[e-mail]]

Comment on attachment 391862 [details] [diff] [review]
proposed fix

>+  var abPopup = document.getElementById("abPopup");
Oh dear, how long has that been missing? ;-)

>+  if (abPopup) {
But I'm fairly sure it exists, given that the abList exists...

>-function OnUnloadSelectAddress()
>-{
>-  CloseAbView();
>-}
This is wrong, we need to close the view whether we OK or cancel the dialog.

>+  removeButton.disabled = !bucketTree.view ||
How can that happen? It's unusual for a tree not to have a view...

> 
>+  <vbox flex="1">
> 
>-  <keyset id="dialogKeys"/>
>-
>-  <vbox flex="1">
>-  
An hg diff oddity?

>-     <!--<splitter id="bucket-splitter" collapse="before" persist="state"/>-->
For a second I thought I was seeing double ;-)

>   <!-- we need to override the dialogs default key behavior so we can re-route enter/return
>        to the add button if the add button is suddenly the default action
>   -->
>   <keyset id="customHeaderDialogKeys">
>     <key keycode="VK_ENTER" oncommand="enterKeyPressed();"/>
>     <key keycode="VK_RETURN" oncommand="enterKeyPressed();"/>
>     <key keycode="VK_ESCAPE" oncommand="doCancelButton();"/>
So switching to a <dialog> is going to involve some recoding too... we had similar problems with the spellcheck dialog.

>-  <keyset id="dialogKeys"/>
>-  
>   <hbox class="box-header" id="header"
>        title="&importTitle.label;"
>        description="&importShortDesc.label;"/>
> 
>   <deck id="stateDeck" selectedIndex="0">
>     <vbox class="wizard-box">
I can't see what you're replacing the keyset with, but I don't think it's worth touching this window. Unless of course you want to turn it into a <wizard/> ;-)

Comment 2

[Robert Kaiser]

This bug is open but targeted for seamonkey2.0b2, which has been released a significant time ago. Please set the target milestone to an appropriate value, "---" if it has no specific target.

Comment 3

[Robert Kaiser]

Magnus, any chance you can pick this up again and correct the patch according to Neil's nits?

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Not taking on the seamonkey address selector, and leaving the import dialog at least for now.

The virtual folders dialog doesn't react to the defult ok/esc keys until focus changes, but that seems to be the case now too.

Comment 5

[[e-mail]]

Comment on attachment 728775 [details] [diff] [review]
proposed fix, v2

>-function enterKeyPressed()
>+function onOk()
> {
>    // if the add button is currently the default action then add the text
>   if (gHeaderInputElement.value != "" && !gAddButton.disabled)
>   {
>     onAddHeader();
>+    return false;
>   }
>-  else
>-  {
>-    // otherwise, the default action for the dialog is the OK button
>-    if (!gOKButton.disabled)
>-      doOKButton();
>-  }
>-}
> 
>-function onOk()
>-{
r=me on the other dialogs, but I don't think this is going to work because onOk is called when you click the OK button, and you definitely don't want an add to happen in that case. Additionally you're messing with the <dialog>'s default button tracking. If you still want to convert this <window> to a <dialog>, you could try giving the other buttons their own dlgtype (extra1 and extra2 are spare) and then you can alter the <dialog>'s defaultButton property as needed.

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Better fix

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 731551 [details] [diff] [review]
proposed fix, v3

Review of attachment 731551 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/search/content/CustomHeaders.xul
@@ +50,5 @@
> +          <button id="removeButton"
> +                  label="&removeButton.label;"
> +                  accesskey="&removeButton.accesskey;"
> +                  dlgtype="extra2"
> +                  oncommand="onRemoveHeader();"/>

I realized these oncommands aren't neded anymore.

Comment 8

[[e-mail]]

Comment on attachment 731551 [details] [diff] [review]
proposed fix, v3

>-    gOKButton = document.getElementById("ok");
>+    gOKButton = document.documentElement.getButton("accept");
No longer used, might as well remove it entirely.

>+  // only update the button if we disabled state changed
Nit: grammar incorrect

>+  document.documentElement.defaultButton = (aDisable) ? "accept" : "extra1";
Nit: don't need the ()s

>+  if (gRemoveButton.disabled) {
>+    document.documentElement.defaultButton = "accept";
What's this got do do with anything?

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to [e-mail] from comment #8)
> >+  if (gRemoveButton.disabled) {
> >+    document.documentElement.defaultButton = "accept";
> What's this got do do with anything?

Ys seems unnecessary now. In an earlier patch version i didn't move focus to the input field and then there was a strage state that remove was default/focused but disabled.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

Comment 11

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/comm-central/rev/54463e2e941d -> FIXED