Closed Bug 680695 Opened 13 years ago Closed 13 years ago

Implement / add context menu with actions for all attachments for right-click on [paperclip icon], "X attachments" or [size] labels of attachment bar

Categories

(Thunderbird :: Message Reader UI, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: thomas8, Assigned: squib)

References

Details

(Whiteboard: [has interactive demo incl. bug 680667])

Attachments

(5 files)

STR

1) from message reader's attachment bar (especially when collapsed),
2) right-click on paperclip icon or "X attachments" label or [attachment size] label

Actual result
- shows context menu for attachment bar ("Customize")

Expected result
- show context menu with actions for all attachments (see attached mockup screenshot):

Open all...
Save all...
-------------
Detach all...
Delete all...

Reasons:
- when attachment pane is collapsed, the paperclip icon and the "x attachments" label are the only visual representation of the attachments.
- the [Save all] button is far away from that visual representation (which may have good design reasons, but it's not exactly honoring the ux principle of in-place editing)
- customizing the attachment toolbar is a very rare use case, which is easily and intuitively available on attachment bar's vast blank space area and on the [save all] button.
- actions on all attachments are a much more frequent use case
- low-cost, no-risk mouse shortcut for users who find this intuitive
Attachment #554664 - Flags: feedback?(bwinton)
Comment on attachment 554664 [details]
Screenshot mockup of "all attachments"-context menu on attachment bar icon/label

I don't _mind_ the idea, but I'm giving it f-, because I want to hear more details about how this would interact with the single-attachment case.

Taking into account bug 680667, the icon and size of multiple attachments will behave for left-clicks like the twisty or the empty space on the right.  Having them behave differently for right-clicks seems like the wrong thing to do.

But, as I mention in that bug, perhaps we want to make the multiple attachments label do similar things to the single attachment label, for hover, left clicks, and right clicks…

What do you think about that?
Attachment #554664 - Flags: feedback?(bwinton) → feedback-
I think we already have plenty of ways to get at the various actions for attachments (File > Attachments menu, the expanded attachment list, keyboard shortcuts in the expanded list, the toolbar button). Adding more ways to do the same thing has the cost of making it harder to change things in the future without disrupting people. Case in point: commenters on BMO have argued in favor of context menus because context menus always stay they same, whereas buttons "come and go". While I'm not sure that's a reasonable position to take, doing this would create the expectation for them that we keep these context menus around forever.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #1)
> But, as I mention in that bug, perhaps we want to make the multiple
> attachments label do similar things to the single attachment label, for
> hover, left clicks, and right clicks…

This would probably mean 1) getting rid of the "1 attachment: " prefix for the single attachment case to be consistent with how the label gets highlighted on hover, and 2) opening all attachments on a single click of "N attachments". (1) might be bad since we'd lose the keyword "attachments" that people are looking for, and (2) is definitely bad, since you'd end up with a big pile of dialogs/application windows if you do that on a message with lots of attachments.

As an alternative, I'd suggest adding non-default toolbar buttons for "Open", "Detach", and "Delete". From most points on the screen, this would have better Fitts's Law properties than a context menu on the "N attachments" label, since the buttons are taller than the label, not to mention taking fewer clicks. This would also help people who frequently want to do something other than save attachments.
Summary: Implement / add context menu with actions for all attachments for right-click on paperclip icon or "X attachments" label of attachment bar → Implement / add context menu with actions for all attachments for right-click on [paperclip icon], "X attachments" or [size] labels of attachment bar
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #1)
> Comment on attachment 554664 [details]
> Screenshot mockup of "all attachments"-context menu on attachment bar
> icon/label
> 
> I don't _mind_ the idea, but I'm giving it f-, because I want to hear more
> details about how this would interact with the single-attachment case.

An interactive demo says more than a thousand words... please test it (right-/left-click on twisty/paperclip/labels)!

> Taking into account bug 680667, the icon and size of multiple attachments
> will behave for left-clicks like the twisty or the empty space on the right.
> Having them behave differently for right-clicks seems like the wrong thing
> to do.

In the eye of the user, the whole sequence of [paperclip icon]+[X attachments]+([file name])+[size] is more like a single entity that represents attachments. Therefore, consistent contextual menus on right-clicking anywhere within that entity are a logical and intuitive enhancement, which allows in-place action without having to uncollapse the attachment pane.

