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

RESOLVED FIXED in Thunderbird 51.0

Status

Thunderbird
Message Reader UI
RESOLVED FIXED
10 months ago
10 months ago

People

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

Tracking

({regression})

51 Branch
Thunderbird 51.0
regression

Thunderbird Tracking Flags

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

Details

Attachments

(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
(Reporter)

Description

10 months 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.
(Assignee)

Comment 1

10 months 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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Message Reader UI
Ever confirmed: true
Flags: needinfo?(acelists)
Keywords: regression
(Assignee)

Comment 2

10 months ago
Alice, if you have a lot of time, you could look for the regression here.
STR:
- 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

10 months ago
Good: 20160308030405
Bad : 20160327030426
(No windows build between them)

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=05c087337043dd8e71cc27bdb5b9d55fd00aaa26&tochange=63be002b4a803df1122823841ef7633b7561d873
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3e342c12aed2&tochange=5a74c06663ea

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)
(Assignee)

Comment 4

10 months ago
Alice, thank you so much!!!

Aceman, another one we broke, grrr.
(Assignee)

Comment 5

10 months 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)
(Assignee)

Comment 6

10 months ago
Created attachment 8784707 [details] [diff] [review]
Proposed fix (v1).

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

Comment 7

10 months 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

10 months ago
Well, 

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

Comment 9

10 months 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+
(Assignee)

Comment 10

10 months ago
Easy candidate for those looking for something to land ;-)
Keywords: checkin-needed
(Assignee)

Comment 11

10 months 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

10 months ago
http://hg.mozilla.org/comm-central/rev/635867a05c38
http://hg.mozilla.org/releases/comm-aurora/rev/95c294c874f9
http://hg.mozilla.org/releases/comm-beta/rev/bf311c97af59
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-thunderbird49: affected → fixed
status-thunderbird50: affected → fixed
status-thunderbird51: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
(Assignee)

Comment 13

10 months 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.
(Assignee)

Comment 14

10 months 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

10 months 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-)
(Assignee)

Comment 16

10 months 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

10 months 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...
(Reporter)

Comment 18

10 months 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: https://bugzilla.mozilla.org/show_bug.cgi?id=199433
You need to log in before you can comment on or make changes to this bug.