remove some usage of obsolete dialogOverlay.xul in mailnews/

RESOLVED FIXED in Thunderbird 23.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Magnus Melin, Assigned: Magnus Melin)

Tracking

Trunk
Thunderbird 23.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

11.77 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
Attachment #391862 - Flags: superreview?(neil)
Attachment #391862 - Flags: review?(neil)

Comment 1

8 years ago
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/> ;-)
Attachment #391862 - Flags: superreview?(neil)
Attachment #391862 - Flags: superreview-
Attachment #391862 - Flags: review?(neil)
Version: unspecified → Trunk

Comment 2

7 years ago
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.
(Assignee)

Updated

7 years ago
Target Milestone: seamonkey2.0b2 → ---

Comment 3

7 years ago
Magnus, any chance you can pick this up again and correct the patch according to Neil's nits?
(Assignee)

Updated

4 years ago
Summary: remove usage of obsolete dialogOverlay.xul in mailnews/ → remove some usage of obsolete dialogOverlay.xul in mailnews/
(Assignee)

Updated

4 years ago
Component: MailNews: General → Mail Window Front End
Product: SeaMonkey → Thunderbird
(Assignee)

Comment 4

4 years ago
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.
Attachment #391862 - Attachment is obsolete: true
Attachment #728775 - Flags: review?(neil)

Comment 5

4 years ago
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.
Attachment #728775 - Flags: review?(neil) → review-
(Assignee)

Comment 6

4 years ago
Created attachment 731551 [details] [diff] [review]
proposed fix, v3

Better fix
Attachment #728775 - Attachment is obsolete: true
Attachment #731551 - Flags: review?(neil)
(Assignee)

Comment 7

4 years ago
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

4 years ago
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?
(Assignee)

Comment 9

4 years ago
(In reply to neil@parkwaycc.co.uk 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.
(Assignee)

Comment 10

4 years ago
Created attachment 731808 [details] [diff] [review]
proposed fix, v4
Attachment #731551 - Attachment is obsolete: true
Attachment #731551 - Flags: review?(neil)
Attachment #731808 - Flags: review?(neil)

Updated

4 years ago
Attachment #731808 - Flags: review?(neil) → review+
(Assignee)

Comment 11

4 years ago
http://hg.mozilla.org/comm-central/rev/54463e2e941d -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
You need to log in before you can comment on or make changes to this bug.