(Message Header) Missing context menu to copy Subject line when mail opened in new window

RESOLVED FIXED in Thunderbird 43.0

Status

Thunderbird
Message Reader UI
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: llamaunix, Assigned: gportioli)

Tracking

Trunk
Thunderbird 43.0
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091121 Thunderbird/3.0

When viewing an email message in a new window, the right-click menu for the subject line does not show. Right-clicking on the subject line produces no menu. However, when the email is opened in a tab, or viewed from the message pane, the right-click menu does show properly. This prevents a user from copying the subject or part of a subject when viewing an email in a new message window.

Note: if highlight the subject and press ctrl+c, the subject is copied to the clipboard though.

Reproducible: Always

Steps to Reproduce:
1. Change the "Open messages in" setting to "a new message window"
2. Open an email
3. Highlight the subject, or part of the subject line
4. Right click on the highlighted text
Actual Results:  
No right click menu appears showing the Copy menu item.

Expected Results:  
A menu should appear showing the Copy menu item

Here is a screenshot of the right-click menu that's shown correctly when right-clicking on the subject line when viewing an email through the message pane:

http://imgur.com/YQA50.gif
Confirmed also in Windows 7.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091201 Lightning/1.0b1pre Shredder/3.0.1pre ID:20091201032021
Status: UNCONFIRMED → NEW
Component: Toolbars and Tabs → Message Reader UI
Ever confirmed: true
QA Contact: toolbars-tabs → message-reader
Summary: Right-click menu missing for Subject line when mail opened in new window → (Message Header) Missing context menu to copy Subject line when mail opened in new window
Version: unspecified → Trunk
(Assignee)

Comment 2

2 years ago
Hi there,

I've got a fix for this bug. Can someone please assign it to me?

Cheers!

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED

Comment 3

