Closed Bug 1113610 Opened 5 years ago Closed 5 years ago

New version of other actions button breaks CompactHeader addon

Categories

(Thunderbird :: Message Reader UI, defect)

36 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 37.0

People

(Reporter: joachim.herb, Assigned: joachim.herb)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attachment #8539233 - Flags: review?(richard.marti)
Attachment #8539233 - Flags: review?(mkmelin+mozilla)
Of course the two lines 
    let openInTab = document.getElementById("otherActionsOpenInNewTab");
    let openInNewWindow = document.getElementById("otherActionsOpenInNewWindow");
have to be removed.
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.
Attachment #8539233 - Flags: review?(richard.marti)
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
Attachment #8539233 - Flags: review?(mkmelin+mozilla) → review+
Revised patch including the changes requested by :mkmelin.

Check-in needed. Only in trunk or also for Earlybird?
Assignee: nobody → joachim.herb
Attachment #8539233 - Attachment is obsolete: true
Attachment #8539816 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/3a9f80c236e0 -> FIXED
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 37.0
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...
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Now the menu is correct also if the other actions button is added newly to a toolbar.
Attachment #8543470 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8543472 - Flags: review?(mkmelin+mozilla)
Attachment #8543470 - Attachment is obsolete: true
Attachment #8543470 - Flags: review?(mkmelin+mozilla)
Attachment #8539816 - Attachment description: bug1113610.patch → [checked in] bug1113610.patch
Attachment #8543472 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.