The current XBL bindings for the attachment list in the reader have some problems. Aside from being an obscure name (a <description> tag with selectable="true"), there's no keyboard accessibility, the arrow keys behave strangely, and things don't get updated properly when switching between large and small icons. Attached is a WIP XULRunner app with a new set of bindings for the attachment list (to be used in both the reader and the composer). Here's a short list of why this is better than the status quo: 1) it has built-in support for displaying file size (prettily) 2) it supports scrolling and scrolling-related methods 3) it has a less-stupid name 4) it has both a horizontal and vertical mode so it can be used in multiple places (e.g. reader and composer) 5) up/down/left/right work as expected in horizontal mode 6) it works better when swapping view modes 7) it has smooth scrolling (so drag-and-drop reordering will be easier) 8) it matches platform theming of selected icons a la Windows Explorer and friends (well, it will; currently only Windows theming is done) 9) easier to set context menus (via the itemcontext attribute) 10) built-in handling of attachment objects via appendItem It may also be worth considering whether checkboxes should be used to select attachments (preferably this would be optional, since it would violate ux-consistency).

It's been a while since I've done any real work on this patch, but I wanted to upload it before I forgot about it entirely. bwinton, if you'd like to take a quick look at this, that'd be nice. I know clarkbw is a bit unhappy about the focus/selection story for the attachment pane, but I think this is a step in the right direction, and will make it much easier to implement his ideas as an addon for us to test out. Assuming I can get this version (or something like it) into Thunderbird, I'll even go ahead and make that addon. :)

Comment on attachment 526462 [details] [diff] [review] WIP patch Review of attachment 526462 [details] [diff] [review]: I agree that this seems like a step forward, and we could probably turn it into something we all like. Thanks, Blake.

Adding more dependencies for things that this bug should fix.

This version has most everything working, I think. Notably, I added an SVG filter over the icons so that they get highlighted; I'm not sure the SVG file is getting recognized by the build system, though...

I'm pretty sure everything totally works, including platform-specific styling and behavior. All relevant tests pass now. I'm not asking for review just yet, since I need to a) sleep on this patch to make sure I didn't miss anything obvious, and b) post a summary of the UI changes (they're mostly minor though). With some luck, this might be able to get into 3.3, though I'm certainly cutting it close!

Whoops: forgot to make sure that hitting enter/double-clicking the attachment opens it. I'll fix that tomorrow, though.

Jim, a big, big thank you for doing this!!! Can't wait to see complete and standard-conform UI interaction return to attachment panel. Should the following be included in this bug? Bug 526998 - Implement F2 keyboard shortcut for renaming focused attachments when composing (on Windows and Unix) Even just having F2 to trigger the current pop-up rename dialogue would be great! So I am hoping Bug 526998 might be assigned to you... Please don't get frustrated over attachment bugs mess... you are doing a great job in addressing so many long-standing and beneficial issues!!! I have pushed some of the tracked bugs some small steps further (towards clarification or closure), and I will continue on that as time permits...

Here's a summary of what's changed by this patch: 1) Better name <description selectable="true"> was a really confusing name. Now it's <attachmentlist>. 2) Built-in handling of attachment objects via appendItem Can just pass in the attachment object and let <attachmentlist> do the work. 3) Easier to set item context menus Added an itemcontext attribute, so that it's no longer necessary to make context menus that hold both the item-specific and whole-list menuitems. This also means that bug 516294 is fixed. 4) Built-in support for displaying file size prettily File size gets shown right-aligned in grey. 5) Horizontal and vertical orientation Allows this to be used in both the composer and reader. 6) New view modes Added a new view mode for items ((c) below) for a total of 3. The benefit of (c) is that it allows the most space for the filename and wastes less space than (b). a) [] attachment.txt 123KB b) ┌─┐ └─┘ attachment.txt 123 KB c) ┌─┐ attachment.txt └─┘ 123 KB 7) Smooth scrolling This will make drag-and-drop reordering easier (bug 229224). 8) Matches platform theming The list is designed to look like the platform's file explorer. On Windows and Linux, just the attachment name is highlighted (and the icon is overlaid with a translucent highlight). On Mac, the whole row is highlighted. Also, on Windows/Linux, right-clicking on the non- highlighted areas (i.e. the space after the name) in horizontal mode clicks through to the whole list to simplify getting at the whole-list context menu. 9) Keyboard accessible Use arrow keys to navigate, shift/ctrl/space work like expected for selecting multiple attachment, Ctrl+A selects all, Ctrl+S saves, [Shift]+Delete deletes. 10) Supports scrolling and scrolling-related methods This improves keyboard accessibility by making the attachment list scroll when moving around with the arrow keys. 11) Up/down/left/right work as expected in horizontal mode Before, the <description> bindings treated up as left and down as right. Now up and down actually move up and down.

