Menu item 'Copy' is inactive when viewing .eml attachment

RESOLVED FIXED in Thunderbird 51.0


Message Reader UI
a year ago
a year ago


(Reporter: psn, Assigned: Jorg K (GMT+2))



51 Branch
Thunderbird 51.0

Thunderbird Tracking Flags

(thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr24 unaffected, thunderbird_esr38 unaffected, thunderbird_esr45 unaffected, thunderbird51 fixed)



(1 attachment, 1 obsolete attachment)

1.24 KB, patch
Jorg K (GMT+2)
: review+
Jorg K (GMT+2)
: approval-comm-aurora+
Jorg K (GMT+2)
: approval-comm-beta+
Details | Diff | Splinter Review


a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160823072522

Steps to reproduce:

Windows 10 32-bit.
I received copy of an e-mail as attached .eml file.
Clicked on it and the attachment was opened in another window.
Then selected some text in the window (could be done with keyboard or mouse).

Actual results:

In context menu items Copy and Select All are always disabled even though corresponding keyboard shortcuts are active.

Expected results:

The message is in read-only state, but context menu items Copy and Select All should be active.

Comment 1

a year ago
Hmm, in my TB 51 Daily the context (right-click) menu looks pretty terrible on a message opened from a file, there are even empty entries. So something has gone bust there.

|Copy/Select all| are disabled. This worked OK in TB 45. Thanks for reporting this.

Aceman, can you have a look.
Component: Untriaged → Message Reader UI
Ever confirmed: true
Flags: needinfo?(acelists)
Keywords: regression

Comment 2

a year ago
Alice, if you have a lot of time, you could look for the regression here.
- open an .eml file using File > Open > Saved Message
  (Menu varies in different versions).
- Right click on the message body and inspect the menu.
  The menu should have about 10 entries, "Select All" at the top.

Right now, in TB 51, it has about 50+ entries all mixed up terribly.

TB 45 was OK, TB 51 is not.
Flags: needinfo?(alice0775)

Comment 3

a year ago
Good: 20160308030405
Bad : 20160327030426
(No windows build between them)

Regression window:

Regressed by:
e2845f0b82eb	Thomas Düllmann — Bug 1106412 - Implement Edit Draft Message Command. r=jorgk

Error console in the bad build:
Timestamp: 2016/08/25 10:38:25
Error: TypeError: msg.folder is null
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 575

Timestamp: 2016/08/25 10:38:27
Error: TypeError: gContextMenu is undefined
Source File: chrome://messenger/content/mailContextMenus.js
Line: 50
Blocks: 1106412
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → affected
status-thunderbird51: --- → affected
status-thunderbird_esr24: --- → unaffected
status-thunderbird_esr38: --- → unaffected
status-thunderbird_esr45: --- → unaffected
tracking-thunderbird49: --- → ?
tracking-thunderbird50: --- → ?
tracking-thunderbird51: --- → ?
Flags: needinfo?(alice0775)

Comment 4

a year ago
Alice, thank you so much!!!

Aceman, another one we broke, grrr.

Comment 5

a year ago
Clearing the tracking flags. I will land on TB 51 and uplift to TB 50 and TB 49.
tracking-thunderbird49: ? → ---
tracking-thunderbird50: ? → ---
tracking-thunderbird51: ? → ---
Flags: needinfo?(acelists)

Comment 6

a year ago
Created attachment 8784707 [details] [diff] [review]
Proposed fix (v1).

One review only, whoever comes first.
Assignee: nobody → jorgk
Attachment #8784707 - Flags: review?(mkmelin+mozilla)
Attachment #8784707 - Flags: review?(acelists)

Comment 7

a year ago
Comment on attachment 8784707 [details] [diff] [review]
Proposed fix (v1).

Review of attachment 8784707 [details] [diff] [review]:

Works, but I'd simplify it like below.

::: mail/base/content/mailWindowOverlay.js
@@ +580,2 @@
>    let folder = gFolderDisplay.displayedFolder;
>    let inDraftFolder = (msg &&

You could just make this row

let inDraftFolder = (msg && msg.folder
Attachment #8784707 - Flags: review?(mkmelin+mozilla)
Attachment #8784707 - Flags: review?(acelists)
Attachment #8784707 - Flags: review+

Comment 8

a year ago

let inDraftFolder = (msg && msg.folder &&

Comment 9

a year ago
Created attachment 8784969 [details] [diff] [review]
Proposed fix (v1b).

Thanks, that was quick ;-)
Your approach is nicer. I left the comment in, I hope you don't mind.
Attachment #8784707 - Attachment is obsolete: true
Attachment #8784969 - Flags: review+

Comment 10

a year ago
Easy candidate for those looking for something to land ;-)
Keywords: checkin-needed

Comment 11

a year ago
Comment on attachment 8784969 [details] [diff] [review]
Proposed fix (v1b).

[Approval Request Comment]
Regression caused by (bug #): Bug 1106412
User impact if declined: Right menu for messages opened from file doesn't work.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
No risk.
Attachment #8784969 - Flags: approval-comm-beta+
Attachment #8784969 - Flags: approval-comm-aurora+

Comment 12

a year ago
Last Resolved: a year ago
status-thunderbird49: affected → fixed
status-thunderbird50: affected → fixed
status-thunderbird51: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0

Comment 13

a year ago
Thanks for pushing this. However, as already discussed in bug 507640 comment #60, I handle the uplifts personally. I do them in blocks in order not to push trivial changes individually. If you want this job, you can have it ;-) In this case I also wanted to push bug 1211005. Now I have to do this separately.

Comment 14

a year ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #13)
> Now I have to do this separately.
And I will land them with DONTBUILD since the test can run next time a build is done, most likely through a blocklist update on the weekend.

Comment 15

a year ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #10)
> Easy candidate for those looking for something to land ;-)

In that case, you probably shouldn't have set the "checkin-needed" flag or provide some instructions where to land in the whiteboard. And no, I don't want to have that job. 8-)

Comment 16

a year ago
My comment was mostly for the Aurora and beta uplifts that I usually handle myself.
Anyway, the push to C-C revealed a new test failure: Bug 1298339.

Comment 17

a year ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #16)
> My comment was mostly for the Aurora and beta uplifts

I know, but it was ambiguous. I had usually put something like "[c-n: comm-central]" into the whiteboard with a checkin-needed to be specific where a patch has to land.

> Anyway, the push to C-C revealed a new test failure: Bug 1298339.

Well, at least something good came out of it...

Comment 18

a year ago
Checked today's 32-bit update and this bug appears to be fixed.

Though while reading your e-mail exchange I see a different issue, possibly this one:
You need to log in before you can comment on or make changes to this bug.