Closed Bug 507541 Opened 10 years ago Closed 2 years ago

Selected text from unrelated message is quoted when right-click replying to another message, cunningly with correct attribution line

Categories

(MailNews Core :: Composition, defect, critical)

defect
Not set
critical

Tracking

(thunderbird_esr5255+ fixed, thunderbird56 fixed, thunderbird57 fixed)

RESOLVED FIXED
Thunderbird 56.0
Tracking Status
thunderbird_esr52 55+ fixed
thunderbird56 --- fixed
thunderbird57 --- fixed

People

(Reporter: jan-bugreport, Assigned: jorgk)

References

(Blocks 1 open bug)

Details

(Keywords: privacy, Whiteboard: [DUPEME][bogus-focus])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.12) Gecko/2009070611 Firefox/3.0.12 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.1) Gecko/20090715 Thunderbird/3.0b3

In TB3 beta 2 and 3 (at least), if you select a piece of text in one e-mail (in the message preview pane), then right-click on another e-mail (in the subject list above) and click reply, the selected text from the open mail (i.e. not the one you are replying to!) is quoted. Additionally, the attribution line ("on date, xy wrote") contains the name of the author of the mail you are replying to, NOT the one the quoted text is from.

Reproducible: Always

Steps to Reproduce:
1. Open a folder with at least 2 mails from different authors. I call them mail 1 from author A and mail 2 from author B.
2. Click on mail 1 so you can see the text in the message pane.
3. Select some of the text
4. Right-Click mail 2, select reply
Actual Results:  
You will see:
On (date), B wrote:
> (text by A from selected mail)

Expected Results:  
Quote complete mail that was right-clicked. Maybe refresh the preview pane before any actions are performed, immediately when the context menu is displayed. As the blue "this message is selected" marking switches immediately, the preview pane should reflect this to avoid confusion.
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090730 SeaMonkey/2.0b2pre, no duplicate found.

This is indeed irritating, the quote and the attribution doesn't match, neither does the subject. Either the context-menu item should be disabled if a quote is selected, or the full other message quoted after selecting it.
Status: UNCONFIRMED → NEW
Component: General → Composition
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: general → composition
Version: unspecified → Trunk
Whiteboard: DUPEME
I've seen several bugs where bogus-focus when right-clicking messages in message list is a problem.
I know some people are saying it's by design to preserve multiple selections (where you can right-click on single msgs without losing such selection). However, such behaviour is nowhere to be found in any other windows application, and hence I doubt many people are using it that way. Otoh, not moving focus on right-click violates basic focus principles, and causes a lot of confusion and bugs. I don't think the following workflow is very intuitive and frequent:
- Select a bunch of messages (so you're intending to act on the bunch, right?),
- then suddenly act by right-click on a single message without seeing the preview of that msg,
- while expecting to keep your selection?
Whiteboard: DUPEME → [DUPEME][bogus-focus]
Duplicate of this bug: 539648
See Also: → 181028
Duplicate of this bug: 729053
Does this still happen? (can't test right now)
If yes:

This is a serious and cunning violation of user privacy, and should be fixed asap.
The cunning part here is that the correct and expected attribution line gets stuck on top the wrong quote from an unrelated message, so without double-checking the quote, user may not even notice sending the wrong thing.
We should really kill those bugs where stuff from unrelated emails silently gets sent to people.
This is among the worst things a mailer can do.

Removing the deviant selection behaviour described in my comment 2 would be the safest way of avoiding bugs like this one, as right-click would first select the new message before doing any command.
The selection behaviour is reminiscent of the other selection bug on contacts side bar, maybe even the same thing because of poor tree implementation. I wouldn't call this a feature, but others might find it useful, as it allows acting differently on a single message while you select multiple messages. See comment 2.
Severity: normal → critical
Flags: needinfo?(jan-bugreport)
OS: Windows XP → All
Hardware: x86 → All
Summary: Wrong quoting/attribution line with selected text and right-click reply to other mail → Selected text from unrelated message is quoted when right-click replying to another message, cunningly with correct attribution line
Keywords: privacy
Still reproduces on 45.8.0 using the original repro steps. Make sure to select enough text in step 3, apparently no quote is made if only a single word is selected.
Flags: needinfo?(jan-bugreport)
Reproducible in Daily TB 55, quoted text is taken from the wrong message. Interesting.
Fix needs to be somewhere in mailCommands.js (ComposeMessage()) or nsMsgComposeService::OpenComposeWindow() which calls nsMsgComposeService::GetOrigWindowSelection(). We need to check that the selection belongs to the message that's being replied to. That's easier said than done. Maybe we should just reset the selection if replying to a different message.
Blocks: 742721
Duplicate of this bug: 742721
This bit of code tests whether the displayed message is being replied to or another message.

In the latter case we should not quote the selection from the displayed message.

Full patch to follow.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Better like this. Clearing the selection doesn't work, but we can't be far off.
Attachment #8891701 - Attachment is obsolete: true
This works, also tested on stand-alone message window, replies can still quote.
Attachment #8891702 - Attachment is obsolete: true
Attachment #8891729 - Flags: review?(acelists)
Comment on attachment 8891729 [details] [diff] [review]
507541-selection-reply.patch - (v3).

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

It is ugly that this can't be solved in the UI, but the composeService grabs into the window and gets the selected text on its own.

::: mail/base/content/mailCommands.js
@@ +163,5 @@
>    let match = /.+#(\d+)/.exec(uri);
>    return (match) ? match[1] : null;
>  }
>  
>  // type is a nsIMsgCompType and format is a nsIMsgCompFormat

Please convert this to the modern format while here ;)

::: mailnews/compose/public/nsIMsgComposeParams.idl
@@ +39,5 @@
>       */
>      const long Redirect                 = 14;
> +
> +    /**
> +     * Add this value to a reply type so suppress quoting the selection.

"*to* suppress"? I'd expand the description to "suppress quoting the current text selection".
Attachment #8891729 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5dcb4353fcec
Ignore selection when not replying to displayed message. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
Comment on attachment 8891729 [details] [diff] [review]
507541-selection-reply.patch - (v3).

The bug is old enough to deserve an uplift.
Attachment #8891729 - Flags: approval-comm-esr52?
Thanks. Yeah, not cool. (although you would hope in most cases the user would notice this)
(In reply to :aceman from comment #13)
> Please convert this to the modern format while here ;)
Done.
 
> "*to* suppress"? I'd expand the description to "suppress quoting the current
> text selection".
Done, I haven't used the word "text" since the selection is HTML in general.

I've also taken the liberty for a further small optimisation:
https://hg.mozilla.org/comm-central/rev/5dcb4353fcec#l1.52
since we already have the message key.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8530044d4d8c
Follow-up: cater for case where no message is displayed. r=me
Sadly that tripped up a test (test-reply-multipart-charset.js) which was doing some stuff while the preview pane was turned off. Being a bit more careful with accessing |gMessageDisplay.displayedMessage| fixes the issue.
Attachment #8891729 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.