57566 - Can't delete mail attachments using the mouse

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[Blake Ross]

Steps to Reproduce:

(1) Double-click the Mozilla icon on your desktop to open the browser.
(2) Click File > New > Message
(3) Click the Attachments button
(4) (Double?) Click the name of the file you wish to attach
(5) Oops! You clicked the wrong file, silly. Click the name of the attachment 
in the tree (if not already focused)
(6) Press the Delete key

Results: everything up to step 6 can be accomplished using the mouse.  So why 
do I have to go to the keyboard to complete step 6?  Edit | Delete should be 
enabled when one or more attachments is/are selected (filing separate bug to 
make the tree multiselect).  A context menu might also be added.  cc'ing 
Matthew for implementation ideas here.

Comment 1

[Matthew T (active 1999-2002)]

The following should all work when an attachment is selected:
* pressing the Backspace key
* pressing the Delete key
* clicking the `Delete' button in the toolbar (which should be right next to the
  `Paste' button -- sorry, the composition window hangs my computer at the
  moment, so I can't see whether it has a Delete button yet or not)
* dragging the attachment to the Recycle Bin/Trash
* choosing `Edit' > `Delete' from the composition window menu
* choosing `Delete' from the attachment's context menu.

Comment 2

[Jean-Francois Ducarroz]

Accepting

Comment 3

[scottputterman]

changing milestone to unknown.  It will get changed back when we figure out what
milestone to put this bug in.

Comment 4

[fenella]

Assign it to myself.

Comment 5

[fenella]

to Esther ..

Comment 6

[Jean-Francois Ducarroz]

->varada. We already have a bug for the menu item disable. But this bug is more
complete than the other one.

Comment 7

[laurel]

*** Bug 112030 has been marked as a duplicate of this bug. ***

Comment 8

[laurel]

Current build (nov26 commercial trunk) Delete key or backspace does work. Other
bug about context menu for delete attachment is bug 56339.

Comment 9

[Andreas Premstaller]

Attachment: Proposed patch, V0.1

The patch 1) enables the delete menu item when an attachment is selected and 2)
adds an attachment context menu as described in the mailnews-specs
(http://www.mozilla.org/mailnews/specs/compose/Comp_Menus.html#ContextSpecificMenus).


Actually, to achieve 1) an additional menu item is created for deleting
attachments, but it is hidden unless an attachment is selected. If an
attachment is selected, the generic menu_delete is hidden. 

The patch was created against build 2002020208 on W2K using Patchmaker.

varada, can you please have a look at the patch and add any comments here?

Comment 10

[(not reading, please use [e-mail] instead)]

taking from varada.

I'll work on landing this during 0.9.9

Comment 11

[Andreas Premstaller]

Seth, would you prefer me to try and insert the new commands in the command
chain of the defaultController in MsgComposeCommands.js? If yes, can you give me
any hint were cmd_delete is handled and where I should add the check if
attachments have focus?

Comment 12

[(not reading, please use [e-mail] instead)]

over to sean, this is related to accessibility.

sean, there's a patch, but I haven't looked at it yet.

Comment 13

[scottputterman]

cc'ing varada.  Varada or Sean, could you look at this patch?  Jennifer, does
this sound ok to you?

Comment 14

[jglick]

From mpt's comment #1

>The following should all work when an attachment is selected:
>* pressing the Backspace key
>* pressing the Delete key
>* dragging the attachment to the Recycle Bin/Trash
>* choosing `Edit' > `Delete' from the composition window menu
>* choosing `Delete' from the attachment's context menu.

Context menu for a selected attachment:

Delete 
Select All
-------------  
Add Attachment

Comment 15

[Sean Su]

Comment on attachment 67688 [details] [diff] [review]
Proposed patch, V0.1

a few things I found:

please use '.hasChildNodes()' instead of '.length' to check for attachments.

If there are attachments, but none are selected, the 'Delete' context menu item
needs to be disabled.

If the 'bucketBody' has the focus and there are unselected attachments in the
attachments box, the 'Edit'|'Select All' menu item should select all
attachments.

Comment 16

[Sean Su]

Andreas, are you working on a revised patch or did you want me to take over it?
 Just wondering because this bug is still marked for 0.9.9.

Comment 17

[Andreas Premstaller]

Sean, I am away for travel until 2002-02-25 (Monday), and will then change the
patch according to your suggested changes. Sorry for the delay, feel free to
take over the patch if it needs to be fixed earlier (or shift the target milestone).

Comment 18

[scottputterman]

moving to 1.0

Comment 19

[Andreas Premstaller]

Attachment: Patch, V0.2

Here is a revised version of the patch that takes care of most things Sean
mentioned in Comment #15.

> please use '.hasChildNodes()' instead of '.length' to check for attachments.
done, however, there are other occurrences of childnode.length in the file that
we may want to replace in a final patch

> If there are attachments, but none are selected, the 'Delete' context menu
> item needs to be disabled.
fixed, I forgot half a line when creating patch V0.1.

> If the 'bucketBody' has the focus and there are unselected attachments in the

> attachments box, the 'Edit'|'Select All' menu item should select all
> attachments.
Mostly done, the menu item now works correctly but I need a little help how to
implement the key binding for select all. The check for del/backspace is done
from the attachmentTree via an onkeypress. What's the way to go here?

Comment 20

[Andreas Premstaller]

Regarding comment #1 from mpt and comment #14 from jglick: 
Currently working (without patch)
* pressing the Backspace key
* pressing the Delete key

Additionally working with patch
* choosing `Edit' > `Delete' from the composition window menu
* choosing `Delete' from the attachment's context menu.
* Context menu as described in the mail-specs

Not working
* dragging the attachment to the Recycle Bin/Trash
I have No clue if that can be done with js alone.

Comment 21

[Sean Su]

>> If the 'bucketBody' has the focus and there are unselected attachments in the
>> attachments box, the 'Edit'|'Select All' menu item should select all
>> attachments.
>Mostly done, the menu item now works correctly but I need a little help how to
>implement the key binding for select all. The check for del/backspace is done
>from the attachmentTree via an onkeypress. What's the way to go here?

I would suggest adding a cmd_selectall: to the default handler in
MsgComposeCommands.js and performing the select all of the attachments there. 
Let me know if it doesn't work.

As for 'dragging the attachment to the Recycle Bin/Trash', I wouldn't worry
about that right now.  I don't know how to get that working either.  Ben Goodger
would probably know how.

Comment 22

[Jean-Francois Ducarroz]

If you add a cmd_SelectAll attachment handler (which seems to be a good idea),
you need to make sure it doesn't break the Select all when the message body has
the focus. In that case editor take care of that.

Comment 23

[Sean Su]

when I did my little test, it didn't break the functionality in the compose
(message) pane.  But you shouldn't just take my word for it :)

Comment 24

[Andreas Premstaller]

Attachment: Patch, V0.3

As Sean suggested, I have added "cmd_selectAll" to the defaultController.
Everything works fine for me, including the shortcut, and Select All continues
to work in message body, to: and subject field (without any special action
needed).

Comment 25

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 26

[Andreas Premstaller]

Attachment: Patch V0.4, now uses default controller for delete and select

The patch has undergone a major overhaul. Delete and Select All for attachments
are now integrated into the command handler of the compose window. 

Edit and context menu now use the same code for enabling/disabling, therefore
the patch got smaller. ssu's suggestion to use "command="cmd_..."" led me to
the better solution. As a further benefit, Select All in the Edit menu is now
disabled (as the context menu item was all the time) if no attachments are
present and the attachment bucket has the focus.

ssu, seth, would you please take a look at the patch?

Comment 27

[Sean Su]

Comment on attachment 72954 [details] [diff] [review]
Patch V0.4, now uses default controller for delete and select

wow, it looks really nice now.

r=ssu

jglick, the Mail Compose Context menu specs is slightly incorrect.  The access
key for the "Add Attachment" in the attachments area should be something other
than 'A' (already taken by 'Select All').  I recommend using what is in
Andreas' patch, 'T'.

Also, in addition to the comment that's already there:
  "Delete" greyed if attachment is not selected.

You should probably add another comment for the 'Select All' menu item:
  "Select All" greyed if there are no attachments.

Andreas' patch does that too.

Comment 28

[jglick]

Will do. Updated spec will appear shortly.

Comment 29

[(not reading, please use [e-mail] instead)]

two comments:

1) +  var focusedElement = top.document.commandDispatcher.focusedElement;

doing this work before you need to.  do this lazily.
and in addition, no need for a var here.  (only used once)

2)  

to simplify, re-use MessageHasAttachments()

Here's how you can address these two points:

+function MessageHasAttachments()
+{
+  var bucketTree = document.getElementById("attachmentBucket");
+  if (bucketTree)
+  {
+    var body = document.getElementById("bucketBody");
+    return (body && body.hasChildNodes() && (bucketTree == 
top.document.commandDispatcher.focusedElement));
+  }
+  return false;
+}
+
+function MessageHasSelectedAttachments()
+{
+  var bucketTree = document.getElementById("attachmentBucket");
+
+  if (bucketTree) {
+    return (MessageHasAttachments() && bucketTree.selectedItems.length);
+  }
+  return false;
+}

Comment 30

[(not reading, please use [e-mail] instead)]

once you address those two comments, sr=sspitzer

Comment 31

[Andreas Premstaller]

Attachment: Patch V0.5, with the 2 changes Seth recommended

Comment 32

[Andreas Premstaller]

Comment on attachment 74220 [details] [diff] [review]
Patch V0.5, with the 2 changes Seth recommended

Sean, do you need to/want to re-review the patch, or is it ok to just transfer
your r= to the latest version? The only changes are the ones proposed by Seth? 

Setting the sr-flag, based on Seth's comment #30.

Comment 33

[Sean Su]

Comment on attachment 74220 [details] [diff] [review]
Patch V0.5, with the 2 changes Seth recommended

transfering r= from previous patch.

let me know if you need help checking into the tree.

Comment 34

[Andreas Premstaller]

Thanks, Sean!
Now seeking a=. Once that has happened, it would be great if you can check the
patch into the tree.

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 74220 [details] [diff] [review]
Patch V0.5, with the 2 changes Seth recommended

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 36

[Sean Su]

patch checked in for [e-mail] to the trunk only.

Comment 37

[Andreas Premstaller]

*** Bug 56339 has been marked as a duplicate of this bug. ***

Comment 38

[Trix Supremo]

verified on trunk 2002042512