The new landed feature appears broken as show in attached screenshot Mozilla/5.0 (Windows NT 6.1; rv:2.0pre) Gecko/20110329 Thunderbird/3.3a4pre In safe-mode behaves the same.

Hm, it looks like -moz-appearance:treetwisty was never implemented on Windows: bug 115746. I guess we'll just need images for Windows. Relatedly, there are a couple theme issues I see on XP: * There's a border on the bottom of the toolbars (message header and attachment area) * The attachment area's background isn't quite the same color as the header Those should be easy, though.

It looks like Windows don't support -moz-appearance: treetwisty and -moz-appearance: treetwistyopen. A quick hack with list-style-image: url("chrome://global/skin/tree/twisty-clsd.png"); and list-style-image: url("chrome://global/skin/tree/twisty-open.png"); shows the image. Without button appearance the twistys would look lost. I think this needs some artwork for Windows and the button appearance can be switched off.

Andreas I add you to this Bug for a feedback what you think about the appearance and eventually a needed artwork.

Something like this should work, and would get rid of the button border: #attachmentToggle { -moz-appearance: none; min-width: 0; margin: 0; border: 0; background-color: transparent; list-style-image: url("chrome://global/skin/tree/twisty-clsd.png"); }

(In reply to comment #2) > Without button appearance the twistys would look lost. I think this needs some > artwork for Windows and the button appearance can be switched off. The styling in comment 4 is what got ui-review originally, and it's what Mac and Linux look like, so I don't think a button (or a new image) is really necessary. See attachment 516546 [details].

(In reply to comment #5) > (In reply to comment #2) > > Without button appearance the twistys would look lost. I think this needs some > > artwork for Windows and the button appearance can be switched off. > > The styling in comment 4 is what got ui-review originally, and it's what Mac > and Linux look like, so I don't think a button (or a new image) is really > necessary. See attachment 516546 [details]. If the twisty dimensions are okay, I have no problem with this.

Comment on attachment 522752 [details] Twisty with styling from comment 4 Just to be on the safe side... clarkbw, does this look ok for Windows 7? (Windows XP will be pretty similar too, with a + or - icon.)

Comment on attachment 522752 [details] Twisty with styling from comment 4 The twisty looks fine. Though the icon looks very close the "1 attachm..." text.

I added a 2px margin after the paperclip to make things look a bit less crammed together. I'm also not sure which color scheme looks best in XP: 1) match the find bar and the main toolbar (usually shows a gradient background), or 2) match the message header (a solid color background). The attached patch shows how things look in Linux (just the margin change) and Windows XP (margin change + twisty fix + optional background color change).

This patch fixes margins for all themes (2px after the paperclip), fixes the twisty for Windows, and sets the background of the attachment pane to match the message header for Windows.

This patch is as above, sans the background change.

Comment on attachment 522874 [details] The attachment header under Windows XP and Linux 2px is perfect, thanks! I'd go with 1) for the color.

(Removing Linux background-color override too, since it's not needed, and this way is more consistent with the find bar.) :bwinton, since you reviewed the parent bug for this, do you mind taking a look at it? Hopefully, this will be the last time I make you suffer through bug 282068 or one of its derivatives. :) :paenglab, you seem to be pretty aware of Vista/7 theme stuff, so a quick sanity check on the Aero portion of this patch would be much appreciated.

Comment on attachment 522885 [details] [diff] [review] Patch without Windows/Linux background change This looks good under Win7, also the bottom border from inline-toolbar is gone. Only one question, why have you made the space here? + background-color: transparent; + + list-style-image: url("chrome://global/skin/tree/twisty-clsd.png");

That's just to make it obvious that the last line isn't a part of the button style overrides.

This should block Thunderbird 3.3.

Comment on attachment 522885 [details] [diff] [review] Patch without Windows/Linux background change Okay, so I like this patch, but… I really feel that we shouldn't keep the little dots around the twisty after we've clicked it, which I think falls under the domain of this patch. And I kind of feel like we should be able to click anywhere on the attachment bar to open and close the details, but that seems like it could be a new bug, and probably one that Bryan has an opinion on. So, for now, r=me if you remove the dots. ;) Thanks, Blake.

seems fine to me except I thought we were going to make the single attachment name clickable; which shouldn't impeded this change. I just want to be cautious that area immediately around a single attachment name isn't the click to expand option. i.e. slight miss clicks intended to open don't expand Other than that I'd ask for a slight onHover background color indication. Maybe just kick the opacity down. So people are more aware of the function possibility.

(In reply to comment #18) > I really feel that we shouldn't keep the little dots around the twisty after > we've clicked it, which I think falls under the domain of this patch. Is this talking about the focus ring around the twisty? (In reply to comment #19) > seems fine to me except I thought we were going to make the single attachment > name clickable; which shouldn't impeded this change. I just want to be > cautious that area immediately around a single attachment name isn't the click > to expand option. i.e. slight miss clicks intended to open don't expand Do you envision a "null" area where a click does nothing, or just making the single attachment name have some clickable padding around it? > Other than that I'd ask for a slight onHover background color indication. > Maybe just kick the opacity down. So people are more aware of the function > possibility. Is this for the whole attachment pane or just the single attachment name? I don't think I'd like that for the whole pane, since it would be visually disruptive if I were just moving my mouse to click on the "save" button, for instance. I'm ok with the "click on the attachment pane" not having any visuals, since the twisty is the obvious main target. I see it as the same sort of "hidden feature" as middle-clicking on a tab to close it. Another thing I plan on doing is adding a hotkey to focus/expand the attachment pane, but that needs to wait until the actual attachment list is focusable.

Hmm, testing this on my Windows build (ok, I just monkeypatched the CSS), I don't see a focus ring around the twisty at all...

(In reply to comment #21) > Hmm, testing this on my Windows build (ok, I just monkeypatched the CSS), I > don't see a focus ring around the twisty at all... Weird. I really do. Uh, screenshots available, but I don't know how much they'ld help. I wonder if making it look more like a button in an :hover style would help? Later, Blake.

The focus ring can be removed with: #attachmentToggle > .button-box { border: none; }

I really have no idea if this works, since I couldn't get the focus ring to show up, but I'll take paenglab's word for it. Flagging for review just in case, though. :)

Comment on attachment 525955 [details] [diff] [review] Fix focus ring around twisty (I assume) WFM! One final thing I might add is to highlight the twistys in blue on hover, like in the account list. (Screenshots available upon request.) But I'm also happy if you want to do that in a separate patch, or a new bug. Thanks, Blake.

Ah ha. That's an Aero theme thing (I'm working on XP here). I also noticed that for Aero, there's an RTL version of the twisty icons, so I'll need to make sure to account for that in the next patch.

This patch adds a hover state in Aero and RTL versions of all the Aero states (XP doesn't care about this since its twisty is symmetric, and Linux/Mac pull the image in from the OS). Incidentally, now your comments about the :hover state make more sense, since I didn't realize Aero had a separate image for that. Also, while testing RTL on XP, I noticed that with the gradient background for the attachment pane, the attachment toolbar's background clashed, so I made it transparent. Technically that's not part of this bug, but it's such a simple fix that I figure I can let it stow away in this patch. ;)

To be applied on top of attachment 526460 [details] [diff] [review]. This patch makes the attachment name clickable to open (with a style similar to email addresses in the message header), as well as making empty regions of the attachment bar clickable to toggle expansion. The attachment name can also be dragged and dropped to copy the file around. I had to do a bit of restructuring with the context menu and the attachment DnD observer, but the changes are pretty simple. Tests are included, though nothing for drag-and-drop, since that's a bit difficult to test (though, I think, possible).

Comment on attachment 526460 [details] [diff] [review] Add :hover and RTL states for Aero, fix transparency for XP [checked in] Review of attachment 526460 [details] [diff] [review]: The padding on the twisty in the RTL case seems to be wrong. http://dl.dropbox.com/u/2301433/Attachment/Padding.png Other than that, it seems like an improvement, so r=me with that fixed. Thanks, Blake.

Comment on attachment 528264 [details] [diff] [review] Make attachment name clickable Review of attachment 528264 [details] [diff] [review]: I think we need to make the highlight a little smaller: http://dl.dropbox.com/u/2301433/Attachment/Single.png And since there's nothing we can do to the expanded attachment that we couldn't do right here, I suggest we remove the twisty, and disable expanding the box entirely in this case. (The code looks mostly decent, but I'm going to hold off on the full review until after those changes are made.) Thanks, Blake. ::: mail/base/content/msgHdrViewOverlay.js @@ +1743,5 @@ + // the first (and only) attachment as our "selected" attachments. + if (event.explicitOriginalTarget.id == "attachmentName") + var selectedAttachments = [attachmentList.getItemAtIndex(0).attachment]; + else + var selectedAttachments = [i.attachment for each(i in attachmentList.selectedItems)]; I think having two "var selectedAttachments" here is a little confusing. Could you split it out into: var selectedAttachments = [attachmentList.getItemAtIndex(0).attachment]; if (event.explicitOriginalTarget.id != "attachmentName") selectedAttachments = [i.attachment for each(i in attachmentList.selectedItems)]; instead?

(In reply to comment #30) > And since there's nothing we can do to the expanded attachment that we couldn't > do right here, I suggest we remove the twisty, and disable expanding the box > entirely in this case. Bad idea, users may want to drag-and-drop the attachment to another message or the desktop or a folder, which I don't think you can do with the link...

You can! I tested it on Windows 7. :)

Ok, I didn't test it, just was thinking loud when I saw that. Just hiding the twisty should do the job (pending any further improvement in bug 647036), though I don't see any harm done allowing the user to open the pane anyway.

(In reply to comment #29) > Comment on attachment 526460 [details] [diff] [review] > Add :hover and RTL states for Aero, fix transparency for XP > > Review of attachment 526460 [details] [diff] [review]: > > The padding on the twisty in the RTL case seems to be wrong. > http://dl.dropbox.com/u/2301433/Attachment/Padding.png > > Other than that, it seems like an improvement, so r=me with that fixed. What's going on with that vertical line running through the scrollbar? That seems like a problem. If it weren't for that line, the padding would look about the same as it does in LTR to me.

This is the folderpane_splitter. I'll open a Bug for this.

(In reply to comment #30) > I think we need to make the highlight a little smaller: > http://dl.dropbox.com/u/2301433/Attachment/Single.png 1px less padding on all sides? I intentionally made that match the mail addresses, but a little smaller might be ok. > And since there's nothing we can do to the expanded attachment that we couldn't > do right here, I suggest we remove the twisty, and disable expanding the box > entirely in this case. I disagree on this. First, it's more complexity, especially if we do something to allow the pane to be initially expanded. Second, I think it would help eliminate confusion to keep the twisty around all the time. Third, technically there is one thing you can't do in the collapsed mode: see the icon (I intentionally left the icon out of the collapsed bar, since I think it's already on the edge of being too busy). Finally, and this is the main reason, it would hurt my attempts to make the attachment bar more keyboard accessible. My plan with bug 630759 is to allow a user to focus and expand the attachment list by pressing Alt-C and then (say) saving the attachment by pressing Ctrl-S. I could make the focus move to the attachment name in the summary bar instead, but I think that's even more confusing than a hidden twisty.

Opened Bug 652939 for the folderpane_splitter issue.

(In reply to comment #36) > (In reply to comment #30) > > I think we need to make the highlight a little smaller: > > http://dl.dropbox.com/u/2301433/Attachment/Single.png > 1px less padding on all sides? I intentionally made that match the mail > addresses, but a little smaller might be ok. My real concern there is that it's butting up against the ":" on the left-hand-side and making it look weird. If you want to fix that some other way, I would be up for that. > > And since there's nothing we can do to the expanded attachment that we couldn't > > do right here, I suggest we remove the twisty, and disable expanding the box > > entirely in this case. > I disagree on this. First, it's more complexity, especially if we do something > to allow the pane to be initially expanded. That's a fairly big if. If we handle the case of one-to-three-items-that-fit-on-a-line a little better, I think we won't need to auto-expand the pane. > Second, I think it would help eliminate confusion to keep the twisty around all the time. I think of the twisty like a scrollbar. People are quite used to scrollbars only appearing when there's too much content to fit, and so I don't think they'll have too much of a problem with this only appearing when there's too much content to fit. > Third, technically > there is one thing you can't do in the collapsed mode: see the icon (I > intentionally left the icon out of the collapsed bar, since I think it's > already on the edge of being too busy). Hmm. This is a good point. Let me think about it a bit. > Finally, and this is the main reason, it would hurt my attempts to make the > attachment bar more keyboard accessible. My plan with bug 630759 is to allow a > user to focus and expand the attachment list by pressing Alt-C and then (say) > saving the attachment by pressing Ctrl-S. I could make the focus move to the > attachment name in the summary bar instead, but I think that's even more > confusing than a hidden twisty. Surely letting them just hit Ctrl-S to save without having to press Alt-C and listen to the expanded list of a single item would be better… ;) Making it more accessible is certainly a worthwhile goal, but I'm not totally convinced that the change I'm asking for makes it much harder to achieve that. Thanks, Blake.

(In reply to comment #38) > That's a fairly big if. If we handle the case of > one-to-three-items-that-fit-on-a-line a little better, I think we won't need to > auto-expand the pane. I don't know if trying to put multiple attachments in the collapsed bar is a good idea. It would be considerably harder to work with a subset of all attachments (e.g. save attachments 1 and 3), which is where the full attachment list comes in. This comes up fairly often for me when reading messages from Gmane, where I end up with bogus "attachments" that are really just footers from mailing lists. I'd rather not be forced to click "save" on each attachment I want, or have to go and delete bogus files after the fact. > Surely letting them just hit Ctrl-S to save without having to press Alt-C and > listen to the expanded list of a single item would be better… ;) There needs to be *some* action to shift the focus to the attachments, otherwise when you hit Ctrl-S, you'll save the message instead of the attachment. I also think that Alt-C would be a good shortcut for expanding the attachment pane in general. Basically, Alt-C would be doing double duty (change focus and expand the list), and I think it's simpler to have it behave the same way all the time.

A question related to : Bug 327302 - Forward of a message with detached attachment should attach the saved file. (dated 2006-02-15) The user had no visual clue that the attachment has been detached : when he forward it Tb send something (in fact the name of the detached file) that is not the attachment. This is very misleading ! If the status of attachment (still attached, detached, deleted) was clearly visible (colour coding, acronym, etc.) the bug may be alleviated by manually reattaching it. Do you plan to give the status of the attachment in this bug or in an other of the series ? Please advise on the right place to post this comment.

This bug certainly isn't the place for that. Bug 327302 would be a closer match, but I think you're really talking about bug 292385.

Yes you are right : I was not aware of bug 292385. Thanks for the information.

(In reply to comment #29) > Comment on attachment 526460 [details] [diff] [review] > Add :hover and RTL states for Aero, fix transparency for XP > > Review of attachment 526460 [details] [diff] [review]: > > The padding on the twisty in the RTL case seems to be wrong. > http://dl.dropbox.com/u/2301433/Attachment/Padding.png > > Other than that, it seems like an improvement, so r=me with that fixed. Since we've got a followup bug for the RTL case, can this be checked in?

Yes, definitely. (It's sort of a shame that we don't have checkin-required on a per-attachment basis. As a workaround, did you want to split out the "Make attachment name clickable" part into a new bug?) Thanks, Blake.

Comment on attachment 526460 [details] [diff] [review] Add :hover and RTL states for Aero, fix transparency for XP [checked in] Checked in: http://hg.mozilla.org/comm-central/rev/25b29b62976d

(In reply to comment #44) > (It's sort of a shame that we don't have checkin-required on a per-attachment > basis. As a workaround, did you want to split out the "Make attachment name > clickable" part into a new bug?) Well, the patch is checked in now, so everything should be ok. Technically, the second patch should be in a new bug, but I don't think it matters too much right now... :)

Adding [has l10n impact] since the second patch changes strings. Should that patch (the one to make the attachment name clickable) block 3.3?

Comment on attachment 528264 [details] [diff] [review] Make attachment name clickable Review of attachment 528264 [details] [diff] [review]: ----------------------------------------------------------------- So, I still think this is the wrong UI, but making the attachment name clickable is definitely a win, so I'm going to say ui-r=me for this patch, with the provision that we need to take another look at it after 3.3. Thanks, Blake.

(In reply to comment #47) > Adding [has l10n impact] since the second patch changes strings. Should that > patch (the one to make the attachment name clickable) block 3.3? Yes, we want that clickable, and if it requires strings, it definitely blocks (and can we get this in by Tuesday?).

This version adds a little bit more space between "1 attachment:" and the filename, so that the hover state doesn't look weird. I also cleaned up the code for getting the selected attachments in the menu popup.

Comment on attachment 531084 [details] [diff] [review] Fix spacing Review of attachment 531084 [details] [diff] [review]: ----------------------------------------------------------------- So, there's the nit I mentioned below, but that was pretty much the only thing I could find, so r=me. (I did have another thought, though. It might be nice to have the single-attachment hover be the same colour as the address hover in the message header. And having it continue to be highlighted as long as the context menu is displayed, again like the addresses do, would also be good, I feel.) Thanks, Blake. ::: mail/base/content/mailCore.js @@ +584,5 @@ > + * > + * @param aAttachment the attachment object > + * @return the TransferData > + */ > +function CreateAttachmentTransferData(aAttachment) { The { should go on its own line for functions.

(In reply to comment #51) > (I did have another thought, though. It might be nice to have the > single-attachment hover be the same colour as the address hover in the > message header. This should be the case already, but chances are I did the Mac theme wrong, since the CSS rules are always different in pinstripe, it seems. > And having it continue to be highlighted as long as the > context menu is displayed, again like the addresses do, would also be good, > I feel.) I'll take a look at this. I had briefly considered doing it, but it seemed complicated. Maybe it's not so bad... > > Thanks, > Blake. > > ::: mail/base/content/mailCore.js > @@ +584,5 @@ > > + * > > + * @param aAttachment the attachment object > > + * @return the TransferData > > + */ > > +function CreateAttachmentTransferData(aAttachment) { > > The { should go on its own line for functions. So, is the dominant coding style pure K&R then? I ask because I plan on cleaning up msgHdrViewOverlay.js one day to make it use a single style (it uses probably half a dozen at the moment). I notice that the Mozilla coding style page says to use 1TBS (though they call it K&R with "cuddle else on both sides"), but it doesn't seem like Thunderbird does that.

in mail code, the rule is to use the prevailing style in the file, if any. Otherwise, it's up to whoever owns/maintains the file.

(In reply to comment #52) > > > +function CreateAttachmentTransferData(aAttachment) { > > > > The { should go on its own line for functions. > > So, is the dominant coding style pure K&R then? I ask because I plan on > cleaning up msgHdrViewOverlay.js one day to make it use a single style (it > uses probably half a dozen at the moment). I notice that the Mozilla coding > style page says to use 1TBS (though they call it K&R with "cuddle else on > both sides"), but it doesn't seem like Thunderbird does that. Well, we're mostly K&R, I believe, as per: https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Naming_and_Formatting_code But following the style of the file is more important. And we don't usually accept patches that just change the whitespace. And some people really like to do things their own way. So, uh, I don't know quite where that leaves us. :) (We discussed the rest of the changes over IRC, and they seem simple enough that you can carry forward my ui-r+ and r+, if you'ld like.) Thanks, Blake.

Pulling forward r+, ui-r+.

Checked in: http://hg.mozilla.org/comm-central/rev/239f022b6dc8

Comment on attachment 531216 [details] [diff] [review] Patch for checkin Review of attachment 531216 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/locales/en-US/chrome/messenger/messenger.properties @@ +353,5 @@ > +# LOCALIZATION NOTE (attachmentCountSingle): This is the format for the > +# attachment header when a message has only one attachment. This is separate > +# from attachmentCount above, since attachmentCountSingle typically ends with a > +# colon. > +attachmentCountSingle=1 attachment: I wonder why the attachment's filename argument was dropped. I think its position should generally be localizable (even though that's not really needed for my locale).

(In reply to comment #57) > > +attachmentCountSingle=1 attachment: > I wonder why the attachment's filename argument was dropped. I think its > position should generally be localizable (even though that's not really > needed for my locale). Well, the name will really be the name followed by the size in a different colour, so it's not really representable as a string. Fortunately, RTL languages should automatically reverse the boxes, and thus the filename will appear in the appropriate place. If there is a language who would like the filename to appear somewhere other than the end (and thinks that the size of the attachment should still appear at the end), please comment in this bug. Thanks, Blake.

More importantly, it would be impossible to make the attachment name clickable and have a hover state if it were part of the same element as the "1 attachment:" string. Assuming localizers can add custom CSS (I have no idea if that's possible), it should be a simple matter of setting -moz-box-ordinal-group on the elements.

For future reference, changed summary to include that for the single attachment case, this bug is where the attachment name on the attachment bar became clickable for opening, context menu, and drag & drop. Relevant considerations for that feature can also be found in Bug 647036 Comment 10 (by Bryan) Bug 647036 Comment 14 (by Jim)