Last Comment Bug 507622 - remove some usage of obsolete dialogOverlay.xul in mailnews/
: remove some usage of obsolete dialogOverlay.xul in mailnews/
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 23.0
Assigned To: Magnus Melin
Depends on:
  Show dependency treegraph
Reported: 2009-07-31 05:55 PDT by Magnus Melin
Modified: 2013-04-04 03:13 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

proposed fix (22.08 KB, patch)
2009-07-31 05:55 PDT, Magnus Melin
neil: superreview-
Details | Diff | Splinter Review
proposed fix, v2 (10.88 KB, patch)
2013-03-24 13:54 PDT, Magnus Melin
neil: review-
Details | Diff | Splinter Review
proposed fix, v3 (11.45 KB, patch)
2013-03-30 14:29 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
proposed fix, v4 (11.77 KB, patch)
2013-04-01 03:18 PDT, Magnus Melin
neil: review+
Details | Diff | Splinter Review

Description User image Magnus Melin 2009-07-31 05:55:23 PDT
Created attachment 391862 [details] [diff] [review]
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 User image 2009-07-31 13:55:06 PDT
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 User image Robert Kaiser 2009-12-20 07:43:36 PST
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 User image Robert Kaiser 2010-04-13 08:03:09 PDT
Magnus, any chance you can pick this up again and correct the patch according to Neil's nits?
Comment 4 User image Magnus Melin 2013-03-24 13:54:05 PDT
Created attachment 728775 [details] [diff] [review]
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 User image 2013-03-26 17:01:22 PDT
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 User image Magnus Melin 2013-03-30 14:29:58 PDT
Created attachment 731551 [details] [diff] [review]
proposed fix, v3

Better fix
Comment 7 User image Magnus Melin 2013-03-31 01:57:34 PDT
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 User image 2013-03-31 17:01:59 PDT
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 User image Magnus Melin 2013-04-01 03:17:50 PDT
(In reply to 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 User image Magnus Melin 2013-04-01 03:18:34 PDT
Created attachment 731808 [details] [diff] [review]
proposed fix, v4
Comment 11 User image Magnus Melin 2013-04-04 03:13:42 PDT -> FIXED

Note You need to log in before you can comment on or make changes to this bug.