2 years ago
(In reply to Giulio Portioli [:gportioli] from comment #2)
> Hi there,
> 
> I've got a fix for this bug. Can someone please assign it to me?
> 
> Cheers!

Great! Please attach your fix as a patch and request review.
(Assignee)

Comment 4

2 years ago
Created attachment 8645178 [details] [diff] [review]
Rev 1 - Missing context menu and DTD added to XUL file

This patch adds to messageWindow.xul the "copyPopup" XUL element that produces the context menu that is currently missing from the message window. It also adds the DTD required by the copyPopup element.
All the code added by this patch has been copied and pasted from messenger.xul, where it correctly works for the main Thunderbird window.

TESTING PERFORMED:
In a new message window, right-clicked on the Subject line; the context menu appeared as expected (i.e. same behaviour as for tabbed messages in the main TB window) and also worked as expected ('Copy', 'Create rule from filter').

REGRESSION TESTING PERFORMED:
Right-clicked on all areas of TB's main window, tabbed message and message window; all context menus appeared as expected.
Attachment #8645178 - Flags: review?(squibblyflabbetydoo)

Comment 5

2 years ago
It would be nice to avoid duplicating this code, and instead to remove the existing duplication of the other menupopups in messageWindow.xul too. Was there a reason these had to be duplicated in the first place?
Flags: needinfo?(richard.marti)
I'm sorry I can't say why they are duplicated. If it's possible to remove the menupopups this would it make easier maintainable.
Flags: needinfo?(richard.marti)

Comment 7

2 years ago
Comment on attachment 8645178 [details] [diff] [review]
Rev 1 - Missing context menu and DTD added to XUL file

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

Giulio: OK, so now we know that adding that code to messageWindow.xul works. But if we can, we should avoid duplicated code, because it can lead to bugs in the future if one copy is modified but not the other.

So could you try instead of copying the code across, actually removing whatever is duplicated in messageWindow.xul (the other menupopups in particular). Then, to make that code available to both messenger.xul and messageWindow.xul, it has to be moved to a file that is included in both places. I would suggest trying mailWindowOverlay.xul, which is already included as a xul-overlay in both.
Attachment #8645178 - Flags: review?(squibblyflabbetydoo) → feedback+
(Assignee)

Comment 8

2 years ago
Created attachment 8645400 [details] [diff] [review]
Rev 2 - Duplicated menu popups moved from XUL base files to overlay

I've done as suggested and it seems to work. Re-tested satisfactorily as in comment No. 4.

I'm not sure about one point though: is it OK to remove completely the menupopup definitions from the base files (messenger.xul and messageWindow.xul), as I've done in this patch? or should a declaration of each menu popup, like

   <menupopup id="emailAddressPopup" />

be left in the base file, in place of the definition that has been moved to the overlay? I'm asking because I see that the base files are full of declarations like the above one for XUL elements defined elsewhere (but the popup menus in question work regardless).
Attachment #8645178 - Attachment is obsolete: true
Attachment #8645400 - Flags: review?(aleth)

Comment 9

2 years ago
(In reply to Giulio Portioli [:gportioli] from comment #8)
> Created attachment 8645400 [details] [diff] [review]
> Rev 2 - Duplicated menu popups moved from XUL base files to overlay
> 
> I've done as suggested and it seems to work. Re-tested satisfactorily as in
> comment No. 4.
> 
> I'm not sure about one point though: is it OK to remove completely the
> menupopup definitions from the base files (messenger.xul and
> messageWindow.xul), as I've done in this patch? or should a declaration of
> each menu popup, like
> 
>    <menupopup id="emailAddressPopup" />
> 
> be left in the base file, in place of the definition that has been moved to
> the overlay? I'm asking because I see that the base files are full of
> declarations like the above one for XUL elements defined elsewhere (but the
> popup menus in question work regardless).

The rule is "Bases and overlays are merged when they share the same ID attribute."
What the code should be doing is this: 
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Overlays#UI_Reuse_with_Overlays

For example, your current patch works (I think) because the <popupset> is overlaid, which automatically adds all the child nodes, including the menupopup id="emailAddressPopup".

It seems like it could possibly be tidied up a bit. You could go through (e.g. with dxr) and add comments saying which overlay file will override a particular menupopup/popupset declaration in messenger.xul/messageWindow.xul. That should clarify what is currently a bit confusing (and may reveal some more duplication we can get rid of).
(Assignee)

Comment 10

2 years ago
Created attachment 8645788 [details] [diff] [review]
Rev 3 - Duplicated menu popups moved from base files to overlay; tidy-up

I haven't found any more duplication among the menupopups of messenger.xul, messageWindow.xul and the overlays. The only thing slightly out of place was in messageWindow.xul, where these two menupopups:

<menupopup id="messageIdContext"/>
<menupopup id="mailContext"/>

were declared inside the <popupset id="mainPopupSet"> node, whereas in the overlay that provides them and the popupset, they are not part of any popupset, so I've moved them out of the popupset in the base file and rearranged them so that they are grouped consistently between the two base files (I don't suppose it matters in which order the menupopup nodes appear in the XUL base file, unlike the menuitem nodes).

Comments also added with reference to relevant overlay. Re-tested as before with satisfactory result.
Attachment #8645400 - Attachment is obsolete: true
Attachment #8645400 - Flags: review?(aleth)
Attachment #8645788 - Flags: review?(aleth)

Comment 11

2 years ago
Comment on attachment 8645788 [details] [diff] [review]
Rev 3 - Duplicated menu popups moved from base files to overlay; tidy-up

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

Thanks for disentangling this! It is a good idea because this bug was probably an accidental side-effect of the overlay structure having become too confusing.

This is r- but there's just some style nits left to do.

::: mail/base/content/mailWindowOverlay.xul
@@ +2137,5 @@
> +  - The following popup menus were previously defined twice: in messenger.xul
> +  - and also in messageWindow.xul. To eliminate duplication (and fix bug 532084
> +  - at the same time), they have been moved to this overlay, which is included
> +  - by both the above files.
> +  -->

Now things are tidied up, we can streamline this comment to "Used by messenger.xul and messageWindow.xul" and move it above the <popupset>. Are there any other files that include mailWindowOverlay.xul and use that popupset? Seems worth checking.

::: mail/base/content/messageWindow.xul
@@ +97,1 @@
>      <panel id="editContactPanel"/>

Same here as in messenger.xul.

::: mail/base/content/messenger.xul
@@ +127,5 @@
>  <keyset id="mailKeys">
>    <keyset id="tasksKeys"/>
>  </keyset>
>  
> +<popupset id="mainPopupSet">    <!-- Provided by mailWindowOverlay.xul -->

Move the comment above the <popupset>, I think that's the usual style.

@@ +132,1 @@
>    <panel id="editContactPanel"/>

If this is in the overlay, we shouldn't need the panel here.

@@ +141,5 @@
> +<menupopup id="copyUrlPopup"/>                    <!-- msgHdrViewOverlay.xul -->
> +<menupopup id="toolbar-context-menu"/>            <!-- mailWindowOverlay.xul -->
> +<menupopup id="remoteContentOptions"/>            <!-- mailWindowOverlay.xul -->
> +<menupopup id="phishingOptions"/>                 <!-- mailWindowOverlay.xul -->
> +<menupopup id="folderPaneContext"/>               <!-- mailWindowOverlay.xul -->

Please group all the ones from each file together, put a blank line in between and a single comment above each block.
Attachment #8645788 - Flags: review?(aleth) → review-

Comment 12

2 years ago
(In reply to Giulio Portioli [:gportioli] from comment #10)
> The only thing slightly out of place was
> in messageWindow.xul, where these two menupopups:
> 
> <menupopup id="messageIdContext"/>
> <menupopup id="mailContext"/>
> 
> were declared inside the <popupset id="mainPopupSet"> node, whereas in the
> overlay that provides them and the popupset, they are not part of any
> popupset

Sounds like this was another bug, though possibly one without consequences...
(Assignee)

Comment 13

2 years ago
Created attachment 8646081 [details] [diff] [review]
Rev 4 - Duplicated popups, code layout and comments sorted

From what I can see, there are no other files under comm-central\mail that use "mainPopupSet" included from mailWindowOverlay.xul, apart from the two base files in question.

Similarly to the two <menupopup> mentioned in comment #10, <panel id="editContactPanel"/> too is not part of any popupset, in the overlay file that provides it, so I've taken it out of the popupset in the two base files and changed the popupset element there to an empty one, which seems to work just as well.

I've repeated the tests in comment #4 and I still see all the popup menus I was seeing before (plus the missing one), including the 'Edit contact' panels.
Attachment #8645788 - Attachment is obsolete: true
Attachment #8646081 - Flags: review?(aleth)

Comment 14

2 years ago
Comment on attachment 8646081 [details] [diff] [review]
Rev 4 - Duplicated popups, code layout and comments sorted

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

This is a great improvement, thanks for fixing this!
Attachment #8646081 - Flags: review?(aleth) → review+

Updated

2 years ago
Keywords: checkin-needed
Thanks!

https://hg.mozilla.org/comm-central/rev/7ce9f4763bfa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
You need to log in before you can comment on or make changes to this bug.