> But, as I mention in that bug, perhaps we want to make the multiple
> attachments label do similar things to the single attachment label, for
> hover, left clicks, and right clicks…
> What do you think about that?

For multiple attachments, imo we cannot easily have a single-click to open them all, as this would be unexpected and even risky without knowing the file names. Still, the UI proposed in the demo is 100% ux-consistent for both single and multiple attachment cases:

- New: Left-click on paperclip icon, 'x attachment(s)' or 'size' label -> show attachment list (like paperclip icon in Outlook etc.) (Bug 680667)
- New: Right-click on paperclip icon, 'x attachment(s)' or 'size' label -> show context menu for single attachment/all attachments respectively (Bug 680695)
- /In addition/, for single attachment case, keep existing 'shortcut' functionality on the file name (left-click -> open, right-click -> context menu) (implemented by Bug 646032).

The main idea is very simple:
- left-click always shows the attachment list (for added comfort, in case of single attachment, clicking exactly /on the filename/ will open it; still consistent as we don't show the file names for multiple attachments)
- right-click always shows the contextual menu for single/all attachments respectively

I've played with this for a couple of days, finding it comfortable and intuitive all the time.

Implementing these enhancements will also mitigate (not solve) the serious UI impediments inflicted on users as described in Bug 647036 (Allow attachment pane of a message to open non-collapsed by default). I love the collapsed attachment pane (thanks Jim!!!), but we must give users a choice to keep it open by default. The enhancements proposed in the demo will make it as easy as possible for users to access or act on the hidden attachments.
Attachment #556236 - Flags: feedback?(bwinton)
(In reply to Jim Porter (:squib) from comment #2)
> I think we already have plenty of ways to get at the various actions for
> attachments (File > Attachments menu, the expanded attachment list, keyboard
> shortcuts in the expanded list, the toolbar button).

File > Attachments is quite far away from the actual UI (thus not too discoverable), similarly for the toolbar button on the far right, and the expanded list will only help after expanding - which is where bug 680667 and this bug 680695 are trying to make things even easier than before. I am thinking of users who intuitively try left- or right-clicking on the [paperclip-x-attachments] visual entity and I believe we could be helpful for those clicks instead of doing nothing.

> While I'm not sure that's a reasonable
> position to take, doing this would create the expectation for them that we
> keep these context menus around forever.

Attaching an existing context menu doesn't involve heavy coding (3 lines, said Jim) or maintenance or disruption even for addons who might wish to change things. On the other hand, if users expect us to keep a context menu forever, that seems like good sign that the context menu is actually useful :)

> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #1)
> > But, as I mention in that bug, perhaps we want to make the multiple
> > attachments label do similar things to the single attachment label, for
> > hover, left clicks, and right clicks…
> 
> This would probably mean 1) getting rid of the "1 attachment: " prefix for
> the single attachment case to be consistent with how the label gets
> highlighted on hover, and 2) opening all attachments on a single click of "N
> attachments". (1) might be bad since we'd lose the keyword "attachments"
> that people are looking for, and (2) is definitely bad, since you'd end up
> with a big pile of dialogs/application windows if you do that on a message
> with lots of attachments.

Yes, (1) and (2) are bad indeed. Fortunately, they are not required to create a consistent behaviour, as shown in the demo of attachment 556236 [details]. The little difference in the current UI comes with the fact that we only show the file name in the single attachment case, which makes sense and doesn't interfere with the proposals here.

> As an alternative, I'd suggest adding non-default toolbar buttons for
> "Open", "Detach", and "Delete". From most points on the screen, this would
> have better Fitts's Law properties than a context menu on the "N
> attachments" label, since the buttons are taller than the label, not to
> mention taking fewer clicks. This would also help people who frequently want
> to do something other than save attachments.

Optional buttons are cool, but not everyone likes more buttons. Fortunately, having them or not will not adversely affect or be preclusive of the proposals here and in bug 680667 in any way.
See Also: → 680667
Whiteboard: [has interactive demo incl. bug 680667]
Important usage note on Attachment 556236 [details]

Please open the Interactive XUL Demo in fullscreen mode by clicking directly on the _attach ment 556236_ link; unfortunately, clicking on [details] puts it in a frame that will cut of the bottom part of the demo on small screens without adding scrollbars. There are *two* attachment toolbars in the demo, one for single and one for the multiple attachments case.
Comment on attachment 556236 [details]
Interactive XUL Demo (V1, 2011-08-27) for attachment bar UX enhancements (left-click / context menu, bug 680667 / bug 680695)

