1113610 - New version of other actions button breaks CompactHeader addon

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Joachim Herb]

Attachment: broadcaster_for_menu_entries.diff

The new version of the other action button (introduced by Bug 511625) breaks the CompactHeader addon. 

At https://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#260 the hidden attributes of two of the menu entries of the button are set. Unfortunately, this function is called before the otherActionsOpenInNewTab and otherActionsOpenInNewWindow have appeared in the DOM if the CompactHeader addon is installed (and the button has been removed from the toolbar). I also do not know how I could e.g. monkeypatch the function OnLoadMsgHeaderPane() because it is called quite early before addon overlays are loaded (at least, it looks like this to me). 

So I suggest to use a broadcaster instead to set the hidden attribute (or to be more specific to use two of them: One for the messageWindow and one for the messenger). See the attached patch.

Comment 1

[Joachim Herb]

Of course the two lines 
    let openInTab = document.getElementById("otherActionsOpenInNewTab");
    let openInNewWindow = document.getElementById("otherActionsOpenInNewWindow");
have to be removed.

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 8539233 [details] [diff] [review]
broadcaster_for_menu_entries.diff

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

It's better Magnus makes the code review. I've seen only the two nits.

::: mail/base/content/msgHdrViewOverlay.xul
@@ +367,5 @@
>                              oncommand="gConversationOpener.openConversationForMessages(gFolderDisplay.selectedMessages);"/>
>                    <menuitem id="otherActionsOpenInNewWindow"
>                              label="&otherActionsOpenInNewWindow1.label;"
>                              accesskey="&otherActionsOpenInNewWindow1.accesskey;"
> +			    observes="support_tabs"

Please use spaces instead of tabs.

@@ +372,5 @@
>                              oncommand="MsgOpenNewWindowForMessage();"/>
>                    <menuitem id="otherActionsOpenInNewTab"
>                              label="&otherActionsOpenInNewTab1.label;"
>                              accesskey="&otherActionsOpenInNewTab1.accesskey;"
> +			    observes="support_tabs"

Please use spaces instead of tabs.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 8539233 [details] [diff] [review]
broadcaster_for_menu_entries.diff

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

Looks good to me. r=mkmelin with the naming changed and spaces instead of tabs

::: mail/base/content/messenger.xul
@@ +120,5 @@
>    <command id="cmd_close" oncommand="CloseTabOrWindow();"/>
>  </commandset>
>  
> +<broadcasterset id="otherActionButtonBroadcasters">
> +   <broadcaster id="support_tabs"/>

support_tabs doesn't say much. (and also, camelCase is preferred)

how about otherActionsOpenIn

Comment 4

[Joachim Herb]

Attachment: [checked in] bug1113610.patch

Revised patch including the changes requested by :mkmelin.

Check-in needed. Only in trunk or also for Earlybird?

Comment 5

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/3a9f80c236e0 -> FIXED

Comment 6

[Joachim Herb]

The original implementation as well as the one of this bug have a problem: The menu of the other actions button is not setup correctly, if the button is added to the toolbar from the customization dialog, i.e. the two entries about opening the message in a new window/tab are visible. Only after closing and reopening a message window the menu is correct.

The reason is the check of the button:
  if (document.getElementById("otherActionsButton")) {
    let opensAreHidden = document.getElementById("tabmail") ? false : true;
    document.getElementById("otherActionsOpenIn").hidden = opensAreHidden;
  }

But as a broadcaster is used this check is not necessary. See following patch...

Comment 7

[Joachim Herb]

Attachment: Remove obsolete check for button visiblilty

Now the menu is correct also if the other actions button is added newly to a toolbar.

Comment 8

[Joachim Herb]

Attachment: bug1113610-revision-v2.patch

Actually it makes more sense to test of the availability of the broadcaster to make sure that the line 
document.getElementById("otherActionsOpenIn").hidden = opensAreHidden;
does not cause a runtime error if the broadcaster is for whatever reason not available.

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/befb9c45eef6