Last Comment Bug 799451 - Can't remove attachment with keyboard in forwarded e-mail
: Can't remove attachment with keyboard in forwarded e-mail
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Thunderbird 20.0
Assigned To: Magnus Melin
Depends on:
Blocks: 526998
  Show dependency treegraph
Reported: 2012-10-09 04:40 PDT by Onno Ekker [:nONoNonO UTC+1]
Modified: 2013-02-08 11:13 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

proposed fix (2.96 KB, patch)
2012-12-08 03:31 PST, Magnus Melin
squibblyflabbetydoo: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description Onno Ekker [:nONoNonO UTC+1] 2012-10-09 04:40:18 PDT
Sometimes I need to forward e-mails (inline), but I have to strip the attachment. So I select the message, press Ctrl+L to forward the message. Then in the Compose window I press Alt+M to go to the attachments bucket, press Space to select the attachment and press Del to delete it, but nothing happens.

I can delete the attachment by going to the menu Edit -> Remove attachment (Del).

I see the following error in the Error Console:
Timestamp: 9-10-2012 13:35:47
Error: TypeError: boundTarget is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 4175

It doesn't matter whether I'm in safe-mode or not.
Comment 1 Ludovic Hirlimann [:Usul] 2012-10-18 04:35:43 PDT
Onno can you find the regression range ?
Comment 2 Onno Ekker [:nONoNonO UTC+1] 2012-10-18 05:40:29 PDT
This stopped working between Thunderbird 11.0.1 and Thunderbird 12.0.1.
I'll have to dig deeper to narrow this range...
Comment 3 Onno Ekker [:nONoNonO UTC+1] 2012-10-18 08:01:28 PDT
I've tested this with Thunderbird Daily / comm-central:
20120114030033 works
20120115030024 fails

I'm not sure how to go further from this, but from browsing hg around this date I see the following change is suspect, although there are other changes a bit later for backing out the change again:
Comment 4 Onno Ekker [:nONoNonO UTC+1] 2012-11-05 01:38:50 PST
Next change is - Jim Porter — Backout changeset 25f87dd2453f due to test bustage in test-attachment.js
Comment 5 Jim Porter (:squib) 2012-11-05 10:02:10 PST
This WFM on a fairly recent nightly.
Comment 6 Onno Ekker [:nONoNonO UTC+1] 2012-11-05 11:44:12 PST
Strange. I just tried with Daily 20121105030251 and it fails... I've used a fresh profile and tried both with composition in HTML as in plain text and both fail with the same error message, only with linenumber 4272.
I've tried both with Windows 7 32 bit and Windows XP and it fails on both OS-es.
Comment 7 Onno Ekker [:nONoNonO UTC+1] 2012-11-06 03:34:56 PST
The bug not only occurs when I press Alt+M, but also when I use Tab to go to the first attachment and press space to select it and then press Del. When I use Tab to go to the attachment, there is no error message in the error console.

When I select the message with Alt+M and space, press Shift+Tab to go to the Subject and press Tab again to go back to the AttachmentBucket, removing the attachment by pressing Del *does* work...
Comment 8 Magnus Melin 2012-11-30 12:30:55 PST
Yeah this is from bug 526998  - and 100% reproducible for me on linux.

Apparently due to a bug in listbox.xml code earlier there was an ugly workaround 

let enabled = bucketList && bucketList.getRowCount() && (bucketList == top.document.commandDispatcher.focusedElement);
Comment 9 Magnus Melin 2012-12-08 03:31:35 PST
Created attachment 690075 [details] [diff] [review]
proposed fix

What seems to happen is:
cmd_delete is one of the commands that is updated when focus changes (since it is used elsewhere too).
When entering the attachments area using keyboard (Alt+M) the attachmentBucket gets focus,
 -> command updating, but at that point there's no selection so cmd_delete is
disabled. When an item gets selected that doesn't cause the commands to get
updated, so cmd_deleted is always disabled.

There was also an exception for keyboard "clicks", since then the attachmentBucket is the event's original target, and that isn't anonymous, so doesn't have a bindingparent.
Comment 10 Magnus Melin 2012-12-08 03:33:06 PST
(And I don't think there was a bug in listbox.xml like earlier stated.)
Comment 11 Jim Porter (:squib) 2012-12-10 01:06:23 PST
Comment on attachment 690075 [details] [diff] [review]
proposed fix

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

I've never been able to reproduce this, even though I'm on Linux too, so I'll just trust you that this fixes it. The code *looks* sane, anyway.
Comment 12 Magnus Melin 2012-12-10 11:44:44 PST -> FIXED
Comment 13 Thomas D. (needinfo?me) 2012-12-10 11:55:27 PST
Would this be safe to land for versions earlier than TB20, esp esr17 for corporate environs?
Comment 14 Magnus Melin 2012-12-10 22:14:56 PST
Comment on attachment 690075 [details] [diff] [review]
proposed fix

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

This should be save to take on esr.

On a related note i don't know where the Alt+M shortcut to select the attachment bucket is coming from, but it seems undocumented?
Comment 15 Thomas D. (needinfo?me) 2012-12-11 05:15:11 PST
(In reply to Magnus Melin from comment #14)
> On a related note i don't know where the Alt+M shortcut to select the
> attachment bucket is coming from, but it seems undocumented?

It comes from the attachment pane caption "1 attach_m_ent", which is only found in attachment pane of /composition/. So it's an *access key* (not a keyboard shortcut), and we generally do not document access keys because they are always visible in the primary UI itself. On some OS, you have to press Alt to see the access keys underlined in the UI.
Comment 16 Mark Banner (:standard8, limited time in Dec) 2013-01-03 04:12:12 PST
Comment on attachment 690075 [details] [diff] [review]
proposed fix

[Triage Comment]
Given the age of when this was introduced, we'll let the patch soak during the 19 beta cycle and take it into 17.0.2 (or whatever it is equivalent to when gecko 19 is released).

So we'll take it onto aurora now before the merge, so it'll be ready for the next beta.
Comment 17 Mark Banner (:standard8, limited time in Dec) 2013-01-03 04:14:06 PST
Comment 18 Mark Banner (:standard8, limited time in Dec) 2013-02-08 11:13:28 PST

Note You need to log in before you can comment on or make changes to this bug.