Thomas, all I get when I click on the demo is "Remote XUL
 This page uses an unsupported technology that is no longer available by default in Firefox."

Is there another way you can demonstrate the UI you're trying to get my feedback on?  (I'm clearing out the feedback? flag, since I can't really give you any feedback on it as it is.)

Thanks,
Blake.
Attachment #556236 - Flags: feedback?(bwinton)
Blake, you could install Remote XUL Manager <https://addons.mozilla.org/en/firefox/addon/remote-xul-manager/> and then add a permission for bugzilla.mozilla.org. With this the demo works.
Comment on attachment 556236 [details]
Interactive XUL Demo (V1, 2011-08-27) for attachment bar UX enhancements (left-click / context menu, bug 680667 / bug 680695)

(In reply to Richard Marti [:paenglab] from comment #7)
> Blake, you could install Remote XUL Manager
> <https://addons.mozilla.org/en/firefox/addon/remote-xul-manager/> and then
> add a permission for bugzilla.mozilla.org. With this the demo works.

That's right. Quick & easy indeed! And, to play with xul files on your own harddisk:
about:config -> (from context menu) new -> boolean -> dom.allow_XUL_XBL_for_file -> true

Setting feedback? flag again, as re-enabling xul is a matter of 2 mins, compared to hours spent on producing the demo...

Remember comment 5, you need fullscreen view of demo to see both attachment bars.
Attachment #556236 - Flags: feedback?(bwinton)
Comment on attachment 556236 [details]
Interactive XUL Demo (V1, 2011-08-27) for attachment bar UX enhancements (left-click / context menu, bug 680667 / bug 680695)

Thank goodness for addons, eh?  :)

So, I've played around with it for a while, and it is quite consistent.

My biggest complaint, and it's relatively minor, is that there's no way to tell when right-clicking will give me the "Customize" menu, and when it will give me the Attachment menu.  If we could shade the background slightly for the parts that will show the Attachment menu, that could help.

(And I'm not so concerned about adding another right-click menu to the attachment bar, given that almost everything else in our UI has a right-click menu.)

Thanks,
Blake.
Attachment #556236 - Flags: feedback?(bwinton) → feedback+
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #9)
> Comment on attachment 556236 [details]
> Interactive XUL Demo (V1, 2011-08-27) for attachment bar UX enhancements
> (left-click / context menu, bug 680667 / bug 680695)
> 
> Thank goodness for addons, eh?  :)

see the devilish spark in Blake's eyes ;)

> So, I've played around with it for a while, and it is quite consistent.

I'm happy to hear that :)))
Thank you for quick feedback!

> My biggest complaint, and it's relatively minor, is that there's no way to
> tell when right-clicking will give me the "Customize" menu, and when it will
> give me the Attachment menu.  If we could shade the background slightly for
> the parts that will show the Attachment menu, that could help.

Absolutely. I've added the hover effect to V2 of the demo, for what I refer to as "Attachment Header Range" (the visual entity comprising paperclip icon, 'X attachments' label, and size label), and I love it. Thanks, Blake!

In addition, I tried a hover effect on the twisty, which is so small on Win7, to make it a larger click target. I like it, but I don't have strong feelings about it. On other OS like XP with a [+] twisty (I suppose), things might be different.

Let me know if there is anything else, otherwise I'd love to request ui-review for these UX enhancements, which just put the finishing touches to Jim's great work.
Attachment #557099 - Flags: feedback?(bwinton)
Attachment #554664 - Flags: feedback-
Comment on attachment 557099 [details]
Interactive XUL Demo V2 for attachment bar UX enhancements (now with hover effect)

(In reply to Thomas D. from comment #10)
> > My biggest complaint, and it's relatively minor, is that there's no way to
> > tell when right-clicking will give me the "Customize" menu, and when it will
> > give me the Attachment menu.  If we could shade the background slightly for
> > the parts that will show the Attachment menu, that could help.
> Absolutely. I've added the hover effect to V2 of the demo, for what I refer
> to as "Attachment Header Range" (the visual entity comprising paperclip
> icon, 'X attachments' label, and size label), and I love it. Thanks, Blake!
>
> In addition, I tried a hover effect on the twisty, which is so small on
> Win7, to make it a larger click target. I like it, but I don't have strong
> feelings about it. On other OS like XP with a [+] twisty (I suppose), things
> might be different.

I'm not a fan, because it seems too busy, and the effect when you click or right-click is the same as the empty space to the right which we don't highlight.  (I'm happy leaving the click target that large, but I don't feel we need to show it with a highlight.)

> Let me know if there is anything else, otherwise I'd love to request
> ui-review for these UX enhancements, which just put the finishing touches to
> Jim's great work.

Removing the highlight from the twisty is the main thing for me.  f=me with that changed.  :)

