Closed Bug 1297674 Opened 8 years ago Closed 8 years ago

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

Categories

(Thunderbird :: Message Reader UI, defect)

51 Branch
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Thunderbird 51.0
Tracking Status
thunderbird48 --- wontfix
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr24 --- unaffected
thunderbird_esr38 --- unaffected
thunderbird_esr45 --- unaffected
thunderbird51 --- fixed

People

(Reporter: psnmbox, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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
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)
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
Alice, thank you so much!!!

Aceman, another one we broke, grrr.
Clearing the tracking flags. I will land on TB 51 and uplift to TB 50 and TB 49.
Flags: needinfo?(acelists)
Attached patch Proposed fix (v1). (obsolete) — Splinter Review
One review only, whoever comes first.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8784707 - Flags: review?(mkmelin+mozilla)
Attachment #8784707 - Flags: review?(acelists)
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+
Well, 

let inDraftFolder = (msg && msg.folder &&
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+
Easy candidate for those looking for something to land ;-)
Keywords: checkin-needed
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+
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.
(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.
(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-)
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.
(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...
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.

Attachment

General

Created:
Updated:
Size: