Implement cmd_removeAllAttachments, cmd_focusAttachmentPane and polish composition's attachment bucket UX/behaviour

Status

Thunderbird
Message Compose Window
--
enhancement
ASSIGNED
10 days ago
9 days ago

People

(Reporter: Thomas D. (currently busy elsewhere), Assigned: Thomas D. (currently busy elsewhere))

Tracking

(Blocks: 4 bugs, {ux-consistency, ux-efficiency, ux-minimalism})

Trunk
ux-consistency, ux-efficiency, ux-minimalism
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 8940534 [details] [diff] [review]
Patch V.1 (see comment 0)

+++ This bug was initially created as a clone of Bug #1427092 +++

Jörg, amongst other benefits, this implements Alt+M to summon the attachment pane, "cmd_removeAllAttachments" and the smart attachment pane close button; maybe you would want to review this to fast-track landing?

Richard has essentially UX-approved this already:
Bug 1427092, comment 11:
> Works good, what I tested and all makes sense you added. :)
> ui-review+ (Bug 1427092, comment 14)
Pls have another look as there have been some minimal changes (e.g., moved cmd_focusAttachmentPane to View menu, now with other commands which show/hide things).

Richard, this needs some CSS to fine-tune the header bar as you promised, and all CSS for non-Windows OS.

This was originally developed for Bug 1427092, but I figured that we can land all the peripheral UX improvements and all things "under the hood" first as we don't know if, how or when Bug 1427092 is going to land. I'll keep the numbering from Bug 1427092 comment 0, but I'll adjust the description to match the current plan.

Changes/Improvements:

3) Attachment pane header: Dynamically replace useless "0 bytes" total size information with an [x] button to close the pane when there are no attachments, incl. button tooltip to for disambiguation.

4a) Fix/clean up RemoveAllAttachments() algorithm; now consistent with RemoveSelectedAttachment()
4b) Implement cmd_removeAllAttachments.
4b) Add "Remove All Attachments" menu item to attachment *list* context menu.

5) Preserve keyboard shortcut Alt+M to (show and) focus attachments pane and expose it in View Menu; implement new command: Show/Focus Attachment Pane (cmd_focusAttachmentPane).
6) Alt+M aka cmd_focusAttachmentPane now helpfully shows the attachment pane even when it's hidden, providing a convenient starting point for adding attachments; dynamic caption: "Show Attachment Pane" | "Focus Attachment Pane"
(unfortunately, due to existing bugs, this command doesn't always work)
7) Extending the existing functionality of "N attachments" label: Also allow click on header blank space, total size label to focus attachment pane, and add a tooltip to expose this functionality: "Attachments pane"

8) For attachment pane whitespace, restore "single-click-to-attach-file(s)" functionality and expose it with a tooltip: "Attach File(s)"
9) While there are *selected* attachments, the first left-click on attachment pane whitespace must clear selection; dynamically adjust tooltip accordingly: "Clear Selection"

10) Polish some command handlers and other nits:
- cmd_sortAttachmentsToggle: More correct label "Sort A-Z|Z-A" when all attachments selected.
- Attachment pane box focus border now only shown when there are no attachments (avoids irritating double focus border: box and item)
- Ensure that there's always a focused item (with existing focus on pane, e.g. after removing all and re-adding some; otherwise there's something in listbox.xml which auto-sets listitem focus when pane is focused)
- Improved helper functions
Attachment #8940534 - Flags: ui-review?(richard.marti)
Attachment #8940534 - Flags: review?(jorgk)

Comment 1