Oh, and the styling of the attachment bar has changed a bit in TB9, but I think we want to change it back, so that's something you'll need to keep in mind when you write the patch...

Thanks,
Blake.
Attachment #557099 - Flags: feedback?(bwinton) → feedback+
My two cents: I don't think we really need a hover state; I can't think of anything that has a hover state just because it has a context menu. Usually a hover state means "this is left-clickable". (clarkbw suggested making the entire bar have a hover state to indicate that you can click on it to expand/collapse the bar, but I resisted since it was somewhat obnoxious changing the color of the whole bar when you hovered over it, especially if you were just trying to click the "save all" button.)

One factor that makes this more complex is that we don't actually want the same context menu for the single attachment case and the multiple attachment case. The single attachment case should say "Open" instead of "Open all...", like the toolbar button does.

I'm also not sure about the context menu on the size label, since I'd expect something related to the size when I right-click on that; of course, the "Customize..." item doesn't make much sense there either. Adding the context menu to the "N attachments" label is probably ok though. I'm neutral on the context menu for the paperclip; I guess you could argue that it's part of the "N attachments" label, even though it's really just there as a visual cue.
Yeah, I was originally thinking of it always showing up as slightly different, but I could see removing the hover state entirely, since other bits of the UI don't mark context-menu changes that way.
(In reply to Jim Porter (:squib) from comment #12)
> My two cents: I don't think we really need a hover state; I can't think of
> anything that has a hover state just because it has a context menu. Usually
> a hover state means "this is left-clickable".

Exactly. Hover state indicates that "this is left-clickable"! Jim, have you tried that in the demo? The whole "attachment header range" (paperclip, n-attachments-label, and size) is left-clickable to un-collapse attachment panel.

The logic is: "Where are my attachments?" "Oh, look there, it says 'x attachments', so let me click there to see them" and that's exactly what we should do (ux-discovery), un-collapse the list, as in the demo (bug 680667). It doesn't make sense that we can left-click on blank parts of the bar, and it expands, but when we click where it /actually/ says "x attachments", nothing happens, currently. And we can't do much else on left-click, really: Opening all attachments would be unexpected, cluttered, and risky, while a /menu/ on /left/-click would be unexpected and obfuscates access to individual attachments.

I understand that Blake tested the left-clicks in the demo and they are included in his feedback+ ... :)

As for the hover state on the attachment header range (from paperclip to size), personally I like it, it's unobtrusive, and I think it helps ux-discovery for both left- and right-click (but I won't cry if it gets dropped as long as we keep the left/right-click behaviour).

> clarkbw suggested making the
> entire bar have a hover state to indicate that you can click on it to
> expand/collapse the bar, but I resisted since it was somewhat obnoxious
> changing the color of the whole bar when you hovered over it, especially if
> you were just trying to click the "save all" button.)

I agree with Jim. Hover state for the whole bar would be overkill ("too busy"). I was thinking about a tooltip like "Click here to show attachments list", but I am not sure. I also agree with Blake that the hover state for the twisty should be dropped.
 
> One factor that makes this more complex is that we don't actually want the
> same context menu for the single attachment case and the multiple attachment
> case. The single attachment case should say "Open" instead of "Open all...",
> like the toolbar button does.

Yes, that's exactly how I presented it in the demo, and I'd think it's about 2 lines of code to change the context attribute of the respective enclosing box, using the same condition that you use to change the button.

> I'm also not sure about the context menu on the size label, since I'd expect
> something related to the size when I right-click on that; of course, the
> "Customize..." item doesn't make much sense there either. Adding the context
> menu to the "N attachments" label is probably ok though. I'm neutral on the
> context menu for the paperclip; I guess you could argue that it's part of
> the "N attachments" label, even though it's really just there as a visual
> cue.

