282068 - RFE: Ability to collapse attachment pane

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[石庭豐 (Seak, Teng-Fong)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

The attachment pane in the mail window can be very big in a lot of situations:
eg when there're a lot of attachments, attachments whose names is very long,
forwards over forwards in addition to other attachments, etc

It would be nice if this pane could be collapsed, just like the subject pane
where there is a little minus sign.

Reproducible: Always

Steps to Reproduce:

Comment 1

[石庭豐 (Seak, Teng-Fong)]

Attachment: An (artificial) example showing a big attachment

I made up this artificial example because I don't want to show my personal
emails as examples.

Comment 2

[Maxxer]

> It would be nice if this pane could be collapsed, just like the subject pane
> where there is a little minus sign.

It would also be nice to be (small and) scrollable.
It happened to me (using Thunderbird) to receive a mail with LOTS of
attachments, and not being able to access them because the list was longer than
the displayed one. So I could go down with cursor keys, but I can't know
which/what file was selected.

Comment 3

[石庭豐 (Seak, Teng-Fong)]

Hmm, maybe a better solution/compromise would be a resizeable attachment pane,
just like the message pane or mail account pane.

Comment 5

[peter Liepmann]

What I'd like is a toggleable  option *not* to show the attachments pane by default.  

Comment 6

[石庭豐 (Seak, Teng-Fong)]

(In reply to comment #5)
> What I'd like is a toggleable  option *not* to show the attachments pane by
> default.  

One is not in conflict with the other.  But you'd better file another RFE for your idea, instead of commenting in this bug.  Thanks.

Comment 8

[Jim Porter (:squib)]

Taking this bug. I have a work-in-progress patch for it.

Comment 9

[Jim Porter (:squib)]

Attachment: Collapsed view

The attachment pane when it's collapsed.

Comment 10

[Jim Porter (:squib)]

Attachment: Expanded view

The attachment pane after expanding it.

:clarkbw, what do you think of this? There's still some room for improvement, like the "Save all" button could turn into "Save selected" when you expand, but I haven't decided the best way to do this yet.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 496346 [details]
Collapsed view

Looks really nice.  I have some suggestions for the styling of the button and am wondering what the drop down actions are exactly (I'm assuming it's the full list of actions).

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 496347 [details]
Expanded view

Also looks good.

Comment 13

[Jim Porter (:squib)]

(In reply to comment #11)
> Comment on attachment 496346 [details]
> Collapsed view
> 
> Looks really nice.  I have some suggestions for the styling of the button and
> am wondering what the drop down actions are exactly (I'm assuming it's the full
> list of actions).

Right now, it's "Save all", "Detach all", and "Delete all".

One thing I could do is, when the attachment pane is expanded, switch the default action to "Save" and then have the dropdown show "Save", "Open", "Detach", "Delete", and then the various "* All" ones.

Comment 14

[Jim Porter (:squib)]

Here are some bugs that might end up getting fixed in this bug, just for reference:

Bug 285997 - Need quicker way to save some selected attachments
Bug 360647 - Need configurable view for list of attachments
Bug 436555 - attachment presentation in received messages needs improvement
Bug 541336 - Implement / add "Save All Attachments" button in attachments pane
Bug 573230 - Attachments in attachment panel are not focusable
Bug 575879 - attachments area UI badly needed improvements

Comment 15

[Jim Porter (:squib)]

Attachment: Work in progress

Here's a WIP patch that makes things look like the above images. Only Linux theme for now.

Comment 16

[Jim Porter (:squib)]

Attachment: Make attachment list focusable

Still a work in progress. Changes since the last one:
* Cleanup expanding/collapsing code
* Attachment list is now keyboard selectable, responds to Ctrl+A, Ctrl+S, and 
  (Shift+)Delete

Comment 17

[Jim Porter (:squib)]

Setting blocking on keyboard focus bugs fixed by attachment 497640 [details] [diff] [review].

Comment 18

[Jim Porter (:squib)]

:clarkbw, what do you think about doing things like I mentioned in comment 16? This would resolve some of the problems listed in bug 436555 (but not what you mentioned in bug 436555 #46*), as well as the dependent bugs I just added here.

You also mentioned in comment 11 that you had some suggestions for the button styling. Any ideas there?

* I'd prefer staying away from checkboxes since that would make the UI look "heavier", and they'd do pretty much the same thing as having keyboard accessibility, but that's just my opinion. :)

Comment 19

[Jean-Marie COUPRIE]

Coming from
https://bugzilla.mozilla.org/show_bug.cgi?id=466060
 after your comment 37. I had taken previously a look to the present bug but I was not aware that you were working full speed !

Your "Expanded view" attachment please me very much except that I wish to have "detach all" as default button. Is this configurable in prefs ?
Reasons :
-As I have said in bug 466060, I receive frequently mails from my children with pictures as attachments, I detach them to my picture_directory. This means ... smaller (by 3 to 10 Mega Octets each time) mailboxes that is good for the speed of TB.
-As I have said in Bug 327302 - Forward of a message with detached attachment should attach the saved file (Comment 8), there are presently in the mailbox the elements to solve this second problem. OK "save all" + "delete all" is functionally equivalent (may be safer) to "detach all" but one button pressure is better than 2 and I am not sure that you find in the mailbox the elements to solve the problem.
So I think that "detach all"  will be the most common choice.

Comment 20

[Thomas D. (:thomas8)]

Jim, thanks for the great work you are doing. 100% support for your comment 16.

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: quick sketch of a layout

(In reply to comment #18)
> :clarkbw, what do you think about doing things like I mentioned in comment 16?
> This would resolve some of the problems listed in bug 436555 (but not what you
> mentioned in bug 436555 #46*), as well as the dependent bugs I just added here.

This looks good so far.  I'm a bit concerned about the focus issues.

> You also mentioned in comment 11 that you had some suggestions for the button
> styling. Any ideas there?

Attached.  I don't like to use the () around text like size - (138 KB) - if we are in labels and I can get away with the GrayText color instead.

For the button style I wanted to make it a bit smaller so there is some padding between it and the edges.  Also I wanted to make sure the drop down marker looked more like a separate button instead of just an arrow (the reply button should have this as well).  I'm not sure about the right hand side but I wanted to try it and see what it looked like.

> * I'd prefer staying away from checkboxes since that would make the UI look
> "heavier", and they'd do pretty much the same thing as having keyboard
> accessibility, but that's just my opinion. :)

Right, this is what we struggled with in the other bug.  Focus is a precarious way to save data which checkboxes relieve us from.  But with checkboxes they look "heavy" unless they are all at least lined up in a column.

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 498899 [details]
quick sketch of a layout

awesome

Comment 23

[Jim Porter (:squib)]

(In reply to comment #21)
> For the button style I wanted to make it a bit smaller so there is some padding
> between it and the edges.  Also I wanted to make sure the drop down marker
> looked more like a separate button instead of just an arrow (the reply button
> should have this as well).  I'm not sure about the right hand side but I wanted
> to try it and see what it looked like.

It might be possible to piggyback on Firefox's styling for the dropdown buttons they're using; as I recall, it looks similar to your screenshot. I'll have to see where that's defined in the CSS.

> Right, this is what we struggled with in the other bug.  Focus is a precarious
> way to save data which checkboxes relieve us from.  But with checkboxes they
> look "heavy" unless they are all at least lined up in a column.

With my patch, the focus behavior is the same as that in the thread pane. Since we don't have a "Selected" column in the thread pane, I think it's reasonable to do the same in the attachment pane. It also looks/works more like file browsers (Windows Explorer, Gnome Nautilus), which is a plus in my book.

I could see an argument in favor of adding a "Selected" column to both the thread pane and the attachment pane, however. That would just require having a "table" mode for the attachment pane.

The only issue I did see is that keyboard navigation doesn't automatically scroll the focused item into view, but that should be fixable. I guess you can't click and drag to make a selection box either, but I bet we could do that too with minimal issues.

It's possible that part of your concern with focus issues is because the focus was completely disabled in the attachment pane before, which caused weird behavior when trying to focus or do focus-related tasks. With focus enabled, everything works pretty well actually.

Comment 24

[Thomas D. (:thomas8)]

FTR:

- notwithstanding the ingenious goodness and usefulness of this enhancement, as a matter of fact we are also consuming more vertical space (again!), in addition to the desastrous space waste of the default message header without compact header addon. I don't have a better solution for this problem, but we should be aware.
- in windows, both thunderbird buttons and any buttons in the whole os for that sake are flat (no 3d button borders) and they only buttonize (stand out as a real button) when hovering over them. Which makes a much cleaner and less cluttered interface (hence bug 525210 against the wrong button layout in header pane). I don't understand why, as seen in attachment 498899 [details], we keep adding buttons *with permanent (blue) borders* that disregard the default layout and thus add visual clutter to our ui. Can we please flatten those buttons according to OS default.

Comment 25

[Jim Porter (:squib)]

Thomas: actually the attachment panel is 7 pixels shorter on Linux, and I'd expect that the worst-case scenario is on Windows Vista/7, where it should be the same height in the end. It is, of course, taller when expanded, but since people probably aren't trying to read the message and the attachment names at the same time, I think this is ok.

As for the button style, the button will use the same style as the message header buttons. If the message header buttons get a new style one day, it will (or should) automatically get changed in the attachment pane too.

Comment 26

[Jean-Marie COUPRIE]

Please take a look at : "
Bug 621882 After sorting by increasing size, when detaching/suppressing an attachment, the selected message is lost from view and the next one in the list is selected.
" to see if it can be corrected by your work.

Comment 27

[Jim Porter (:squib)]

That bug is totally separate from this one, since this is just about how the UI looks, not how any of the commands behave. I can take a look at it later, though.

Comment 28

[Jean-Marie COUPRIE]

Comment 27 is not so obvious : my bug is on the loss of selection of the previously selected mail and here you are discussing loss of focus in the UI.

Comment 29

[Jim Porter (:squib)]

The selection issues aren't in the attachment pane though. They're in the thread pane, which I'm not touching right now. I'm not sure what the actual solution is, but if I try to fix it, it won't be in this bug.

Comment 30

[Jean-Marie COUPRIE]

  (In reply to comment #29)
> The selection issues aren't in the attachment pane though. They're in the
> thread pane, which I'm not touching right now. I'm not sure what the actual
> solution is, but if I try to fix it, it won't be in this bug.

As long as the bugs are fixed (here or elsewhere), I am happy !
You know the coding, I do not !

Comment 31

[Jean-Marie COUPRIE]

I love the idea to have the number of attachments in the interface : when I
detach all, I can verify in the destination directory (sorted by date) that no
one has disappeared in a black hole !
If, in your example, I suppress 1 attachment and detach 1 other, what will be
shown by the UI ? Still 7 or 5 + 1 +1 ?

Comment 32

[Jim Porter (:squib)]

(In reply to comment #31)
> If, in your example, I suppress 1 attachment and detach 1 other, what will be
> shown by the UI ? Still 7 or 5 + 1 +1 ?

clarkbw, ideas on this?

In other news, one bizarre thing about the attachment pane is that it's a "description" element, but not a regular XUL <description>. There's a CSS selector (description[selectable="true"]) overloading the description and forwarding it on to "extdescription". It seems like it would make more sense to just change the name entirely to something like "columnlistbox", since it derives from listbox-base anyway. There are probably a few other things that could be improved by fixing the XBL bindings, like making scrolling work better. Does anyone know if there's an actual reason for all this, or is it just a weird historical artifact?

Comment 33

[Jim Porter (:squib)]

As mentioned elsewhere, I'm going to remove the keyboard accessibility fixes from this bug, since they'll be getting fixed in another bug. For discussion on this, see the following tb-planning thread: <http://groups.google.com/group/tb-planning/browse_thread/thread/4d66adfe9898432c>. This will help to minimize the feature creep in this bug, and also help improve both attachment panes (reading and composing).

Comment 34

[Jim Porter (:squib)]

Attachment: Updated patch

Ok, I got rid of the focus changes (that's for another bug), cleaned up the CSS (still only for gnomestripe), and added some (but not all) tests.

Stuff left to do:
1) CSS for other themes
2) Consider making the attachment pane buttons customizable
3) Count deleted/detached attachments differently?
4) Test the actual expand/collapse behavior for the pane

:clarkbw, thoughts on (2) and (3)?

Comment 35

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #34)
> Created attachment 505281 [details] [diff] [review]
> Stuff left to do:
> 2) Consider making the attachment pane buttons customizable

I'm assuming you mean adding a toolbar like space for the buttons to sit in so people (likely add-ons) could customize this space.  is that correct?

> 3) Count deleted/detached attachments differently?

You mean something like this:
13 attachments ($size) <strike>(##)</strike>

I think that could work, assuming it wasn't too cluttered looking.

Comment 36

[Jim Porter (:squib)]

(In reply to comment #35)
> (In reply to comment #34)
> > Created attachment 505281 [details] [diff] [review]
> > Stuff left to do:
> > 2) Consider making the attachment pane buttons customizable
> 
> I'm assuming you mean adding a toolbar like space for the buttons to sit in so
> people (likely add-ons) could customize this space.  is that correct?

Yeah. Like the message header toolbar. It might be useful to have the omni-button that's there now, plus some non-default buttons for each of the actions (save all, detach all, delete all). Or a button to save/etc the currently selected attachment.

> > 3) Count deleted/detached attachments differently?
> 
> You mean something like this:
> 13 attachments ($size) <strike>(##)</strike>
> 
> I think that could work, assuming it wasn't too cluttered looking.

Yeah. Maybe one of these layouts would work too:

 13 <strike>1</strike> attachments  /123 KB/
 13 attachments (1 deleted)  /123 KB/

Though the latter is a bit more verbose.

Comment 37

[Jim Porter (:squib)]

It looks like I'll need a 16x16 px save icon on pinstripe. Is there some obscure place this is located, or will we need a new icon? (Maybe the other themes want new icons too).

As for the other things to do (namely toolbar customization and counting of deleted/detached attachments), unless someone has some concrete ideas for what to do, I think I might leave those for a followup bug.

Comment 38

[Mark Banner (:standard8)]

(In reply to comment #37)
> It looks like I'll need a 16x16 px save icon on pinstripe. Is there some
> obscure place this is located, or will we need a new icon? (Maybe the other
> themes want new icons too).

Andreas may be able to help you there.

Comment 39

[Andreas Nilsson (:andreasn)]

We have save icons in mail-toolbar.png graphics, but I think it would be neat with symbolic icons on at least OS X and Win7. Will whip some up!
Under Linux the gtk-save stock item should be of satisfaction.

Comment 40

[Andreas Nilsson (:andreasn)]

Attachment: aero download graphics

Comment 41

[Andreas Nilsson (:andreasn)]

Attachment: pinstripe download icon

Comment 42

[Andreas Nilsson (:andreasn)]

Attachment: graphics for windows and osx as patch (untested)

Needs the previous patch applied in order to work

Comment 43

[Jim Porter (:squib)]

Thanks for the icons, Andreas! I used the mail toolbar save icon for XP and moved the new download.png icon to messageHeader-aero.css, since I think that's what we want. I'll upload a combined patch shortly...

Comment 44

[Jim Porter (:squib)]

Attachment: Add tests and CSS for all themes

This is probably review-ready, though I'm not sure if I got the non-gnomestripe themes right.

Some notable things:
* messageHeader.css had a handful of Allman braces (most of it was K&R) so I got
  rid of those (mixed brace styles bug me :))
* I restructured mozmill/attachment/test-attachment.js, though it does pretty
  much the same thing as before.

Blake, since you reviewed the compose version of this bug (bug 623346), I'm asking you for review, though as always feel free to pass it onto someone else if need be.

Comment 45

[Jim Porter (:squib)]

Attachment: Fix toolbar appearance

Whoops, I forgot to set -moz-appearance: none; on the non-gnomestripe versions of the toolbar.

Comment 46

[Jim Porter (:squib)]

Attachment: Fix toolbar button

Sigh... it would also help if I actually handled the toolbar commands properly. Let's hope this is the last boneheaded mistake in there! :)

Comment 47

[Andreas Nilsson (:andreasn)]

(In reply to comment #43)
> Thanks for the icons, Andreas! I used the mail toolbar save icon for XP and
> moved the new download.png icon to messageHeader-aero.css, since I think that's
> what we want. I'll upload a combined patch shortly...

Yes, agreed!

Comment 48

[Jim Porter (:squib)]

Blake: one more thing that I could do in this patch that might be worthwhile would be to add a toolbox and palette for the "Save all..." toolbar button. I'm probably going to avoid adding more buttons to the palette unless someone has a brilliant idea, but 1) this would make it easier for addon authors to customize this, and 2) people who really need the extra few vertical pixels could hide the toolbar entirely. Let me know what you think.

Comment 49

[Jim Porter (:squib)]

review ping...

Comment 50

[Blake Winton (:bwinton) (:☕️)]

Attachment: The OSX button looks weird and we need space between the word "attachment" and the size.

I'm not sure if that's what you were thinking it would look like, but it doesn't seem quite right to me.  ;)

Comment 51

[Blake Winton (:bwinton) (:☕️)]

Attachment: The expanded box on OS X.

I'm not sure whether it makes sense to have the dropdown if we only have one attachment.  I think I would skip it and just inline the attachment in that case.

But there are a couple of other things which I think are more important.
1) We can only collapse the attachment pane if we've expanded it, which seems counter-intuitive.
2) We really need the same amount of space above the rounded-box as we have below.

(Code review coming up.)

Thanks,
Blake.

Comment 52

[Jim Porter (:squib)]

That is... bizarre. What does the OS X attachment area look like without the patch?

Comment 53

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 511311 [details] [diff] [review]
Fix toolbar button

We should probably make the max size of the attachment pane big enough to show all the attachments, but not bigger.

>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -1813,82 +1813,123 @@ function createAttachmentDisplayName(aAt
> function displayAttachmentsForExpandedView()
>-    // IMPORTANT: make sure we uncollapse the attachment box BEFORE we start adding
>-    // our attachments to the view. Otherwise, layout doesn't calculate the correct height for
>-    // the attachment view and we end up with a box that is too tall.

That seems like a reasonable important comment.  Does it no-longer apply?

>-        attachmentView.setAttribute("largeView", "true");
>+        item.setAttribute("largeView", "true");

Do we need this, if we've set the "largeView" attribute on the attachmentList?
(Yeah, we used to have it, but this seems like a place we could simplify things.)

>+    var words = gMessengerBundle.getString("attachmentCount");
>+    var count = PluralForm.get(currentAttachments.length, words)
>+                          .replace("#1", currentAttachments.length);
>+    document.getElementById("attachmentCount").setAttribute("value", count);
>+    document.getElementById("attachmentSize").setAttribute(
>+      "value", messenger.formatFileSize(totalSize));

I think that we…  Oh, you can't have those as a single description element, because we want to use different styling on them, right?  I guess one of the .properties strings should start or end with a space, then.  I seem to remember BenB doing something similar in bug 549045:
  # LOCALIZATION NOTE(resultSSLCertWeak): \u0020 is just a space
  resultSSLCertWeak=\u0020(Warning: Could not verify server)
and that seems like the right thing to do there.

And that exposed another bug with our RTL support:
>       var nameAndSize = displayName;
>-      if (attachment.size != null)
>+      if (attachment.size != null) {
>         nameAndSize += " ("+messenger.formatFileSize(attachment.size)+")";
>-      var attachmentView = attachmentList.appendItem(nameAndSize);
>+        totalSize += attachment.size;
>+      }
>+      var item = attachmentList.appendItem(nameAndSize);

That really won't do the right thing.  I think we want to add a localizable string for this (e.g. "#1 (#2)").  (The big problem is that the direction of the two descriptions for the summary will be flipped, but the text name of the attachment won't.)

>     gBuildAttachmentsForCurrentMsg = true;
>+ }

I think that needs an extra space of indentation.

>+function toggleAttachmentList(expanded)
>+{
>+  var attachmentList        = document.getElementById('attachmentList');

You don't use attachmentList in this function, so it could be removed..

>+  if(expanded === undefined)

There should be a space after the "if".  (In a couple of places.)

>+++ b/mail/test/mozmill/attachment/test-attachment-size.js
>@@ -226,61 +276,107 @@ function check_attachment_size(expectedS
>+  let formattedSize = sizeNode.getAttribute('value');
>+  let expectedFormattedSize = messenger.formatFileSize(size);
>+  if (formattedSize != expectedFormattedSize)
>+    throw new Error('Formatted attachment size ('+formattedSize+') does not ' +
>+                    'match expected value ('+expectedFormattedSize+')');

I believe we prefer "s to 's in our strings.

> function test_text_attachment() {
>+  help_test_attachment_size(0);
> }
> 
> function test_binary_attachment() {
>+  help_test_attachment_size(1);
> }
> 
> function test_image_attachment() {
>+  help_test_attachment_size(2);
> }
> 
> function test_detached_attachment() {
>+  help_test_attachment_size(3);
> }
> 
> function test_detached_attachment_with_no_external_file() {
>+  help_test_attachment_size(4);
> }
> 
> function test_deleted_attachment() {
>+  help_test_attachment_size(5);
>+}
> 
>+function test_multiple_attachments() {
>+  help_test_attachment_size(6);
> }
>+
>+function test_multiple_attachments_one_detached() {
>+  help_test_attachment_size(7);
>+}
>+
>+function test_multiple_attachments_one_detached_with_no_external_file() {
>+  help_test_attachment_size(8);
>+}
>+
>+function test_multiple_attachments_one_deleted() {
>+  help_test_attachment_size(9);

So, these are small enough that I wonder if we should just loop from 0 to 9, and call "help_test_attachment_size" on each number.  I'm not sure if there's a way for a test to pass or fail multiple times, or if we can create the functions dynamically, but I think it would be worth spending a little time on that…

>+++ b/mail/test/mozmill/attachment/test-attachment.js
>@@ -16,16 +16,17 @@
>+ *   Jim Porter <[e-mail]>

Did you want to use this email address here, or [e-mail]?
(I lost my university email address a while ago, and so lost links to a bunch of things I did, unless you know where I went to school.)

>@@ -39,195 +40,179 @@
> var elib = {};
>-Components.utils.import('resource://mozmill/modules/elementslib.js', elib);
>-var EventUtils = {};
>-Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);

You're still using EventUtils down below.  Does it get imported by someone else?

>+  // create some messages that have various types of attachments
>+  messages = [
[snip…]
>+    ];

I think this close bracket should be outdented by two spaces.

>+++ b/mail/themes/gnomestripe/mail/messageHeader.css
>@@ -103,90 +103,104 @@ html|div#expandedHeadersTopBox {
>-description[selectable="true"]:focus > descriptionitem[selected="true"][current="true"] 
>-{
>+description[selectable="true"]:focus > descriptionitem[current="true"]  {

Did you mean to get rid of the check for selected="true" here?

So, I mostly like this patch, but I think the changes I've asked for are large enough to require a re-review, so I'm going to give this version of the patch an r-.

Thanks,
Blake.

Comment 54

[Jim Porter (:squib)]

(In reply to comment #53)
> Comment on attachment 511311 [details] [diff] [review]
> Fix toolbar button
> 
> We should probably make the max size of the attachment pane big enough to show
> all the attachments, but not bigger.

This should be what's happening (it caps at 1/4 the height of the message pane, though). If it doesn't, that's a bug, but it's works for me.

> >+++ b/mail/base/content/msgHdrViewOverlay.js
> >@@ -1813,82 +1813,123 @@ function createAttachmentDisplayName(aAt
> > function displayAttachmentsForExpandedView()
> >-    // IMPORTANT: make sure we uncollapse the attachment box BEFORE we start adding
> >-    // our attachments to the view. Otherwise, layout doesn't calculate the correct height for
> >-    // the attachment view and we end up with a box that is too tall.
> 
> That seems like a reasonable important comment.  Does it no-longer apply?

Correct. I changed what gets expanded and collapsed, and now the expansion of the actual attachment list (which is what that comment was really referring to), happens in toggleAttachmentList(). That function has a similar edge case, though, which I commented:

  // Switch overflow off so that when we expand again we can get the
  // preferred size. (Doing this when expanding hits a race condition.)

> >-        attachmentView.setAttribute("largeView", "true");
> >+        item.setAttribute("largeView", "true");
> 
> Do we need this, if we've set the "largeView" attribute on the attachmentList?
> (Yeah, we used to have it, but this seems like a place we could simplify
> things.)

I think we can get rid of that, but I'll have to double-check.

> >+    var words = gMessengerBundle.getString("attachmentCount");
> >+    var count = PluralForm.get(currentAttachments.length, words)
> >+                          .replace("#1", currentAttachments.length);
> >+    document.getElementById("attachmentCount").setAttribute("value", count);
> >+    document.getElementById("attachmentSize").setAttribute(
> >+      "value", messenger.formatFileSize(totalSize));
> 
> I think that we…  Oh, you can't have those as a single description element,
> because we want to use different styling on them, right?  I guess one of the
> .properties strings should start or end with a space, then.  I seem to remember
> BenB doing something similar in bug 549045:
>   # LOCALIZATION NOTE(resultSSLCertWeak): \u0020 is just a space
>   resultSSLCertWeak=\u0020(Warning: Could not verify server)
> and that seems like the right thing to do there.

I was relying on the margin/padding of the description elements to put an appropriate amount of space there. It looks like OS X doesn't have that, but I could pretty easily add some CSS to do it. It looks nice on Linux, though. :)

We probably want to use margin/padding or spaces, but not both. I lean towards the former.

> And that exposed another bug with our RTL support:
> >       var nameAndSize = displayName;
> >-      if (attachment.size != null)
> >+      if (attachment.size != null) {
> >         nameAndSize += " ("+messenger.formatFileSize(attachment.size)+")";
> >-      var attachmentView = attachmentList.appendItem(nameAndSize);
> >+        totalSize += attachment.size;
> >+      }
> >+      var item = attachmentList.appendItem(nameAndSize);
> 
> That really won't do the right thing.  I think we want to add a localizable
> string for this (e.g. "#1 (#2)").  (The big problem is that the direction of
> the two descriptions for the summary will be flipped, but the text name of the
> attachment won't.)

I was hoping I wouldn't have to deal with this yet, but yeah, it's a problem. (Attachment sizes certainly aren't the only place this is a problem either; the new message count in the folder pane has this issue as well). Maybe this would be best as a followup bug; at the very least, I need to fix the message reader and composer, and while I'm there I may as well do the same for the folder pane.

> >     gBuildAttachmentsForCurrentMsg = true;
> >+ }
> 
> I think that needs an extra space of indentation.

Whoops, fixed.

> >+function toggleAttachmentList(expanded)
> >+{
> >+  var attachmentList        = document.getElementById('attachmentList');
> 
> You don't use attachmentList in this function, so it could be removed..

Removed.

> >+  if(expanded === undefined)
> 
> There should be a space after the "if".  (In a couple of places.)

Also fixed.

> >+++ b/mail/test/mozmill/attachment/test-attachment-size.js
> >@@ -226,61 +276,107 @@ function check_attachment_size(expectedS
> >+  let formattedSize = sizeNode.getAttribute('value');
> >+  let expectedFormattedSize = messenger.formatFileSize(size);
> >+  if (formattedSize != expectedFormattedSize)
> >+    throw new Error('Formatted attachment size ('+formattedSize+') does not ' +
> >+                    'match expected value ('+expectedFormattedSize+')');
> 
> I believe we prefer "s to 's in our strings.

I was sticking to the dominant quote style in the file, though I could change it to double quotes if you prefer.

> > function test_text_attachment() {
> >+  help_test_attachment_size(0);
> > }
[snip]
> >+
> >+function test_multiple_attachments_one_deleted() {
> >+  help_test_attachment_size(9);
> 
> So, these are small enough that I wonder if we should just loop from 0 to 9,
> and call "help_test_attachment_size" on each number.  I'm not sure if there's a
> way for a test to pass or fail multiple times, or if we can create the
> functions dynamically, but I think it would be worth spending a little time on
> that…

I had organized it like this so that if one test fails, it's obvious based on the name which one failed (and also so that if the first test fails, the others still get run). Maybe there's a way to create a function that takes an array of names and spits out global functions. I'll work on that a bit.

> >+++ b/mail/test/mozmill/attachment/test-attachment.js
> >@@ -16,16 +16,17 @@
> >+ *   Jim Porter <[e-mail]>
> 
> Did you want to use this email address here, or [e-mail]?
> (I lost my university email address a while ago, and so lost links to a bunch
> of things I did, unless you know where I went to school.)

I've never quite managed to decided whether I should use my bugmail address in comments or not. I guess I'll just go with the bugmail address for consistency.

> >@@ -39,195 +40,179 @@
> > var elib = {};
> >-Components.utils.import('resource://mozmill/modules/elementslib.js', elib);
> >-var EventUtils = {};
> >-Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);
> 
> You're still using EventUtils down below.  Does it get imported by someone
> else?

Probably, since it works. I'll add it back in though.

> >+  // create some messages that have various types of attachments
> >+  messages = [
> [snip…]
> >+    ];
> 
> I think this close bracket should be outdented by two spaces.

Fixed in both test-attachment.js and test-attachment-size.js.

> >+++ b/mail/themes/gnomestripe/mail/messageHeader.css
> >@@ -103,90 +103,104 @@ html|div#expandedHeadersTopBox {
> >-description[selectable="true"]:focus > descriptionitem[selected="true"][current="true"] 
> >-{
> >+description[selectable="true"]:focus > descriptionitem[current="true"]  {
> 
> Did you mean to get rid of the check for selected="true" here?

Yes, since an unselected but current item should have a border around it for keyboard accessibility. This doesn't actually matter though, since the description[selectable="true"] element isn't keyboard focusable. :( This is an artifact from when I was going to handle keyboard accessibility in this bug, which I gave up on pretty quickly.

I could undo the change, since it doesn't actually affect anything, but it *is* more correct than the old way. I guess if I kept it, I'd want to update it in the other themes though! :)

Thanks for the review!

Comment 55

[Jim Porter (:squib)]

Attachment: Updated attachment header look

This is what the attachment header looks like now. Notable changes since attachment 496346 [details]:

* Save button is on the right
* Save button is all lowercase like the message header buttons
* Not shown, but the save button has a tooltip (also like the message header
  buttons)
* Special case for only one attachment (button and label)
* Added a paperclip icon in an effort to fix bug 635884

:clarkbw, what do you think about this?

I'll upload the patch tomorrow to give me some time to discover the inevitable mistakes I made (especially on non-Linux). :)

Comment 56

[Andreas Nilsson (:andreasn)]

(In reply to comment #55) 
> I'll upload the patch tomorrow to give me some time to discover the inevitable
> mistakes I made (especially on non-Linux). :)

I like it!
What color did you use for the background? Just want to make sure it works on several themes.

Comment 57

[Marco]

Hey everyone!

I'm new here so please forgive if this comment is inappropriate... Just want to help !
Now I see you are working on this, please have a look at my "bug/request", also about attachment handling : https://bugzilla.mozilla.org/show_bug.cgi?id=637970
Maybe it might be solved/implemented together without too much work :)

Best regards, Marco

Comment 58

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 516546 [details]
Updated attachment header look

looks really good!

Comment 59

[Jim Porter (:squib)]

(In reply to comment #56)
> (In reply to comment #55) 
> > I'll upload the patch tomorrow to give me some time to discover the inevitable
> > mistakes I made (especially on non-Linux). :)
> 
> I like it!
> What color did you use for the background? Just want to make sure it works on
> several themes.

I don't think I explicitly specified a color, so usually it will work out to be -moz-Dialog. I'm not too sure about what exact color it is on OS X, but it's some kind of gray anyway (see attachment 516318 [details] for the exact color, though that screenshot doesn't have the latest changes in it). I think it'll look ok on OS X since as I recall, the attachment icon there is pretty dark.

Comment 60

[Jim Porter (:squib)]

(In reply to comment #57)
> Hey everyone!
> 
> I'm new here so please forgive if this comment is inappropriate... Just want to
> help !
> Now I see you are working on this, please have a look at my "bug/request", also
> about attachment handling : https://bugzilla.mozilla.org/show_bug.cgi?id=637970
> Maybe it might be solved/implemented together without too much work :)
> 
> Best regards, Marco

That's actually going to be at least moderately hard to do, and definitely out of scope of this bug, but I'll comment more on the details in that bug (later, though).

Comment 61

[Marco]

Hey Jim,

No problem, we'll see what happens later at that one there! :)

Some other thing I discovered lately with the attachment pane, which might be interesting to look at here is the following :
When someone deletes one attachment from an e-mail with multiple attachments, the "rightclick in free space"-menu does not offer "save all", "delete all" or "detach all" anymore, while in the main menu "file > attachments" the option "save all" still works for the remaining attachments in the message...

Is it better to post this a new bug, or just leave it here as is ?

Best regards, Marco

Comment 62

[Andreas Nilsson (:andreasn)]

Comment on attachment 516546 [details]
Updated attachment header look

Agree with clarkbw. Good job!

Comment 63

[Jim Porter (:squib)]

Attachment: Address review comments

Ok, here's what's changed:

* Save all button is now on a proper toolbar (I did this mostly to save me the
  headache of replicating the OS X message header toolbar's style, but it's a
  generally useful thing anyway)
* Refactored the message header toolbar code/CSS to be a bit more generic
  (Lightning will benefit from this)
* Save all button is now lowercase to match the header (and also has a tooltip
  for the same reason)
* Save all button is specialized for the single-attachment case
* The menu on the save all button properly enables/disables the menu items
* Messages with one attachment show the filename of the attachment in the
  header
* Added a paperclip icon to the attachment header so that it's more noticeable
* Miscellaneous style fixes
* Test code is a bit more compact, thanks to some JS trickery

There are quite a bit of CSS changes in this patch, though they're pretty simple (mostly it's just s/#header-view/.inline/g).

Comment 64

[Jim Porter (:squib)]

Attachment: Interdiff of the above patch and the previous version

Here's an interdiff of the patches to make reviewing easier. About half of it is CSS/locale changes.

Comment 65

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 516718 [details] [diff] [review]
Address review comments

(In reply to comment #54)
> > We should probably make the max size of the attachment pane big
> > enough to show all the attachments, but not bigger.
> This should be what's happening (it caps at 1/4 the height of the
> message pane, though). If it doesn't, that's a bug, but it's works for
> me.

Bugged for me.  Screenshot at http://dl.dropbox.com/u/2301433/Screenshots/TooBig.png

Also, while checking it, I think we need the same amount of space above
the rounded box as below.  It's not too bad in that screenshot, but it
looks really weird at http://dl.dropbox.com/u/2301433/Screenshots/NoSpace.png

> > >-        attachmentView.setAttribute("largeView", "true");
> > >+        item.setAttribute("largeView", "true");
> > Do we need this, if we've set the "largeView" attribute on the
> > attachmentList?  (Yeah, we used to have it, but this seems like a
> > place we could simplify things.)
> I think we can get rid of that, but I'll have to double-check.

Did you double-check, and do we still need it?

> >   # LOCALIZATION NOTE(resultSSLCertWeak): \u0020 is just a space
> >   resultSSLCertWeak=\u0020(Warning: Could not verify server)
> > and that seems like the right thing to do there.
> I was relying on the margin/padding of the description elements to put
> an appropriate amount of space there. It looks like OS X doesn't have
> that, but I could pretty easily add some CSS to do it. It looks nice
> on Linux, though. :)
> 
> We probably want to use margin/padding or spaces, but not both. I lean
> towards the former.

Sounds good.

> > That really won't do the right thing.  I think we want to add a
> > localizable string for this (e.g. "#1 (#2)").  (The big problem is
> > that the direction of the two descriptions for the summary will be
> > flipped, but the text name of the attachment won't.)
> I was hoping I wouldn't have to deal with this yet, but yeah, it's a
> problem.  (Attachment sizes certainly aren't the only place this is a
> problem either; the new message count in the folder pane has this
> issue as well). Maybe this would be best as a followup bug; at the
> very least, I need to fix the message reader and composer, and while
> I'm there I may as well do the same for the folder pane.

I'm okay with this as a followup bug.

> > >+++ b/mail/test/mozmill/attachment/test-attachment-size.js
> > I believe we prefer "s to 's in our strings.
> I was sticking to the dominant quote style in the file, though I could
> change it to double quotes if you prefer.

Sticking to the dominant style trumps my preferences.  :)

> I had organized it like this so that if one test fails, it's obvious based on
> the name which one failed (and also so that if the first test fails, the others
> still get run). Maybe there's a way to create a function that takes an array of
> names and spits out global functions. I'll work on that a bit.

I see that you did that, and it looks pretty good.  My only remaining
suggestion is to put the list of names in with the list of messages,
something like:
  messages = [
    // text attachment
    {name: "text_attachment",
     attachments: [{body: textAttachment,
                    filename: 'ubik.txt',
                    format: ''}],
     attachmentSizes: [textAttachment.length],
    },
    {name: "binary_attachment",
     attachments: [{body: binaryAttachment,
                    contentType: 'application/x-ubik',
    …
So that when you add stuff to or re-order the list of messages, you
automatically get the tests re-ordered.

> > >+++ b/mail/themes/gnomestripe/mail/messageHeader.css
> > >@@ -103,90 +103,104 @@ html|div#expandedHeadersTopBox {
> > >-description[selectable="true"]:focus > descriptionitem[selected="true"][current="true"] 
> > >-{
> > >+description[selectable="true"]:focus > descriptionitem[current="true"]  {
> > Did you mean to get rid of the check for selected="true" here?
> Yes, since an unselected but current item should have a border around
> it for keyboard accessibility. This doesn't actually matter though,
> since the description[selectable="true"] element isn't keyboard
> focusable. :( This is an artifact from when I was going to handle
> keyboard accessibility in this bug, which I gave up on pretty quickly.
> 
> I could undo the change, since it doesn't actually affect anything,
> but it *is* more correct than the old way. I guess if I kept it, I'd
> want to update it in the other themes though! :)

Sounds good.

So, the stuff I've asked for is pretty minor, I think, so I'm going to
say r=me for this, if you fix the nits.

Later,
Blake.

Comment 66

[Jim Porter (:squib)]

(In reply to comment #65)
> Comment on attachment 516718 [details] [diff] [review]
> Address review comments
> 
> (In reply to comment #54)
> > > We should probably make the max size of the attachment pane big
> > > enough to show all the attachments, but not bigger.
> > This should be what's happening (it caps at 1/4 the height of the
> > message pane, though). If it doesn't, that's a bug, but it's works for
> > me.
> 
> Bugged for me.  Screenshot at
> http://dl.dropbox.com/u/2301433/Screenshots/TooBig.png

Is this what it looks like immediately after you click the twisty to expand the attachment list, or did you drag the splitter to make it big? I don't have a maximum size on the attachment pane, though I could easily set the maximum size to be exactly the amount you'd need to have no scrollbar.

> Also, while checking it, I think we need the same amount of space above
> the rounded box as below.  It's not too bad in that screenshot, but it
> looks really weird at http://dl.dropbox.com/u/2301433/Screenshots/NoSpace.png

I changed the margin on pinstripe to be 4px on all sizes (it used to be 6px on left, right, and bottom and 0px on top), which I figure is a decent compromise so that it 1) looks similar to the old way, and 2) doesn't take up too much extra space.

> > > >-        attachmentView.setAttribute("largeView", "true");
> > > >+        item.setAttribute("largeView", "true");
> > > Do we need this, if we've set the "largeView" attribute on the
> > > attachmentList?  (Yeah, we used to have it, but this seems like a
> > > place we could simplify things.)
> > I think we can get rid of that, but I'll have to double-check.
> 
> Did you double-check, and do we still need it?

We don't need it, so I've removed it.
 
> > > That really won't do the right thing.  I think we want to add a
> > > localizable string for this (e.g. "#1 (#2)").  (The big problem is
> > > that the direction of the two descriptions for the summary will be
> > > flipped, but the text name of the attachment won't.)
> > [snip] 
> I'm okay with this as a followup bug.

That's bug 643663.

> I see that you did that, and it looks pretty good.  My only remaining
> suggestion is to put the list of names in with the list of messages,
> something like:
>   messages = [
>     // text attachment
>     {name: "text_attachment",
>      attachments: [{body: textAttachment,
>                     filename: 'ubik.txt',
>                     format: ''}],
>      attachmentSizes: [textAttachment.length],
>     },
>     {name: "binary_attachment",
>      attachments: [{body: binaryAttachment,
>                     contentType: 'application/x-ubik',
>     …
> So that when you add stuff to or re-order the list of messages, you
> automatically get the tests re-ordered.

Done.

Thanks again for looking at all this. I'll upload a new version of the patch once I know what you think I should do with the attachment pane height issue.

Comment 67

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #66)
> > > > We should probably make the max size of the attachment pane big
> > > > enough to show all the attachments, but not bigger.
> > > This should be what's happening (it caps at 1/4 the height of the
> > > message pane, though). If it doesn't, that's a bug, but it's works for
> > > me.
> > Bugged for me.  Screenshot at
> > http://dl.dropbox.com/u/2301433/Screenshots/TooBig.png
> Is this what it looks like immediately after you click the twisty to expand the
> attachment list, or did you drag the splitter to make it big? I don't have a
> maximum size on the attachment pane, though I could easily set the maximum size
> to be exactly the amount you'd need to have no scrollbar.

I had to drag it, but I don't think we should be allowed to drag it that big, so setting the maximum size seems like the right thing to do to me.

> Thanks again for looking at all this. I'll upload a new version of the patch
> once I know what you think I should do with the attachment pane height issue.

I'm looking forward to it.  :)

Thanks,
Blake.

Comment 68

[Jim Porter (:squib)]

Attachment: Address more review comments

Ok, this patch fixes the last few issues from review, plus one more: the attachment label will be cropped if it doesn't fit. Before, it would push the save button off the screen. Since I used a somewhat hacky method of doing this, I'm flagging this for review again, just in case there's actually a better way to handle it.

Comment 69

[Jim Porter (:squib)]

Blake, aside from changes to the test code, this is the only significant part of the interdiff (the hack I mentioned in comment 68): https://bugzilla.mozilla.org/attachment.cgi?oldid=516718&action=interdiff&newid=521080&headers=1#a/mail/base/content/msgHdrViewOverlay.xul_sec1

You probably don't need to go over the rest of the patch, since the only other changes are the minor things you mentioned in comment 65.

Comment 70

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 521080 [details] [diff] [review]
Address more review comments

I'm surprised that your hacky solution does something different than the previous code, but I can't think of a better way to do it, so r=me, I guess.  :)  (If you wanted to ping some people on IRC, and see if they know of a better way, I would be up for that, too.)

Thanks,
Blake.

Comment 71

[Jim Porter (:squib)]

I asked on m.d.t.xul and there doesn't appear to be a better way to do this, so I'll just check this in now.

Comment 72

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/7c7f54344b22

Comment 73

[Joe Sabash [:JoeS1]]

Attachment: expand marker

The expansion widget appears as a rectangle here.
Win2k
Mozilla/5.0 (Windows NT 5.0; rv:2.0pre) Gecko/20110330 Thunderbird/3.3a4pre ID:20110330030014

Do you want a follow-up bug

Comment 74

[Jim Porter (:squib)]

(In reply to comment #73)
> Do you want a follow-up bug

Already filed with a patch awaiting review: bug 646032

Comment 75

[Thomas D. (:thomas8)]

Jim, I just realized that "open all" (for mails with multiple attachments) is absent from the contextual menu of the new button on the attachment header, while we do have "open all" in file > attachments, and it works like a charm (at least with apps specified, which is likely when user wants to open them all...).

Use cases are obvious: have all (image/document/pdf etc.) attachments opened with the convenience of a single click, and then do something with them one by one (view/print/save/act upon content etc.).

I suppose this was just forgotten?
Should be fixed with an add-on patch here or in a new bug.

Comment 77

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #75)
> Jim, I just realized that "open all" (for mails with multiple attachments)
> is absent from the contextual menu of the new button on the attachment
> header, while we do have "open all" in file > attachments, and it works...

FTR: problem of comment 75 has been fixed (wfm on 8.0a2 aurora)