Last Comment Bug 702201 - Clicking the "Delete" toolbar button with an attachment focused should delete the message, not the attachment
: Clicking the "Delete" toolbar button with an attachment focused should delete...
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Reader UI (show other bugs)
: 8 Branch
: All All
: -- minor (vote)
: Thunderbird 11.0
Assigned To: Jim Porter (:squib)
:
Mentors:
Depends on:
Blocks: 630759
  Show dependency treegraph
 
Reported: 2011-11-13 23:42 PST by Jim Porter (:squib)
Modified: 2011-11-21 05:45 PST (History)
0 users
squibblyflabbetydoo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Fix this and test it (7.62 KB, patch)
2011-11-13 23:42 PST, Jim Porter (:squib)
bwinton: review+
bwinton: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Review

Description Jim Porter (:squib) 2011-11-13 23:42:58 PST
Created attachment 574239 [details] [diff] [review]
Fix this and test it

It turns out that the "delete" toolbar button issues the same command as the "delete" key on the keyboard, so hitting the "delete" toolbar button tries to delete an attachment if it's selected/focused. We should issue a different command for the button to tell these apart. Here's a patch to do this. The patch also removes some dead code that tries to disable the delete button for news messages, on the assumption that "delete" = "cancel" in news.

Since this is technically a change in the UX, I'm also requesting UI review on it.
Comment 1 Blake Winton (:bwinton) (:☕️) 2011-11-17 11:57:41 PST
Comment on attachment 574239 [details] [diff] [review]
Fix this and test it

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

Yes, that makes much more sense to me, since the rest of the buttons in the toolbar act on the message (or at least, not on the attachments).  ui-r=me.

The code looks good too, aside from the question below.  r=me with that fixed or explained.

::: mail/base/content/mailWindowOverlay.xul
@@ +1833,1 @@
>           <toolbarbutton id="button-mark-undelete"

Should the un-delete still use cmd_delete?
Wouldn't that lead to us marking something as deleted, then trying to unmark it, and ending up deleting the attachment?
Comment 2 Jim Porter (:squib) 2011-11-17 23:45:52 PST
Checked in with the un-delete button fixed (good catch!):

http://hg.mozilla.org/comm-central/rev/4bd62c62b092
Comment 3 Jim Porter (:squib) 2011-11-17 23:46:38 PST
Comment on attachment 574239 [details] [diff] [review]
Fix this and test it

Since this is a regression, we should probably get it into aurora at least, and maybe beta too.

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