In short: Jim seems to have no objections that we /could/ have the respective attachment(s) context menu on /all/ of the following: paperclip, 'x attachments' label, and size label. Exactly like in the demo.

(As for 'size' label, I agree with Jim that "customize" wouldn't make sense. On the other hand, you could argue that it's the tail of the "x attachments" label, and thus, as a matter of courtesy, we just make the click target for the attachments context menu a bit bigger. Furthermore, right-clicking on the size and getting the attachment context menu might actually make sense as in "wow, those attachments are too big (mouse-pointing at the size), how can I detach/delete them?" and there you go, right-click is ready for you right there with "detach/delete" actions.)

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #11)
> Comment on attachment 557099 [details]
> Interactive XUL Demo V2 for attachment bar UX enhancements (now with hover
> effect)
> 
> (In reply to Thomas D. from comment #10)
> > In addition, I tried a hover effect on the twisty, which is so small...
>
> I'm not a fan, because it seems too busy, and the effect when you click or
> right-click is the same as the empty space to the right which we don't
> highlight.  (I'm happy leaving the click target that large, but I don't feel
> we need to show it with a highlight.)

Note that Blake is /only/ arguing against the hover state for the /twisty/ here, as he points out below.

> > Let me know if there is anything else, otherwise I'd love to request
> > ui-review for these UX enhancements, which just put the finishing touches to
> > Jim's great work.
> 
> Removing the highlight from the twisty is the main thing for me.  f=me with
> that changed.  :)