(In reply to comment #7) > Jim, a big, big thank you for doing this!!! > Can't wait to see complete and standard-conform UI interaction return to > attachment panel. No problem! Even though I don't typically receive a huge amount of attachments, the attachment pane has always bothered me a bit. > Should the following be included in this bug? > Bug 526998 - Implement F2 keyboard shortcut for renaming focused attachments > when composing (on Windows and Unix) > > Even just having F2 to trigger the current pop-up rename dialogue would be > great! > So I am hoping Bug 526998 might be assigned to you... I'll probably take it, but not as a part of this bug. The keyboard accessibility in the composer isn't nearly as bad, so that can wait. I'm sure I'll get to it in the not-too-distant future, though. > Please don't get frustrated over attachment bugs mess... you are doing a great > job in addressing so many long-standing and beneficial issues!!! I have pushed > some of the tracked bugs some small steps further (towards clarification or > closure), and I will continue on that as time permits... Thanks for this. With this bug fixed, the list will be a little more manageable though, since I'll be able to resolve about a half-dozen other issues. There are definitely still some bugs that need looking at in the list though, since I seem to recall a couple that aren't even specifically UX issues.

Alright, I think this is finished, at long last. For a summary of the changes, see comment 8. :bwinton, I asked you for review, since you've been following my attachment changes lately. Don't feel overly pressured to try and get this in before Miramar's string/feature freeze though. I'd hate to force you to review all this in such a short time (unless you'd really like to)! Of course, feel free to pass this review off to anyone else, too. :clarkbw, hopefully these changes seem reasonable to you. I know you've expressed concern about focus issues (specifically, how easy it is to accidentally deselect attachments when trying to select a bunch of them), but I do think this is better than the status quo, and will also greatly simplify any subsequent attempts to resolve those focus issues. It should be pretty straightforward to make an addon (or at least, a patch) from here to try out possible implementations. I've kicked off a try server build, so if everything goes well, I'll post some links to them tomorrow.

Try builds: Linux: http://bit.ly/lX91fT Linux 64 bit: http://bit.ly/mT3b7U Mac: http://bit.ly/mhIi1g Mac 64 bit: http://bit.ly/jIVbD0 Windows: http://bit.ly/lQU4DQ

Comment on attachment 529941 [details] [diff] [review] Fix clicking/hitting enter to open attachments Cancelling actual review for now, since there are several other bugs modifying this part of the code, and I'll just wait until those are done for the sake of bwinton's sanity. :)

Here's an interesting set of guidelines for keyboard selection in Windows: <http://msdn.microsoft.com/en-us/library/ms971323.aspx#atg_keyboardshortcuts_example_of_linear_>. I'm not sure if the Mac/Linux versions of this differ, but maybe we should try to follow these as closely as possible. Notably, my patch treats space as "toggle selection", whereas to deselect in windows, you should be hitting ctrl+space. It actually looks like Windows treats the space bar as identical to clicking, which at least makes things simple. (On the other hand, in HTML forms in Firefox, space is "toggle selection". Oh the wide world of subtly-different UI elements)

Comment on attachment 529941 [details] [diff] [review] Fix clicking/hitting enter to open attachments I played around with the try-server build, and the keyboard navigation and scrolling seem pretty nice, so ui-r=me. (But I'm going to ask you to wait for the other attachment patches to land, and then to update this so that it applies cleanly to trunk before asking me for the review, if you don't mind. :)

(In reply to comment #14) > (But I'm going to ask you to wait for the other attachment patches to land, > and then to update this so that it applies cleanly to trunk before asking me > for the review, if you don't mind. :) Definitely. I've bitrotted this patch several times already, so there's not much sense in looking at the code yet. :) Heck, the changes in bug 657856 were originally done in this bug, due to my frustration at the interface for attachment objects in the reader.

(In reply to comment #14) > (But I'm going to ask you to wait for the other attachment patches to land, > and then to update this so that it applies cleanly to trunk before asking me > for the review, if you don't mind. :) Blake, which are "the other attachment patches" that this bug is waiting for? It would be so great to see the patch of this bug in action...

(In reply to comment #16) > Blake, which are "the other attachment patches" that this bug is waiting for? > It would be so great to see the patch of this bug in action... You can do that now. I posted try builds in comment 11.

A short update: the last bug on my list before this one is bug 661263. Once that's checked in, this bug is next up. After this, there are a couple other minor attachment bugs (I have to go through my CC list again to see exactly what they are), and then - god willing - I'll be done with attachment-related bugs for a while. :) Also, as a note to myself, I think there are a couple theme-related things I need to do before this is completely done.

Ok, I think this is essentially done, barring some theme work for Aero, which could probably be done at a later date. Blake, sorry about the huge patch, but most of the bits in here are interlocked. If it's a big problem, I could probably break it into a couple pieces, but I doubt I could do more than 3 or so without some pieces being useless. If you have any questions about structure or the like, or ideas about how to simplify review, let me know and I'll see what I can do. I'd like to try to get this into Thunderbird 6.0, but if this doesn't get done by July 5, then it's no big deal.

This patch fixes a couple minor issues (mostly the attachment item context menu not showing up in the standalone window). Bienvenu, do you mind taking a look at this? Bwinton seems swamped (probably in no small part due to me). For some background on what this patch does, see comment 8. Conceivably, I could split this out into two or three parts if you think it would help. I also made the Mac theme a bit different from Windows/Linux in an attempt to follow the style in OS X Finder, but that may not be the ideal choice. If someone could test that on Mac and compare to Windows/Linux, it would be much appreciated. See comment 8 item 8 for a description of how this varies per-platform.

This has bit-rotted ever so slightly in msgHdrViewOverlay.xul, but I did get it to apply. I'll run with this on Windows, and try it on the mac. I'm not the best person to review this, but I'll do what I can to help.

On windows, attachments are not selectable (i.e., with multiple attachments, expand the attachment pane, try to click on an attachment, nothing happens).

though the context menu does seem to work on the attachments. But double click doesn't try to open them.

Tried this on the mac - selection does work for attachments, but keyboard navigation doesn't (assuming for example that right arrow is supposed to go to the next attachment). I also found that expanding the attachment widget and selecting an attachment didn't seem to switch focus - pressing up arrow after doing that changed the selected message. But if I tab into the attachment area, then up arrow didn't change the selected message. Was the pref whose name changed a hidden pref? If the pref name was advertised, we should note this change somewhere before shipping the changed version. this method: <method name="getIndexOfFirstVisibleRow"> could theoretically not return a value, since there's no return statement at the end of the method. When I tried to detach an attachment (on windows), I got this error - JavaScript strict warning: chrome://messenger/content/messenger.xul, line 1: ref erence to undefined property this.parentNode.attachments ** failed to handle multiple attachments ** though maybe it has to do with selection not working. Though, using the file menu item detach all also failed - JavaScript strict warning: chrome://messenger/content/messenger.xul, line 1: ref erence to undefined property this.parentNode.attachments ** failed to handle multiple attachments ** I'm going to mark this minus, and give you a chance to try this on windows, if possible.

Hm, it looks like during one of the many times I bitrotted myself in the past and then updated this patch, I lost some important bits. I've started a try build with some fixes to make testing the functionality a bit easier. I'll post the builds once they finish.

This isn't really important, but I realized a fun side effect from this bug: it will be very easy to make a gloda-backed attachment search tab using the <attachmentlist> created here. The attached screenshot is the result of me spending about an hour figuring out how to query gloda and then making a search tab for attachments (the various fields are just placeholders though).

Here's an updated patch, and some try builds: Linux 32: http://bit.ly/nuWlmm Linux 64: http://bit.ly/nlxQh9 OS X 32: http://bit.ly/nqaAot OS X 64: http://bit.ly/oiNepZ Windows: http://bit.ly/o1Qz5u Not asking for review yet, since I still need to make sure it works on Windows...

Comment on attachment 546263 [details] [diff] [review] Fix bitrot properly So, it looks like this works correctly on Windows and Linux now, thus flagging for review again. I'm not sure about Mac, since I don't have access to one, but it's probably pretty similar there. (In reply to comment #24) > Was the pref whose name changed a hidden pref? If the pref name was > advertised, we should note this change somewhere before shipping the changed > version. Yeah, it was a hidden pref. I don't see much in the way of documentation about it either. It might be worth relnoting it, or adding some UI for the pref. Since the attachment bar has a context menu with nothing but "Customize..." on it, we could add a submenu to it to let users pick the view mode. I think that's a separate issue, though.

I bitrotted myself again; here's a new patch (which also contains a fix for the File > Attachments) menu).

I did run with the previous patch yesterday - the first thing I noticed is that the selection color for attachments is very subtle, almost invisible. This is on 64-bit Windows 7

(In reply to comment #30) > I did run with the previous patch yesterday - the first thing I noticed is > that the selection color for attachments is very subtle, almost invisible. > This is on 64-bit Windows 7 Oh, right. Adding new theme files for Windows requires them to be added twice in jar.mn. Here's a patch to fix this. In the long run, we should probably use the Aero-style highlighting that the folder/thread panes use, but I think we could do that in a follow-up.

(In reply to comment #31) > Oh, right. Adding new theme files for Windows requires them to be added > twice in jar.mn. Here's a patch to fix this. This seems to have regressed it, actually. It's back to having no highlight on attachments. And I am using the default 8.01 theme on Windows 7.

Hm, I guess it's because I wrote skin/classic/messenger/ instead of skin/classic/aero/messenger/. I'll try that and actually build on Windows to double check when I get the chance...

Comment on attachment 547968 [details] [diff] [review] Fix Aero theme clearing review while I await new patch...

I've run builds of this in Linux, XP, and Win 7, and it looks alright in all of them.

Comment on attachment 550285 [details] [diff] [review] Fix Aero theme (for real) while I wait for this to build, a few code nits: second line indented too much: + let item = this.getItemAtIndex(this.currentIndex + i); + if (item && !this._canUserSelect(item)) + pageOffset += direction; don't need braces here: + for (let i = 0; i < children.length; i++) { + children[i].setAttribute("imageSize", size); + } should move decl and init of name to where it's first used: + let name = event.attrName.toLowerCase(); + if (event.prevValue == event.newValue) + return; + + if (name == "imagesize") { this can be if (name == "imagesize" && this.hasAttribute...) and don't need braces + if (name == "imagesize") { + if (this.hasAttribute("imagefunc")) + this.setAttribute("image", eval(this.getAttribute("imagefunc"))); + }

Comment on attachment 550285 [details] [diff] [review] Fix Aero theme (for real) looks pretty good.

Comment on attachment 550285 [details] [diff] [review] Fix Aero theme (for real) Pulling forward ui-r+ from comment 14, since this patch hasn't change substantially since then (except for bitrot-fixing).

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

Quick followup, since I forgot to save a file before pushing: http://hg.mozilla.org/comm-central/rev/f5e24cffb9b2