10 days ago
(In reply to Thomas D. (currently busy elsewhere) from comment #0)
> +++ This bug was initially created as a clone of Bug #1427092 +++
> Jörg, amongst other benefits, this implements Alt+M to summon the attachment
> pane, "cmd_removeAllAttachments" and the smart attachment pane close button;
> maybe you would want to review this to fast-track landing?
So we're moving some of the stuff from bug 1427092 here before we decide on how to deal with the attachment button? Divide and conquer?
(Assignee)

Comment 2

10 days ago
(In reply to Jorg K (GMT+1) from comment #1)
> (In reply to Thomas D. (currently busy elsewhere) from comment #0)
> > +++ This bug was initially created as a clone of Bug #1427092 +++
> > Jörg, amongst other benefits, this implements Alt+M to summon the attachment
> > pane, "cmd_removeAllAttachments" and the smart attachment pane close button;
> > maybe you would want to review this to fast-track landing?
> So we're moving some of the stuff from bug 1427092 here before we decide on
> how to deal with the attachment button? Divide and conquer?

Hehe... yeah, something like that :) In fact, we're moving *most* of the stuff from that bug here. My main motive here is to get a more stable codebase faster, especially in MsgComposeCommands.js because I keep updating and re-shuffling rejected or missing code hunks around as somehow this is all connected and playing together...
Apart from deciding the UI, that bug also still needs migration code which will take more time unless somebody else picks that up. (Note that the button can be moved to the right even without that patch applied, but it won't have those extra functions yet).

Comment 3

10 days ago
Comment on attachment 8940534 [details] [diff] [review]
Patch V.1 (see comment 0)

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

First pass through. This is nice. I don't like that you changed the double-click to single-click, that will annoy many people, so please consider changing that back (or is there a comment somewhere that makes a case for it?).

Richard, what do you think?

There are a few nits: I'm really confused by your comparisons. For a "natural" number, < 1 means == 0, no? So if you want to express "no attachment" that's == 0, or perhaps <= 0, if for some reason the count can be negative.

All in all it's a fine effort and I love being able to open the bucket and close an empty bucket. I never liked the thing appearing out of the blue and not being able to get rid of it.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +690,5 @@
> +            cmdFocusAttachmentPane.getAttribute("accesskeyFocus"));
> +        }
> +        return !gWindowLocked &&
> +               (document.activeElement.id != "attachmentBucket") ||
> +               document.getElementById("attachments-box").collapsed;

Can you please explain the code in this function to someone not so familiar with what a cmd_* needs to do.

@@ +1075,5 @@
>          let btnLabelAttr;
>  
> +        if (attachmentsSelCount > 1 &&
> +            attachmentsSelCount < attachmentsCount()) {
> +          // Sort selected attachments only, which needs at least 2 of them :),

What's the smiley doing here?

@@ +4567,5 @@
> +  // currently we don't indicate focus on the listbox itself, assuming
> +  // there'll always be a focused attachment.
> +  let firstAddedAttachment = items[0];
> +  if (firstAddedAttachment) {
> +    if (!(bucket.currentIndex > -1)) {

!(x > -1) is x < 0, no?

@@ +4582,5 @@
> +  } else if (attachmentsCount() > 0) {
> +    // We didn't succeed to add attachments (e.g. duplicate files),
> +    // but user was trying to; so we must at least react by ensuring the panel
> +    // is shown, which might be hidden by user with existing attachments.
> +    // ### To do: don't allow traceless hiding of pane with attachments.

Please make this XXX TODO:

@@ +4788,5 @@
>                                       .createInstance(Components.interfaces.nsIMutableArray);
>  
> +  while (bucket.getRowCount()) {
> +    let item = bucket.removeItemAt(bucket.getRowCount() - 1);
> +    if (item.attachment.size != -1) {

>= 0? Actually, nothing to update for == 0, so > 0? Oh, I see, that's existing code copied from below.

@@ +4802,5 @@
> +      if (item.uploading)
> +        item.cloudProvider.cancelFileUpload(file);
> +      else
> +        item.cloudProvider.deleteFile(file,
> +          new deletionListener(item.attachment, item.cloudProvider));

Interesting addition for cloud providers. Copied from below.

@@ +4828,4 @@
>   *
> + * @param aShowPane {boolean} true:    show the attachment pane
> + *                            false:   hide the attachment pane
> + *                            omitted: just update without changing pane visibility.

No full stops here (which I prefer)

@@ +4846,5 @@
> +
> +  attachmentBucketUpdateTooltip();
> +
> +  // If aShowPane argument is omitted, it's just updating, so we're done.
> +  if (arguments.length < 1)

== 0, right?

@@ +4856,5 @@
>  
>  function RemoveSelectedAttachment()
>  {
>    let bucket = document.getElementById("attachmentBucket");
> +  if (bucket.selectedItems.length < 1)

== 0, no?

@@ +4869,5 @@
> +                                     .createInstance(Components.interfaces.nsIMutableArray);
> +
> +  for (let i = bucket.selectedCount - 1; i >= 0; i--) {
> +    let item = bucket.removeItemAt(bucket.getIndexOfItem(bucket.getSelectedItem(i)));
> +    if (item.attachment.size != -1) {

>=0 or > 0, see above.

@@ +5185,5 @@
>  /**
>   * Show or hide the attachment pane.
>   *
> + * @param aShow {boolean} true:  Show the attachment pane.
> + *                        false: Hide the attachment pane.

Here you have full stops.

@@ +5329,5 @@
> +{
> +  // Show 'Attach File(s)' dialogue when clicking on attachment pane whitespace,
> +  // but only when there are no attachments selected (in which case we need to
> +  // clear selection first) or no attachments at all.
> +  if (attachmentsSelectedCount() < 1) {

== 0?

::: mail/components/compose/content/messengercompose.xul
@@ +1181,3 @@
>                            context="msgComposeAttachmentListContext"
>                            itemcontext="msgComposeAttachmentItemContext"
> +                          onclick="attachmentBucketOnClick(event);"

You really want to do this? You can really easily click to deselect and you're hit with the file dialogue. I noticed that when trying the previous patches and found it really annoying.
Attachment #8940534 - Flags: review?(jorgk) → feedback+
Created attachment 8940551 [details] [diff] [review]
1428631_backend_composeAttachmentPaneUX.patch

This is the first version with styles for Linux and Windows (and the correct bug # in the name ;) ).

Yes, please let's stay with double click to open the picker. When I click into the bucket to focus it, the picker appears immediately and this is a bit distracting.

I'm not so happy with the "Focus Attachment Pane". It sets the focus, that's true but this menu is for showing/hiding elements. Focus isn't something we need to expose, but show/hide. Let it behave like the "Contacts Sidebar" and use only "Attachment Pane" with the checkmark. Unticking the item hides the pane like you can do already with sliding the splitter.
Attachment #8940534 - Attachment is obsolete: true
Attachment #8940534 - Flags: ui-review?(richard.marti)
Attachment #8940551 - Flags: feedback+

Comment 5

10 days ago
(In reply to Richard Marti (:Paenglab) from comment #4)
> ... Unticking the item hides the
> pane like you can do already with sliding the splitter.
Anything is cool as long as I get to keep the (X) to close it (like the contacts sidebar has).
(In reply to Jorg K (GMT+1) from comment #5)
> (In reply to Richard Marti (:Paenglab) from comment #4)
> > ... Unticking the item hides the
> > pane like you can do already with sliding the splitter.
> Anything is cool as long as I get to keep the (X) to close it (like the
> contacts sidebar has).

The empty attachment pane has it too. And with attachment it's closeable with the splitter.
Created attachment 8940552 [details] [diff] [review]
1428631_backend_composeAttachmentPaneUX.patch

Added the Mac styles.
Attachment #8940551 - Attachment is obsolete: true
(Assignee)

Comment 8

10 days ago
(In reply to Jorg K (GMT+1) from comment #3)
> Review of attachment 8940534 [details] [diff] [review]:
> First pass through. This is nice. I don't like that you changed the
> double-click to single-click, that will annoy many people, so please
> consider changing that back.

Hmmm, no... There's a fundamental misunderstanding of history here (and possibly misunderstanding of patch behaviour):

- The *current* behaviour (of release versions since time immemorial) is *single-click* (on attachment pane whitespace to add attachments). So it will definitely annoy many people if we take that away from them and force them into double-clicks.
- It was me who changed this to double-click in bug 663695 in an effort to allow single-click-deselection. Now I've *restored* the single-click, but smarter than before:
- The smart tweak: Keep the simple single-click. But IF there's a selection, the first single click will deselect (as expected and required by standards), and the second single click will attach. So we're having the best of both worlds, and a perfectly correct, predictable, and simple behaviour. And dynamic tooltips will tell you exactly what your next click is going to do: Clear Selection | Attach file(s).
- If you really prefer to double-click (new behaviour never seen before in TB), that's totally up to you and still works with my patch (you'll just get a beep if there's no selection, but it still works).

> You really want to do this? You can really easily click to deselect and you're hit with the file dialogue.

No, I'm *fixing* exactly that problem of the current behaviour, so that can never happen with my patch.
Current behaviour (TB release): Single-click to deselect unexpectedly pops up attach dialog (i.e. single-click to clear selection is NOT possible, which is a bug in terms of ux-consistency and ux-error-prevention).
New behaviour (my patch): Single-click to deselect will deselect as expected (and is even advertised in whitespace tooltip). Obviously when you'd try to deselect when there's no selection in the first place, that would be a logical error on your side (and I don't see how or why anyone would try that).

> I noticed that when trying the previous patches and found it really annoying.

Which implies that you've never clicked on whitespace before, because it's the *current* behaviour which is annoying and incorrect. Again, I am *fixing* single-click-deselection so that it works, for the first time ever.

(In reply to Richard Marti (:Paenglab) from comment #4)
> Yes, please let's stay with double click to open the picker.

Dito, wrong history. If you want to change the current release version behaviour by introducing double-click, pls file a bug and face angry users there...

> When I click into the bucket to focus it, the picker appears immediately and this is a
> bit distracting.

Distracting??? What's your next action after focusing an empty bucket??
There's really not much you could do with an empty bucket, except, well - to add attachments? So we're saving an extra step here for the most common scenario. With existing attachments, there's not much reason either to click on the whitespace just to focus the bucket. Again, what's your next step after that? For almost every action, you would typically use context menu straight, or select first, then context menu. Or to start from the attach button straight in case of adding. Focus first with existing attachments looks like a rare scenario to me. But I've also made it easier than ever before to just focus attachment bucket:
- clicking anywhere in the attachment header will focus attachment pane (clicking on labels always focuses the associated control, that's default behaviour, but for the rest of the header, it's courtesy, made discoverable with tooltip).
- cmd_focusAttachmentPane (to show AND focus the pane), also conveniently available via Alt+M (Ctrl+M on Mac), and from view menu, and from the attach button dropdown in the future.

To put it positively:
Starting from an empty or half-empty attachment bucket, there's nothing more efficient and convenient than just a single click on the very large click target of whitespace to add attachments, right where you want them (as in current release behaviour). I will definitely not be the one to remove that existing convenience. I'd rather expose it even more, e.g. by adding a background text or image to empty whitespace, in addition to the dynamic tooltip which I've added.
(Assignee)

Comment 9

10 days ago
Oh btw, the other reason why I'm very much against enforcing (not allowing) double-click to add attachments, is that it's totally undiscoverable and counterintuitive, because the inherent logic of double-click is that first click selects and second click does default action. On attachment whitespace, there's nothing to select, so there's no reason to double-click, at all. Hence such behaviour has never been found in any application. Whereas single-clicking (tapping) on a spot where you want to add something is a well-known UX pattern from (desktop and) mobile devices.
(In reply to Richard Marti (:Paenglab) from comment #4)

> I'm not so happy with the "Focus Attachment Pane". It sets the focus, that's
> true but this menu is for showing/hiding elements. Focus isn't something we
> need to expose, but show/hide. Let it behave like the "Contacts Sidebar" and
> use only "Attachment Pane" with the checkmark. Unticking the item hides the
> pane like you can do already with sliding the splitter.

Hmmm, I hear where you're coming from and I was considering that aspect, but after some more thinking I concluded that it's a bad idea to have a simple show/hide for attachment pane in the current dispensation (but in the future, with a different layout, we can try that again).

1) I think we should consider preventing the current use pattern where user can make the entire attachment pane with existing attachments disappear without any trace in the UI. That's a massive violation of ux-error-prevention, because some hours or days later, you can think that there are no attachments on that message, whilst they are still there, but completely hidden. That's how you leak sensitive data or the wrong files, or maybe just files which you didn't want to send any more because something has changed about the real situation.

2) Collapsing the panel with existing attachments would require a visible trace ("N attachments") somewhere in the UI, which we don't have yet. Some possibilities:
a) Morphing attach button label into "N attachments" when pane is hidden (requires button on the right). That's slightly odd because I think it will be hard/odd to add a twisty on the toolbar, too. We end up creating a bulky and bad-looking variant of the fully integrated solution b), unless if we try c).
b) Remove Attach button, morph entire button functionality into the pane header (as I did in the Bug 1427092), add a twisty to toggle the bucket only (as in message reader), but keep the pane *header* always visible, which requires reorganizing the box model to collapse just the bucket, but not the header. I believe that might work pretty well, but I suspect it's more work, so I'm not keen for now. Moving the attach button to the right also looks like a good interim step for our users to warm up with the in-place UI, before going all the way and having a smart button only on the pane header.
c) As a variant of b), we could scrap the existing pane header and make the last part of the toolbar become the pane header, smart morph button with dynamic label but just a bit higher on the toolbar. Needs some more technical trickery again, but might work, too.

For the morphing button in b) or c), these are the faces:

(>)  [# Attach        | v]   with no attachments, pane hidden (twisty in front; # = paperclip icon)
(v)  [# 3 Attachments | v]   with 3 attachments, pane shown
(>)  [# 3 Attachments | v]   with 3 attachments, pane hidden
(>/v)[# Attach        | v]   when hovering |3 Attachments | button part, label changes to "Attach" (with or w/o panel shown)


3) So with the current incomplete UI layout, I'm strictly against encouraging/allowing the user to hide a full bucket.

4) I'm adding "cmd_focusAttachmentPane" to the view menu not mainly for focusing, but for
- exposure (menu is better to expose Alt+M keyboard shortcut)
- showing the pane when it's hidden (either empty, or full and hidden by user)
Did you notice that when the panel is hidden, the menu label actually changes to "Show Attachment Pane"? So I think it's exactly right as it is, and we shouldn't change it unless paired with more comprehensive (and more labour-intensive) changes of the overall UI as described above.
(In reply to Thomas D. (currently busy elsewhere) from comment #10)
> (In reply to Richard Marti (:Paenglab) from comment #4)
> 
> > I'm not so happy with the "Focus Attachment Pane". It sets the focus, that's
> > true but this menu is for showing/hiding elements. Focus isn't something we
> > need to expose, but show/hide. Let it behave like the "Contacts Sidebar" and
> > use only "Attachment Pane" with the checkmark. Unticking the item hides the
> > pane like you can do already with sliding the splitter.

TLDR:
- Currently possible feature of "hide full bucket without trace" violates ux-error-prevention, that's a wrong/incomplete implementation.
- That wrong implementation prevents us from having full, traditional "Show/Hide Attachment Pane" on the view menu, and causes a minor tweak of only having "Show/Focus Attachment Pane", i.e. excluding "Hide" (minor ux-implementation-level) for ux-error-prevention.
- However, that tweak is actually useful, as it exposes "Focus Attachment Pane" functionality and shortcut, whereas hiding the full bucket without trace is an unlikely and undesirable action atm.
- Unfortunately, the tweak also prevents us from having "Hide attachment pane" for *empty* bucket (somewhat rare scenario after 'Remove all attachments'), which might be useful for menu completeness. But imo, ux-error-prevention weighs more.
(In reply to Jorg K (GMT+1) from comment #3)
> Comment on attachment 8940534 [details] [diff] [review]
> Review of attachment 8940534 [details] [diff] [review]:
> There are a few nits: I'm really confused by your comparisons. For a
> "natural" number, < 1 means == 0, no? So if you want to express "no
> attachment" that's == 0

Personally, I'd also prefer == 0, but when I did that before, Aceman always changed it to < 1. Maybe he can explain why?

>, or perhaps <= 0, if for some reason the count can
> be negative.

Maybe that's the idea, to be on the safe side if for some reason (or in the future) those functions might return -1.

> > +        if (attachmentsSelCount > 1 &&
> > +            attachmentsSelCount < attachmentsCount()) {
> > +          // Sort selected attachments only, which needs at least 2 of them :),
> 
> What's the smiley doing here?

Ah, why being pedantic, are developers not allowed to smile in a comment?
The reason why I smile here it's because it should be obvious that you need at least 2 items to sort something, but then maybe sometimes the obvious isn't obvious (as in "vor lauter Bäumen den Wald nicht gesehen", didn't see the forest for the trees).

> @@ +4567,5 @@
> > +  // currently we don't indicate focus on the listbox itself, assuming
> > +  // there'll always be a focused attachment.
> > +  let firstAddedAttachment = items[0];
> > +  if (firstAddedAttachment) {
> > +    if (!(bucket.currentIndex > -1)) {
> 
> !(x > -1) is x < 0, no?

No. As explained in the comment, some platforms return undefined when there's no focused item, so we have to catch that, too, hence the negation. !(undefined > -1) == true     !(-1 > -1) == true

> > +    if (item.attachment.size != -1) {
> 
> >= 0? Actually, nothing to update for == 0, so > 0? Oh, I see, that's existing code copied from below.

Yes, that's existing code.
We could change it if we're sure, but does it really help?
Because on the other hand, != -1 works and provides a useful hint that attachment sizes can be that, which isn't obvious. If we have someone like you seeing the new condition > 0, he can say, why do we need this condition, sizes can only be 0 or positive, so we can dump the condition and just subtract 0 or positive numbers, which will backfire for -1 as we'll end up subtracting that which means adding 1.

> @@ +4802,5 @@
> > +      if (item.uploading)
> > +        item.cloudProvider.cancelFileUpload(file);
> > +      else
> > +        item.cloudProvider.deleteFile(file,
> > +          new deletionListener(item.attachment, item.cloudProvider));
> 
> Interesting addition for cloud providers. Copied from below.

Yes, just copied, without any testing.
As I said elsewhere, i would be wise for us to test cloud attachments with our new attachment stuff here, but I can't do it.
Should be something for more test cases.

> @@ +4828,4 @@
> >   *
> > + * @param aShowPane {boolean} true:    show the attachment pane
> > + *                            false:   hide the attachment pane
> > + *                            omitted: just update without changing pane visibility.
> 
> No full stops here (which I prefer)

Thank you. Finally someone with understanding of grammar.

> @@ +4846,5 @@
> > +
> > +  attachmentBucketUpdateTooltip();
> > +
> > +  // If aShowPane argument is omitted, it's just updating, so we're done.
> > +  if (arguments.length < 1)
> 
> == 0, right?

I'm not against it, but ask Aceman.

> @@ +4856,5 @@
> >  
> >  function RemoveSelectedAttachment()
> >  {
> >    let bucket = document.getElementById("attachmentBucket");
> > +  if (bucket.selectedItems.length < 1)
> 
> == 0, no?

dito.

> @@ +4869,5 @@
> > +                                     .createInstance(Components.interfaces.nsIMutableArray);
> > +
> > +  for (let i = bucket.selectedCount - 1; i >= 0; i--) {
> > +    let item = bucket.removeItemAt(bucket.getIndexOfItem(bucket.getSelectedItem(i)));
> > +    if (item.attachment.size != -1) {
> 
> >=0 or > 0, see above.

Not necessarily helpful to touch it, see above.

> @@ +5185,5 @@
> >  /**
> >   * Show or hide the attachment pane.
> >   *
> > + * @param aShow {boolean} true:  Show the attachment pane.
> > + *                        false: Hide the attachment pane.
> 
> Here you have full stops.

I was trained by aceman and others to do that, regardless of grammar :)
Flags: needinfo?(acelists)

Comment 13

9 days ago
< 1 is used for properties that may return negative (e.g. -1), e.g. selectedIndex of some XUL lists. For array.length that don't, you can use == 0.

The trailing full stops should be used for sentences.
Is "Hide the attachment pane." a complete sentence? I'd guess so, but Jorg may be a better expert here.
Flags: needinfo?(acelists)

Comment 14

9 days ago
I haven't looked at comment #8 and below, but let me just clarify this here.
(In reply to :aceman from comment #13)
> < 1 is used for properties that may return negative (e.g. -1), e.g.
> selectedIndex of some XUL lists. For array.length that don't, you can use ==
> 0.
I don't understand this. OK, -1 might mean "no index set", so < 1 is equivalent to <= 0 and that then includes 0 and -1, so index is first or no index. I don't see why it would ever make sense to lump those two together.

Looking at my complaints above we have:
if (!(bucket.currentIndex > -1)) {
that should be < 0 for "no index". Oh, I see, it can be undefined, so why not join that with an or?
At the very least !(xx >= 0) since valid indices start at 0.

if (arguments.length < 1)
That can be == 0, since length doesn't come back negative.

if (attachmentsSelectedCount() < 1) {
That can be == 0, since I assume the function doesn't return a negative count.

You convinced me about if (item.attachment.size != -1) { ;-)

> The trailing full stops should be used for sentences.
> Is "Hide the attachment pane." a complete sentence? I'd guess so, but Jorg
> may be a better expert here.
I don't care either way, but the patch has a both variants.

Comment 15

9 days ago
You got me for selectedIndex :)

It ugly that currentIndex would sometimes be undefined, but it seams real (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Property/listbox.currentIndex).
So can we proceed on this with improving, not removing the existing feature of "single-click-whitespace-to-attach"?

Click-to-deselect now works correctly and reliably (no interference), and apart from that it's highly unlikely to click the whitespace for just focusing the pane, and there are other, easier and always available ways to do that now (Alt+M, click on pane header, or "Focus Attachment Pane" from view menu). Whereas allowing the single click on whitespace to add files comes with a huge efficiency/convenience gain, and would probably cause user protests if we removed that. I'm also exposing this feature with a dynamic tooltip, for discovery and "no-surprise". And double-click also works, for those who want that. More detailed explanation in my comment 8 above.
Flags: needinfo?(richard.marti)
Flags: needinfo?(jorgk)
(In reply to :aceman from comment #13)
> < 1 is used for properties that may return negative (e.g. -1), e.g.
> selectedIndex of some XUL lists. For array.length that don't, you can use ==
> 0.
> 
> The trailing full stops should be used for sentences.
> Is "Hide the attachment pane." a complete sentence? I'd guess so, but Jorg
> may be a better expert here.

It's probably not a complete sentence in the way we use it here.
As a complete English sentence, it can only be 'imperative' form, which would typically get an exclamation mark: Trust me! However, i think our comments are not intended as 'imperatives', i.e. instructions to Thunderbird code base or the computer running the same. Try adding an exclamation mark and see that it's somewhat odd or freaky: "Computer, hide the attachment pane!" Ha-ha...! :)
It's much more likely that this is an 'infinitive', i.e. a non-conjugated verb form without an implicit person, just a neutral, person-less description of the action which happens at this spot. Unfortunately, English doesn't show the difference. German does:
> Hide the attachment pane! (imperative) = Verberge den Anhangsbereich!
> hide the attachment pane (infinitive)  = den Anhangsbereich verbergen

That said, I'll try to be more perfectly consistent next time...

Comment 18

9 days ago
(In reply to Thomas D. (currently busy elsewhere) from comment #16)
> So can we proceed on this with improving, not removing the existing feature
> of "single-click-whitespace-to-attach"?
Yes.

I confirmed that the current release version accepts a single click. Since I'm a Daily user, I've gotten used to the double-click, that's all. Now the first click deselects, the second one opens the attach dialogue. Fine.

As for the smiley: I don't think it's appropriate and I don't see the point in this case.
That said, I've landed a smiley the other day, but that was really funny ;-) (since it was referring to KISS):
https://hg.mozilla.org/comm-central/rev/13a022fef0c88df570f7fb3d0ba435cf5ac7df4d#l2.233

So please fix the nits and we're on the road again, assuming that Richard agrees.

Can you please also fix this:
- Add an attachment
- Slide the bucked to invisibility (I hate that that's possible, what a foot gun!)
- Alt+M doesn't bring the bucket back :-(
Flags: needinfo?(jorgk)
(In reply to Thomas D. (currently busy elsewhere) from comment #16)
> So can we proceed on this with improving, not removing the existing feature
> of "single-click-whitespace-to-attach"?

Yes, let it like it is.

I'm still against the "Focus Attachment Pane" or the "Show Attachment Pane". "Attachment Pane" is enough like the other menuitems. The underlined "M" in the attachment bucket title box is enough to inform the user what access key focuses the bucket. You could make the menuitem disabled, when the bucket has attachments and is not closed. Then the user can't close it with the menu. The menuitem disabling can be removed when we know how we could inform the user that there are attachments but the bucket is closed.

To inform the user, we could highlight the attachment button like the checked QFB button or change the button icon/text color. Or the bucket can only be closed to a minimal width of maybe 20px which shows the left parts of the attachment items (mostly the icons) to not miss that there are attachments.
Flags: needinfo?(richard.marti)
(In reply to Jorg K (GMT+1) from comment #18)

> So please fix the nits and we're on the road again, assuming that Richard
> agrees.

Thanks. 

> Can you please also fix this:
> - Add an attachment

What am I supposed to do with that?

> - Slide the bucked to invisibility (I hate that that's possible, what a foot
> gun!)

Yes, as I mentioned, with the current UI that's suicidal. I'll see what I can do, although we don't have to fix everything here as long as we fix something, preferably fast :)

> - Alt+M doesn't bring the bucket back :-(

That's a mysterious bug which I have no clue how to fix it, need to file. But it's there, and reproducable. Sometimes it does work, mostly after starting new msg and after using the cmd_focusAttachmentPane from menu. And/Or maybe it doesn't have collapsed property but just width=0, but I doubt that.
(Assignee)

Updated

9 days ago
Blocks: 1428977
You need to log in before you can comment on or make changes to this bug.