Removing the hover/highlight from the twisty is fine with me. Thanks for rapid feedback+!
(In reply to Thomas D. from comment #14)
> (In reply to Jim Porter (:squib) from comment #12)
> > My two cents: I don't think we really need a hover state; I can't think of
> > anything that has a hover state just because it has a context menu. Usually
> > a hover state means "this is left-clickable".
> 
> Exactly. Hover state indicates that "this is left-clickable"! Jim, have you
> tried that in the demo? The whole "attachment header range" (paperclip,
> n-attachments-label, and size) is left-clickable to un-collapse attachment
> panel.

It's only left-clickable in the sense that the entire bar will be left-clickable. To change the hover state for the "attachment header range" would be inaccurate, so I'd rather not do it at all. This does hurt discoverability somewhat, but this could be ameliorated by adding tooltips. Specifically:

* "Click to open attachment" on the single attachment name
* "Save all attachments" on the save all button (this is already there, I believe)
* "Click to expand attachment pane" on everything else (or "collapse" when it's
  expanded)

Maybe we should avoid "Click to...", since that seems a bit too much like "Click _here_ to go to Google", which is bad style, at least on web pages. Better suggestions for strings would be appreciated. :)

> (As for 'size' label, I agree with Jim that "customize" wouldn't make sense.
> On the other hand, you could argue that it's the tail of the "x attachments"
> label, and thus, as a matter of courtesy, we just make the click target for
> the attachments context menu a bit bigger.

I'll probably include the size label in the scope of this, just because it's easier than trying to figure out how to disable the context menu entirely.
(In reply to Jim Porter (:squib) from comment #15)
> It's only left-clickable in the sense that the entire bar will be
> left-clickable. To change the hover state for the "attachment header range"
> would be inaccurate, so I'd rather not do it at all. This does hurt
> discoverability somewhat...

I'm afraid Jim is perfectly right in all points made here, so we've lost our case for the hover state *sigh* ;) (it's inaccurate for the left-click, and unheard of for the right-click)

> but this could be ameliorated by adding tooltips.

Which for the "attachment header range" hurts discoverability of the right-click a bit more by pointing out the action for left-click, but hey, the left-click is very important to be discovered, and it certainly wins the trade-off and it's obvious we need the tooltip for the /whole/ bar if we go for it. So yes, let's go for it.

> Specifically [for the tooltips]:
> * "Click to open attachment" on the single attachment name
> * "Save all attachments" on the save all button (...already there...)
> * "Click to expand attachment pane" on everything else (or "collapse" when
> it's expanded)
> Maybe we should avoid "Click to...", ...which is bad style,
> Better suggestions for strings would be appreciated. :)

Let's avoid 'Click to' by all means, it's something you could add to every tooltip and then you end up removing it from everywhere realizing that it doesn't add a single inch of useful information, on the contrary, it obfuscates the relevant information.

Furthermore, let's try to be consistent with other tooltips: Most of them seem to have a "natural language approach" e.g. by adding articles which are not essential for understanding (as opposed to a computer-style brief caption with initial capitals for all words): We have...
- "Create /a/ new message",
- "Go to /the/ addressbook",
- "reply to /this/ message", and
- "save all /the/ attached files" (all natural language style)
As a side note, of course TB is not consistent, so we also have "Edit Contact" (without /this/, and with different capitalizatioin), and "Click to sort by subject" (instead of "Sort messages by subject").

Anyway, I believe the intended default is:
- be explanatory (never mind an extra word if it helps to understand)
- natural language (e.g. simple wording, with added articles)
- initial capital for the first word only

In that line, for the sake of ux-consistency, I suggest:

* "Open the attached file" on the single attachment name (that's really needed!)
* "Save all the attached files" on the save all button (this is already there, but we need to change capitalization in expectation of Bug 478468 Comment 49 where Blake's voice of sanity will finally restore decent capitalization)
* "Show the attachment pane" on everything else while collapsed [1] or
* "Hide the attachment pane" when it's expanded
(Imho, "show" and "hide" are simpler and more natural, compared to "expand" and "collapse" which sound a bit more technical, and "collapse" /might/ also come with negative associations from collapsing buildings, people etc...)

^^^ Blake / Jim, comments on these tooltip suggestions?

> I'll probably include the size label in the scope of this, just because it's
> easier than trying to figure out how to disable the context menu entirely.

Thank you! Albeit for the wrong reasons, but I'm sure it's the right thing to do ;)

[1]: Jim, technically you can assign the tooltip to the /whole/ <toolbar> and then define empty string or different tooltips as an override for the few elements that should /not/ have it.
Assignee: nobody → squibblyflabbetydoo
V3 of the Demo now incorporates the last refinements as suggested by Blake and Jim, so I'd think this is ready for ui-review.

Changes over V2:
- remove hover state completely (Blake's comment 11 for the twisty, and Jim's comment 15 was the final knock-out for hovering attachment header range)
- add tooltips for ux-discovery (Jim's comment 15, refined by my comment 16)

Thank you for constructive criticism! IMO, this looks & feels great now, and the whole UI and behaviour is much more helpful and consistent in those little corners than before.
Attachment #558140 - Flags: ui-review?(bwinton)
(In reply to Thomas D. from comment #17)
> Created attachment 558140 [details]
> Interactive XUL Demo V3 for attachment bar UX enhancements (now with
> tooltips; no hover)

Of course, ignore the glitch where the tooltips sometimes cover the context menus, but actually, that happens in other spots of TB's UI, too (and probably shouldn't, but it's not worth spending too much time on that...)
Here's a patch to do this. Behavior is as follows:

Left click
----------

* On toolbar button -> activates button (duh)
* On attachment filename -> opens file
* Anywhere else -> expands/collapses attachment pane

Right click
-----------

* On attachment info area (paperclip, "N attachments", filename, size) -> opens
  context menu for the attachment(s) (same as the dropmarker for the save
  button)
* Anywhere else -> opens toolbar customization menu

Tooltips
--------

* On toolbar button -> "Save (all) the attached file(s)"
* On attachment filename -> "Open the attached file"
* Anywhere else -> "Show/Hide the attachment pane"

The tests are pretty comprehensive, I think.
Attachment #559714 - Flags: ui-review?(bwinton)
Attachment #559714 - Flags: review?(bwinton)
Status: NEW → ASSIGNED
Comment on attachment 558140 [details]
Interactive XUL Demo V3 for attachment bar UX enhancements (now with tooltips; no hover)

That seems to work well for me.  ui-r+.

Later,
Blake.
Attachment #558140 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 559714 [details] [diff] [review]
Fix this and test it

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

I like the way it looks, and it seems to work well, so ui-r=me.


When I run "make SOLO_TEST=attachment/test-attachment.js mozmill-one", I get a bunch of failures:

SUMMARY-UNEXPECTED-FAIL | d:\Programming\comm-central\mail\test\mozmill\attachment\test-attachment.js | test_attachment_right_click_single
  EXCEPTION: Timeout waiting for popup to open
    at: utils.js line 429
       TimeoutError("Timeout waiting for popup to open") utils.js 429
       waitFor((function () popupElem.state == "open"),"Timeout waiting for popup to open",1000,50) utils.js 467
       wait_for_popup_to_open([object XULElement]) test-folder-display-helpers.js 1379
       subtest_attachment_right_click("attachmentBar","attachment-toolbar-context-menu") test-attachment.js 163
       test_attachment_right_click_single() test-attachment.js 183
            frame.js 554
            frame.js 623
            frame.js 666
            frame.js 503
            frame.js 678
            server.js 182
            server.js 186
[snip...]
SUMMARY-PASS | test_attachment_toolbar_customize
SUMMARY-UNEXPECTED-FAIL | d:\Programming\comm-central\mail\test\mozmill\attachment\test-attachment.js | test_select_all_attachments_key
  EXCEPTION: Should have selected all attachments!: '0' != '2'.
    at: test-folder-display-helpers.js line 2809
       assert_true(false,"Should have selected all attachments!: '0' != '2'.") test-folder-display-helpers.js 2809
       assert_equals(0,2,"Should have selected all attachments!") test-folder-display-helpers.js 2796
       test_select_all_attachments_key() test-attachment.js 326
            frame.js 554
            frame.js 623
            frame.js 666
            frame.js 503
            frame.js 678
            server.js 182
            server.js 186
SUMMARY-UNEXPECTED-FAIL | d:\Programming\comm-central\mail\test\mozmill\attachment\test-attachment.js | test_delete_attachment_key
  EXCEPTION: Timeout waiting for modal dialog to open.
    at: utils.js line 429
       TimeoutError("Timeout waiting for modal dialog to open.") utils.js 429
       waitFor((function () this.waitingForOpen == null && this.monitorizeClose()),"Timeout waiting for modal dialog to open.",10000,100,[object Object]) utils.js 467
       WindowWatcher_waitForModalDialog("commonDialog",(void 0)) test-window-helpers.js 390
       wait_for_modal_dialog("commonDialog") test-window-helpers.js 623
       test_delete_attachment_key() test-attachment.js 347
            frame.js 554
            frame.js 623
            frame.js 666
            frame.js 503
            frame.js 678
            server.js 182
            server.js 186
SUMMARY-UNEXPECTED-FAIL | d:\Programming\comm-central\mail\test\mozmill\attachment\test-attachment.js | test_attachments_compose_menu
  EXCEPTION: Timed out waiting for window open!
    at: utils.js line 429
       TimeoutError("Timed out waiting for window open!") utils.js 429
       waitFor((function () this.monitorizeOpen()),"Timed out waiting for window open!",10000,100,[object Object]) utils.js 467
       WindowWatcher_waitForWindowOpen("msgcompose") test-window-helpers.js 290
       wait_for_new_window("msgcompose") test-window-helpers.js 586
       wait_for_compose_window() test-compose-helpers.js 211
       open_compose_with_forward() test-compose-helpers.js 163
       test_attachments_compose_menu() test-attachment.js 364
            frame.js 554
            frame.js 623
            frame.js 666
            frame.js 503
            frame.js 678
            server.js 182
            server.js 186
make: *** [mozmill-one] Error 1

So I'm going to have to say r-.

(One thing I did notice was that it looked like the popup remained open, which could have caused the rest of the failures, so perhaps the timeout just isn't quite long enough?)

And other than the failing tests, I'm happy with the code, so if you can get them passing, it'll be a pretty quick r=me.

Thanks,
Blake.
Attachment #559714 - Flags: ui-review?(bwinton)
Attachment #559714 - Flags: ui-review+
Attachment #559714 - Flags: review?(bwinton)
Attachment #559714 - Flags: review-
I kicked off a try build here: <http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=ebfec09e9529>

Hopefully it will work; it's possible that the tests depended on something in front of it in my patch queue, but it's in front now, so it should be ok.
It looks like all the tests passed, except for Windows, which suffered a builder exception early on, and Linux, which had an unrelated failure.
Comment on attachment 559714 [details] [diff] [review]
Fix this and test it

Based on comment 24, everything looks good on my end. Could you take another quick look at this?
Attachment #559714 - Flags: review- → review?(bwinton)
Comment on attachment 559714 [details] [diff] [review]
Fix this and test it

r=me, based on the successful try-server run!  Woo!
Attachment #559714 - Flags: review?(bwinton) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/84e7dc2dda02
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
Depends on: 698272
See Also: → 1427092
You need to log in before you can comment on or make changes to this bug.