1428631 - Implement cmd_toggleAttachmentPane (Alt+M), cmd_removeAllAttachments, and polish attachment bucket UX/behaviour and keyboard access

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

Attachment: 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

Comment 1

[Jorg K (CEST = GMT+2)]

(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?

Comment 2

[Thomas D. (:thomas8)]

(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

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: Patch V.2 (V.1 + styles for Windows and Linux)

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.

Comment 5

[Jorg K (CEST = GMT+2)]

(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).

Comment 6

[Richard Marti (:Paenglab)]

(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.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: Patch V.2: including all styles (added Mac)

Added the Mac styles.

Comment 8

[Thomas D. (:thomas8)]

(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.

Comment 9

[Thomas D. (:thomas8)]

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.

Comment 10

[Thomas D. (:thomas8)]

(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.

Comment 11

[Thomas D. (:thomas8)]

(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.

Comment 12

[Thomas D. (:thomas8)]

(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 :)

Comment 13

[:aceman]

< 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.

Comment 14

[Jorg K (CEST = GMT+2)]

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

[:aceman]

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).

Comment 16

[Thomas D. (:thomas8)]

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.

Comment 17

[Thomas D. (:thomas8)]

(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

[Jorg K (CEST = GMT+2)]

(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 :-(

Comment 19

[Richard Marti (:Paenglab)]

(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.

Comment 20

[Thomas D. (:thomas8)]

(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.

Comment 21

[Jorg K (CEST = GMT+2)]

No action here for a month? The end for TB 60 is coming soon.

Comment 22

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #21)
> No action here for a month? The end for TB 60 is coming soon.

I've spent the last 3 days or so coding away around the clock on implementing the "Minimize attachment pane" (aka "avoid the footgun") feature requested by Jörg (and mentioned by me before, so I agree on that idea). One might think it's as simple as just adding min-width to the bucket - alas, FAR from it. This has proved to be a HELL of a lot of work redesigning the entire bucket state machinery (including boolean View Menu entry which only knows |shown vs. hidden|, worse), with DOZENS of devils in the detail. We don't want user to end up with a vertical scrollbar hiding his minimized bucket, but we also don't want weird empty spaces and cut-off attachment numbers and names in the bucket, and that's where it all starts...

I've just finished this but I'm stuck on a mysterious problem where bucket.boxObject.width reports wrongly as 180 whereas it's really minimized to 24px, causing "Restore Attachment Pane" from bucket header click to fail. I'll post a patch asap, then we need to iron this out together if we can.

The good news being that I've radically improved the overall efficiency and versatility of keyboard access to attachment bucket workflows including the new attachment bucket view state machinery, with some nifty max-efficiency shortcuts like "ESC" to close (focused) empty bucket, e.g. after removing all attachments...

Comment 23

[Thomas D. (:thomas8)]

Attachment: Patch V.3: New: Full view state machinery incl. minimized state; Alt+M cycle; headerclick-restore broken (see comment)

So here's to devils in hell, awesome, and broken.

Richard, please double-check the CSS for correctness and if it works as expected on all OS.

Jörg, here's what you get when you ask for "Minimize Attachment Pane" - happy review! ;)

Aceman, everyone: Please help me find a way out for the problem mentioned under "Broken". Was driving me mad.

For introductory remarks, see comment 22.

Devils in the detail:
Minimized Attachment Pane may or may not have a vertical toolbar, and depending on pane size, dragging either splitter (vertical/horizontal), may cause that vertical toolbar to appear. Note the literal "edge" case where vertical height is slightly bigger than all items, but not big enough for the horizontal toolbar. Minimize that, and you get a disabled vertical scrollbar because your horizontal scrollbar doesn't fit. Weird. Likewise, when all items fit vertically, and you start dragging the pane smaller, the vertical scrollbar might appear and would cover your icons in minimized state, which we don't want. Hence the need for significant efforts to make things look "good" no matter what you do to your minimized pane.

Broken: bucket.boxObject.width
(causing broken "Restore Attachment Pane"):
Kindly assist me to find a way out for the problem that bucket.boxObject.width quite randomly reports totally fictitious widths. Especially when bucket is minimized to min-width, bucket.boxObject.width is falsely reported to be 180px (the default width), and many other ways of getting the width report the same, but looking at the real bucket, it's only 24px slim. That's bad because it breaks "Restore Attachment Pane" from bucket header click, where I look at the real width to determine the next action. Moreover, looking at my console.logs, after minimizing all width variants are correctly reporting 41px, only to go back to 180px at the next header click (but I'm not aware of changing anything in the meantime). I'm totally failing to make sense of the misreported width, and I'm not seeing any problem in my code. Have I overlooked anything? Note that those widths are also cunning as they randomly come back as strings, although documentation claims integer for all of them. I tried to address that but I might have missed some. console.logs have some hints on that, too.

Awesome:
This patch (if we can get it to work) delivers a number of nifty UX improvements!

1.) New pane view state: minimized.
- Avoid the footgun of hiding full bucket.
- Smart enough to adjust to vertical scrollbar (dis)appearing as you resize your bucket - omg that was painful to code up!!!
- Clean mini-header just showing the number of attachments (to avoid odd-looking off-cuts), and quite fidgety to get that right in code. (Let me know about any glitches, but most likely it wasn't me).
- Choose your preferred way of getting in and out of that state - anything goes! (see below).
2.) Alt+M from body, hit Enter (on focused, empty bucket): Attach... (totally awesome!). Alt+M will summon the empty bucket if hidden.
3.) ESC (on focused, empty bucket): hide bucket.
4.) ESC (on focused, full bucket): Will first clear selection if present, then focus body. Get me out of attachment bucket for dummies :)
5.) Alt+M (focused, default-size bucket from 2.): Minimize and focus body. Another Alt+M: Restore and focus attachment pane. Perfect cycle. (Some glitches when focus is in small or minimized bucket, probably due to the boxObject width problem described above).

Fineprint:
1.) Ways of getting into minimized state:
- drag until min-width
- click on bucket header when it's default width (15 em) or more
- full bucket: uncheck View > Attachment Pane
- Alt+M with focus on bucket.
Ways of getting out of that state ("Restore Attachment Pane":
- drag pane open again
- click bucket header (currently broken)
- Check View > Attachment Pane
- Alt+M 
- focus bucket
- select attachment item (any selection change)
- click pane whitespace (will also deselect; I guess that's ok although my original idea was first click to expand, 2nd click to deselect).

2.) Bias for default width: Whichever way you're touching or accessing the minimized bucket, even a small bucket with less than default width (the old min-width of 15em where the bucket would suddenly collapse away), it will expand to default-width first. I don't believe in crystal balls, so working on attachment icons without name doesn't look like a useful modus operandi.

3.) cmd_toggleAttachmentPane: completely rewritten to accomodate the full view state machinery (bucket header clicks, menu access, and keyboard shortcuts).
- As menu knows only dual state (shown/hidden), we consider minimized bucket as "hidden".

4.) Lots of other small improvements in code and behaviour.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8950344 [details] [diff] [review]
Patch V.3: New: Full view state machinery incl. minimized state; Alt+M cycle; headerclick-restore broken (see comment)

I've tried this. Of course, as eating takes less time then cooking, trying takes less time than coding (and I guess the ratio is much worse than 1:3 that's assumed for eating:cooking).

In general, as I've said before, I like all the extra bucket functions, and the fact that you can't close it if it's full.

I don't want to appear unappreciative, but I'm sure I didn't check all the things covered in the fine print. However, I think, ESC, ESC to get you into the body seems a bit of an overkill. Unselect is fine, but that should be it.

Clicking onto the bucket header to minimise is unusual but effective.

So f+ from me.

BTW, what happened to positioning the "Attach" button on top of the bucket? I tried "Customise/Restore", but that didn't move it as before. I know there was the migration problem, but it might have been worth it.

Comment 25

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #24)
> So f+ from me.

Thank you!

> BTW, what happened to positioning the "Attach" button on top of the bucket?

I guess that's bug 1427092 territory. But you can always customize your toolbar to get it for yourself.

> I tried "Customise/Restore", but that didn't move it as before. I know there
> was the migration problem, but it might have been worth it.

It's definitely worth it, but I don't feel in a position atm to write migration code.

Comment 26

[Richard Marti (:Paenglab)]

Comment on attachment 8950344 [details] [diff] [review]
Patch V.3: New: Full view state machinery incl. minimized state; Alt+M cycle; headerclick-restore broken (see comment)

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

What work looks good. Please fix my comments in the next version.

::: mail/components/compose/content/messengercompose.xul
@@ -263,5 @@
>         command="cmd_renameAttachment"/>
>    <key id="key_reorderAttachments"
>         key="&reorderAttachmentsCmd.key;" modifiers="alt"
>         command="cmd_reorderAttachments"/>
> -#endif

Removing this #endif breaks Mac.

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +193,5 @@
>    border-top: none;
>  }
>  
>  #attachments-header-box {
> +  min-height: 24px;

Please change to 28px.

@@ +199,5 @@
>  }
>  
>  #attachmentBucketCount {
> +  padding-top: 4px;
> +  padding-bottom: 3px;

Remove this two paddings. Then it is always equally positioned like #attachmentBucketSize.

Comment 27

[Thomas D. (:thomas8)]

Attachment: Patch V. 4: Fast-track the working goodies we've agreed on (removed minimize/restore machinery)

As explained via PM, imo it's highly unlikely that we'll figure out the bucket.boxObject.width problem fast, so with this patch I want to fast-track landing only the safe parts that we've already agreed on and which are working.

Let's please try to get this landed asap. Tia.

I removed the entire min-width/minimize/restore state machinery; after landing the rest, we can try that again, which will then be easier as we can focus on the code which is actually needed for that (with a lot of unrelated code out of the way). I must admit after having been to the UX of minimizing, returning to the footgun of collapsing full bucket to nowhere was kinda painful, but I still think that's the best way forward rather than doing everything at once and getting nothing done...

Jörg, I'd think that changes compared to your first review are mostly cosmetic; I've updated the inner workings of functions like "toggleAttachmentPane()" a bit which will make it easier to try the full state machinery with min-width/minimizing again.

Richard, please double-check all CSS changes.

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #25)
> > BTW, what happened to positioning the "Attach" button on top of the bucket?
> I guess that's bug 1427092 territory. But you can always customize your
> toolbar to get it for yourself.
Sorry, I got confused by so many bugs.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8950909 [details] [diff] [review]
Patch V. 4: Fast-track the working goodies we've agreed on (removed minimize/restore machinery)

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

I spent more than an hour with this and found many nits. I don't really like the super-smart argument passing of toggleAttachmentPane().

I didn't look at the CSS.

On testing it: F9 summons and dismisses the contacts side bar. Alt+M only summons the bucket, but you can't get rid of it that way even when empty. Is that intentional? I noticed that the bucket completely collapses in a flash when its size goes under 180px. Maybe we could have a simple improvement, so at least it shrinks and doesn't collapse completely.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +688,5 @@
> +        }
> +
> +        // enable this command when the compose window isn't locked;
> +        // disable for full, visible bucket (effective for menu only; command's
> +        // shortcut key will still work via bucket's identical access key)

Nits: Start with upper case, end with full stop (always). "compose window locked"? Is that a feature I don't know?

@@ +693,5 @@
> +        return (!gWindowLocked && !(bucket.itemCount > 0 && bucketWidth > 0))
> +      },
> +      doCommand: function() {
> +        // For legacy completeness / addon purposes only (may not work as expected).
> +        // TB doesn't use this, because goDoCommand() doesn't handle events yet

Let's not add dead code. If you must, add a comment:
// No doCommand function provided since ... .

@@ +1451,5 @@
>  }
>  
> +function updateViewItems()
> +{
> +  console.log("updateViewItems()");

Remove console.log here and the seven others.

@@ +4555,5 @@
>      items.push(item);
>  
>      if (aCallback)
>        aCallback(item);
> +  } // end for

Nit: remove "end for". My editor tells me where I am and it's not current practise.

@@ +4825,4 @@
>   *
> + * @param aShowPane {string} "show":  show the attachment pane
> + *                           "hide":  hide the attachment pane
> + *                           omitted: just update without changing pane visibility

Full stop.

@@ +4885,5 @@
> +    }
> +
> +    removedAttachments.appendElement(item.attachment);
> +    // Let's release the attachment object held by the node else it won't go
> +    // away until the window is destroyed

Full stop (yes, was missing before).

@@ +5184,5 @@
> + * If aAction parameter is omitted, auto-cycling of view states, bias for "show".
> + * Note: Forcing "hide" is not recommended as it may violate ux-error-prevention
> + * for full bucket in the current UI layout.
> + * Note: Parameters are optional (passing aEvent is recommended where possible)
> + * and their order doesn't matter.

I don't find this so crash-hot. Why not pass this stuff conservatively?

@@ +5189,4 @@
>   *
> + * @param aAction {string} "show": show attachment pane
> + *                         "hide": hide attachment pane
> + *                         omitted: depending on aEvent, otherwise "show"

I don't understand "depending on aEvent". Its presence?

@@ +5195,4 @@
>   */
> +function toggleAttachmentPane() {
> +  let aEvent;
> +  let aAction;

aXXX refers to arguments, but you don't have any.

@@ +5209,5 @@
> +      aEvent = arg;
> +    }
> +    else
> +      aAction = arg;
> +  }

Is this really necessary? Can't it be done the old fashioned way?

@@ +5376,5 @@
>  
> +function attachmentBucketOnKeyPress(aEvent) {
> +  let bucket = GetMsgAttachmentElement();
> +
> +  // When ESC is pressed...

Nit: space before ...

@@ +5381,4 @@
>    if (aEvent.key == "Escape") {
>      let reorderAttachmentsPanel = document.getElementById("reorderAttachmentsPanel");
>      if (reorderAttachmentsPanel.state == "open") {
> +      // ...first close reorderAttachmentsPanel if open

Nit: space after ... . Full stop missing.

@@ +5385,4 @@
>        reorderAttachmentsPanel.hidePopup();
> +    } else if (bucket.itemCount) {
> +      if (bucket.selectedCount) {
> +        // ... then deselect items in full bucket

Full stop.

@@ +5387,5 @@
> +      if (bucket.selectedCount) {
> +        // ... then deselect items in full bucket
> +        bucket.clearSelection();
> +      } else {
> +        // ... and unfocus full bucket to continue with msg body

Full stop.

@@ +5398,5 @@
> +  }
> +
> +  if (aEvent.key == "Enter" && bucket.itemCount == 0) {
> +    // Enter on empty bucket to add file attachments, convenience
> +    // keyboard equivalent of single-click on bucket whitespace

Full stop.

@@ +5402,5 @@
> +    // keyboard equivalent of single-click on bucket whitespace
> +    goDoCommand('cmd_attachFile');
> +  }
> +
> +  // e.g. on Windows: check if (aEvent.key == "m" && aEvent.altKey)

Huh? Code in the comment?

@@ +5413,5 @@
> +function attachmentBucketOnClick(event)
> +{
> +  // Handle click on attachment pane whitespace:
> +  // - with selected attachments, clear selection first
> +  // - otherwise, e.g. on a plain empty bucket, show 'Attach File(s)' dialogue

Full stop. Sadly Mozilla code is US English, so 'dialog'.

@@ +5431,5 @@
> +function attachmentBucketUpdateTooltips() {
> +  let bucket = document.getElementById("attachmentBucket");
> +  let bucketHeader = document.getElementById("attachments-header-box");
> +
> +  // attachment pane whitespace tooltip

Upper case, full stop, well, even though there's no full sentence.

@@ +5448,5 @@
> +
> +function attachmentBucketSizerOnMouseUp() {
> +  updateViewItems();
> +  if (GetMsgAttachmentElement().boxObject.width == 0) {
> +    // If user collapsed the attachment pane, move focus to msg body.

Is there really much gain in the abbreviation 'msg' instead of 'message' in a comment?

@@ +6144,5 @@
>        return;
>      }
>  
>      if (aFlavour.contentType != "text/x-moz-address") {
> +      // make sure the attachment pane is visible during drag over

Upper case, full stop.

Comment 30

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #29)
> Comment on attachment 8950909 [details] [diff] [review]
> Patch V. 4: Fast-track the working goodies we've agreed on (removed
> minimize/restore machinery)
> 
> Review of attachment 8950909 [details] [diff] [review]:
> -----------------------------------------------------------------
> I spent more than an hour with this and found many nits.
> I don't really like the super-smart argument passing of toggleAttachmentPane().

Thank you very much Jörg for your fast and thorough review, crossing the t's and dotting the i's, I know and appreciate that you're going out of your way and often working late to keep things moving. Consider making the first sentence or even just the first word of your review sound appreciative (e.g.: "Thanks!"); that would go a long way to set a more positive tone of communication, given that you really only found two purely cosmetic and intentional structural style "nits" and are on record to like all the rest of my patch.

- a deliberate comment style decision of mine to not capitalize or put full stop for comments that aren't sentences (in line with English grammar)
- a deliberate decision to leave console.logs in the code so that your review would be easier (no full stop)

> On testing it: F9 summons and dismisses the contacts side bar. Alt+M only
> summons the bucket, but you can't get rid of it that way even when empty. Is
> that intentional?

Yes. I was trying to include "close empty bucket" in the view state cycle, but concluded that it doesn't work well:

Full bucket:
1. Alt+M -> summon, restore, and focus bucket.
2. Alt+M -> Minimize bucket.
3. Alt+M -> same as 1.

Empty bucket:
1. Alt+M -> summon, restore, and focus bucket.
2. Alt+M -> Minimize bucket. (consistent with the full bucket behaviour, and useful imo - ymmv)

Now here's where we would close it, but then we can't. If user has just minimized (i.e. explicitly NOT closed) the empty bucket, that means we must assume he is going to use it again. So for the next Alt+M we must err in favor of restoring the minimized empty bucket.

3. Alt+M -> same as 1.

If we wanted to include "close empty bucket" into the Alt+M cycle, we'd have to skip "minimize" for empty bucket, I'm not sure if that's better. So far, I've considered Alt+M more like an access key rotating the "visible" view states (restored/minimized) without closing the pane. I assume users may want to minimize empty bucket to add stuff later without having a ghastly white gap in the UI. I think adding attachments to an empty (minimized) bucket is much more likely than closing it. Did you try ESC to close the empty bucket? I think that's quite powerful, easy, intuitive, and sufficient.

Unfortunately either way we can never have complete consistency, and we'll always violate someone's legitimate expectations. Empty bucket is always a different animal, and it's hard to tell if users want to keep it or close it.

> I noticed that the bucket completely collapses in a flash
> when its size goes under 180px.

Yes, that's the current behaviour which I haven't changed. My plan was to land everything else before we start meddling with that.

> Maybe we could have a simple improvement, so
> at least it shrinks and doesn't collapse completely.

Not simple unfortunately, allowing continuous shrinking immediately requires reinstating the full state machinery, listeners and all, which I just removed because it still has flaws. Please read my previous comments where I explain exactly why allowing users to minimize the bucket is where the whole trouble starts as we can't let them run into odd and useless UI states where vertical scrollbar covers all icons in your minimized bucket.

I suggest that we first land this incremental improvement over the status quo and improve anything else later in a fresh and clean start, which will also reduce the burden on reviewers as we can focus on the state machinery without distraction of dozens of peripheral, unrelated changes.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +688,5 @@
> > +        }
> > +
> > +        // enable this command when the compose window isn't locked;
> > +        // disable for full, visible bucket (effective for menu only; command's
> > +        // shortcut key will still work via bucket's identical access key)
> 
> Nits: Start with upper case, end with full stop (always).

Oh, and I thought last time you were giving me a flow as I pointed out that these aren't full sentences...
But if we agree to consistently violate English grammar for the sake of uniform appearance, I guess that's ok with me. Admittedly, I've seen so many capitalized, full-stop comments which aren't sentences that I'm also kinda expecting that as the "correct" style...

> "compose window locked"? Is that a feature I don't know?

No, it's a state of the compose window realized in gWindowLocked variable; true after clicking send button I guess, when your compose window is still around but you mustn't touch it until the message has been sent.

> @@ +693,5 @@
> > +        return (!gWindowLocked && !(bucket.itemCount > 0 && bucketWidth > 0))
> > +      },
> > +      doCommand: function() {
> > +        // For legacy completeness / addon purposes only (may not work as expected).
> > +        // TB doesn't use this, because goDoCommand() doesn't handle events yet
> 
> Let's not add dead code. If you must, add a comment:
> // No doCommand function provided since ... .

It's not exactly dead code, it's very much alive for most commands except those that require |event| as a parameter. I think the way commands are constructed, *every* command must have its own doCommand() function, so that the command can be run with goDoCommand("cmd_id"). goDoCommand is actually the preferred generic method for running commands, because it allows more abstraction in the XUL layer, leaving the entire command handling to the js layer (say if you're replacing/renaming the function attached to the command, you won't have to dig back into the XUL layer). Given that, I think add-ons can also rightly assume that every command can be run with goDoCommand(). The real problem here is not having the doCommand() function on the command, it's the shortcoming of goDoCommand which doesn't pass event (bug 959494). From what I saw, omitting this can even break the entire controller, as there are iterating functions on the controller which assume the presence of all command functions on every command. So omitting this is a no-go.

> @@ +1451,5 @@
> >  }
> >  
> > +function updateViewItems()
> > +{
> > +  console.log("updateViewItems()");
> 
> Remove console.log here and the seven others.

Yes, I intententionally left them in for your convenience in the review phase, and also for myself until we're really and fully done here. So will remove them at the last iteration.

> @@ +4555,5 @@
> >      items.push(item);
> >  
> >      if (aCallback)
> >        aCallback(item);
> > +  } // end for
> 
> Nit: remove "end for". My editor tells me where I am and it's not current
> practise.

Sure. I just thought it's helpful because the opening bracket is miles away and there are plenty of other conditionals in between, plus there's another un-bracketed if in the previous row, so someone might waste their time trying to figure where this lonely bracket belongs, or even attempt to move it into a wrong indentation level. I'm not planning to make that default practice, but imho sometimes a more pragmatic approach to comments might be helpful... Even your review time is too precious for that, don't you think?

> @@ +4825,4 @@
> >   *
> > + * @param aShowPane {string} "show":  show the attachment pane
> > + *                           "hide":  hide the attachment pane
> > + *                           omitted: just update without changing pane visibility
> 
> Full stop.

On this one I'm really sure that we had discussed this and you'd explicitly given me a flow, so now I'm confused.

> @@ +4885,5 @@
> > +    }
> > +
> > +    removedAttachments.appendElement(item.attachment);
> > +    // Let's release the attachment object held by the node else it won't go
> > +    // away until the window is destroyed
> 
> Full stop (yes, was missing before).

Indeed. Wasn't me, but needs a full stop, as it's a full sentence.

> @@ +5184,5 @@
> > + * If aAction parameter is omitted, auto-cycling of view states, bias for "show".
> > + * Note: Forcing "hide" is not recommended as it may violate ux-error-prevention
> > + * for full bucket in the current UI layout.
> > + * Note: Parameters are optional (passing aEvent is recommended where possible)
> > + * and their order doesn't matter.
> 
> I don't find this so crash-hot. Why not pass this stuff conservatively?

We don't have to be conservative every time, do we? I've tried passing it conservatively, and it sucked, because mostly when I need the event I don't need the action, and when I need the action I don't need the event. So conservatively, you end up with a lot of ugly, hard-to-read, and needlessly complex function calls which require either "null" placeholders for missing event argument, or "" argument for missing action (and due to the cyclic nature of the function, you really can't always specify an action). Ymmv, but I definitely prefer lean and simple callers (easy to read, easy to write) over conservative argument passing. Nothing wrong with that, and I don't think it's hard to understand inside the function with only two possible arguments, and a one-liner to tell them apart.

Simple callers achieved by flexible argument handling:

> toggleAttachmentPane("show");
> toggleAttachmentPane(event);

Complex, hard-to-read, hard-to-write callers with conservative argument passing:

> toggleAttachmentPane(null, "show")  - or -
> toggleAttachmentPane("", event)


> @@ +5189,4 @@
> >   *
> > + * @param aAction {string} "show": show attachment pane
> > + *                         "hide": hide attachment pane
> > + *                         omitted: depending on aEvent, otherwise "show"
> 
> I don't understand "depending on aEvent". Its presence?

Short for "if omitted, the result will depend on presence/value of aEvent, and otherwise default to 'show'". I think less is more. We can't have a full explanation here, but I don't think anyone cares because it just works, with lean and simple callers as described above.

> @@ +5195,4 @@
> >   */
> > +function toggleAttachmentPane() {
> > +  let aEvent;
> > +  let aAction;
> 
> aXXX refers to arguments, but you don't have any.

Well, I'm passing arguments into the function, I'm just retrieving them differently, using arguments object. Apart from that, there's no difference to named-and-declared arguments at all, so I think it would be much more confusing if we don't name these arguments like normal arguments.

> @@ +5209,5 @@
> > +      aEvent = arg;
> > +    }
> > +    else
> > +      aAction = arg;
> > +  }
> 
> Is this really necessary? Can't it be done the old fashioned way?

Only at the price of complex and ugly callers, see above.
But I'm failing to see any problem here, it's pretty straight forward: Two optional arguments, for caller's convenience we allow skipping any one which isn't needed, and here we tell them apart. That's just smart, as you said.

> @@ +5376,5 @@
> >  
> > +function attachmentBucketOnKeyPress(aEvent) {
> > +  let bucket = GetMsgAttachmentElement();
> > +
> > +  // When ESC is pressed...
> 
> Nit: space before ...

Well, that seems to be a matter of opinion and consistency in style rather than correctness.
https://english.stackexchange.com/questions/26240/what-is-the-proper-way-of-using-triple-dots-and-spaces-before-after-them
But I'll just put those spaces in to make you happy ;-)
For coders, spaces also helpful to avoid misreading as spread operator :-))

> @@ +5381,4 @@
> >    if (aEvent.key == "Escape") {
> >      let reorderAttachmentsPanel = document.getElementById("reorderAttachmentsPanel");
> >      if (reorderAttachmentsPanel.state == "open") {
> > +      // ...first close reorderAttachmentsPanel if open
> 
> Nit: space after ... . Full stop missing.

Not really, I was trying to create a multi-part comment which stretches across several conditions but can be read as one continuous sentence. Sorry for that outburst of creativity. For Jörg's peace of mind, maybe we should just omit all the ellipses, capitalize the beginning of the line, and put a full stop on each, so that we keep conservative style... (no space before ellipsis required, as can be considered punctuation which is formally no different from a full stop or comma).

> @@ +5385,4 @@
> >        reorderAttachmentsPanel.hidePopup();
> > +    } else if (bucket.itemCount) {
> > +      if (bucket.selectedCount) {
> > +        // ... then deselect items in full bucket
> 
> Full stop.

Dito.

> @@ +5387,5 @@
> > +      if (bucket.selectedCount) {
> > +        // ... then deselect items in full bucket
> > +        bucket.clearSelection();
> > +      } else {
> > +        // ... and unfocus full bucket to continue with msg body
> 
> Full stop.

Dito.

> @@ +5398,5 @@
> > +  }
> > +
> > +  if (aEvent.key == "Enter" && bucket.itemCount == 0) {
> > +    // Enter on empty bucket to add file attachments, convenience
> > +    // keyboard equivalent of single-click on bucket whitespace
> 
> Full stop.

Not a full sentence (just a noun phrase), but I'll put the full stop anyway if we agree on making the ungrammatical our default style. Or actually, no, in this case I won't, because even Jörg would probably NOT do this:

// Enter.

And my comment is structurally just that, a single noun ("Enter"), extended by some attributes/appositions, whatever they are. You really want a full stop after that?

> @@ +5402,5 @@
> > +    // keyboard equivalent of single-click on bucket whitespace
> > +    goDoCommand('cmd_attachFile');
> > +  }
> > +
> > +  // e.g. on Windows: check if (aEvent.key == "m" && aEvent.altKey)
> 
> Huh? Code in the comment?

Oops, sorry, that was a leftover comment from when the code below was cryptic enough to justify making it explicit with the net effect code in the comment:

>  let matchingModifiers = false;
>  keyModifiersArray.forEach(modKey => {
>    if (!(modKey == "access" || modKey == "any")) {
>      matchingModifiers = aEvent.getModifierState(modKey);
>      if (!matchingModifiers)
>        break;
>    }
>  });
>  // e.g. on Windows: check if (aEvent.key == "m" && aEvent.altKey)
>  if (aEvent.key == keyKey && matchingModifiers) {
>    attachmentBucketEnsureDefaultWidth();
>  }

Fortunately, it turned out that this wasn't needed.

> @@ +5413,5 @@
> > +function attachmentBucketOnClick(event)
> > +{
> > +  // Handle click on attachment pane whitespace:
> > +  // - with selected attachments, clear selection first
> > +  // - otherwise, e.g. on a plain empty bucket, show 'Attach File(s)' dialogue
> 
> Full stop. Sadly Mozilla code is US English, so 'dialog'.

Ah well, true, but does that really matter?

> @@ +5431,5 @@
> > +function attachmentBucketUpdateTooltips() {
> > +  let bucket = document.getElementById("attachmentBucket");
> > +  let bucketHeader = document.getElementById("attachments-header-box");
> > +
> > +  // attachment pane whitespace tooltip
> 
> Upper case, full stop, well, even though there's no full sentence.

Any style guide describing those?
That would look quite heavy, odd, and ungrammatical, don't you think?
I think we could afford some freedom in comments... and I'm a person who really cares about correct style and spelling and all... but sometimes a light-weight comment just seems much more appropriate, even more readable. That's the main point of comments, they should be readable. Not just brute-force following some random style scheme.

> @@ +5448,5 @@
> > +
> > +function attachmentBucketSizerOnMouseUp() {
> > +  updateViewItems();
> > +  if (GetMsgAttachmentElement().boxObject.width == 0) {
> > +    // If user collapsed the attachment pane, move focus to msg body.
> 
> Is there really much gain in the abbreviation 'msg' instead of 'message' in
> a comment?

Is there really much gain in writing it out? Who cares? But there's a gain in faster coding and just getting things done. Otoh, if it disturbs the reviewer, that's also a minimal net loss of productivity...
I'll write it out.

> @@ +6144,5 @@
> >        return;
> >      }
> >  
> >      if (aFlavour.contentType != "text/x-moz-address") {
> > +      // make sure the attachment pane is visible during drag over
> 
> Upper case, full stop.

Dito. Technically, not a sentence, but I'll just follow the unpublished(?) default style, regardless of ... whatever.

Comment 31

[Thomas D. (:thomas8)]

Attachment: Patch V. 4.1: (nitfixes) Fast-track working goodies w/o view state machinery

Jörg: nits fixed.

Richard: Pls double-check CSS.

Note that this is not necessarily the final UX but this is just incrementally improving the status quo.

Comment 32

[Richard Marti (:Paenglab)]

Only on Windows I saw the attachment title bar shrinks when the empty attachment bucket becomes an attachment. I'll look later into this. Maybe we need like Linux and Mac a min-height. I don't want adding a padding on the labels to work better with bigger font sizes.

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951215 [details] [diff] [review]
Patch V. 4.1: (nitfixes) Fast-track working goodies w/o view state machinery

(In reply to Richard Marti (:Paenglab) from comment #32)
> Only on Windows I saw the attachment title bar shrinks when the empty
> attachment bucket [gets|receives] an attachment.
Yes, I can see that, too.

(In reply to Thomas D. from comment #30)
> Empty bucket:
> 1. Alt+M -> summon, restore, and focus bucket.
> 2. Alt+M -> Minimize bucket. (consistent with the full bucket behaviour, and
> useful imo - ymmv)
Hmm, that becomes a matter of taste. Repeated F9 will show/hide the contacts sidebar.
I'd show/hide an empty bucket with Alt+M and maximise/minimise a non-empty one.
As long as we don't have minimise, Alt+M on a non-empty bucket becomes a no-op.
Richard, any preference?

> Complex, hard-to-read, hard-to-write callers with conservative argument
> passing:
> > toggleAttachmentPane(null, "show")
I think this would work for me. And the second argument can be optional. Aceman, what do you think?

Comment 34

[Thomas D. (:thomas8)]

Attachment: Addon-Patch V.1 (on top of patch 4.1): Implement view state machinery incl. minimized/restore; headerclick-restore broken, but Alt+M restore works

Note: To be applied on top of patch attachment 8951215 [details] [diff] [review].

So here's my original view state machinery as an "addon" patch. See comment 23 for details, especially about what's broken.

Aceman, any idea why bucket.boxObject.width reports wrong values which don't match the real bucket width? And why does restoring (triggered by looking at current bucket width) work for Alt+M, but not for click on the header?

So from here we are now in a better position to play with other ideas for the view state machinery, e.g. what Jörg suggested that Alt+M should alternate between hiding and showing the bucket (i.e. hide when Alt+M is pressed with normal-size, already focused empty bucket).

Comment 35

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #33)
> Comment on attachment 8951215 [details] [diff] [review]
> (In reply to Thomas D. from comment #30)
> > Empty bucket:
> > 1. Alt+M -> summon, restore, and focus bucket.
> > 2. Alt+M -> Minimize bucket. (consistent with the full bucket behaviour, and
> > useful imo - ymmv)
> Hmm, that becomes a matter of taste. Repeated F9 will show/hide the contacts
> sidebar.
> I'd show/hide an empty bucket with Alt+M and maximise/minimise a non-empty
> one.

I'll try that in the next iteration of the addon patch.
Not that minimizing empty bucket will then be keyboard-inaccessible (which is wrong by definition, but maybe we could live with that). Btw all splitters are also keyboard-inaccessible, tough luck for people who depend on that access. Although these days you can even operate mouse with your keyboard...

> As long as we don't have minimise, Alt+M on a non-empty bucket becomes a
> no-op. Richard, any preference?

> > Complex, hard-to-read, hard-to-write callers with conservative argument
> > passing:
> > > toggleAttachmentPane(null, "show")
> I think this would work for me. And the second argument can be optional.

Both arguments are optional, and so far we're only using either one of them, never both. I'm still failing to see why you'd prefer to pass useless empty arguments into the function just to preserve the traditional way of reading them into the function, when it only takes one straightforward conditional to tell them apart inside the function. I'd also think sometimes code contributors should be given a little bit of room for creativity / new patterns / own style if there's no real disadvantage.

> Aceman, what do you think?

Comment 36

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #34)
> Aceman, any idea why bucket.boxObject.width reports wrong values which don't
> match the real bucket width? And why does restoring (triggered by looking at
> current bucket width) work for Alt+M, but not for click on the header?

Maybe this bug could play into it?
https://bugzilla.mozilla.org/show_bug.cgi?id=101511

Comment 37

[Richard Marti (:Paenglab)]

Comment on attachment 8951215 [details] [diff] [review]
Patch V. 4.1: (nitfixes) Fast-track working goodies w/o view state machinery

Looks good. Please add to the windows file this rule:

#attachments-header-box {
  min-height: 26px;
}

(In reply to Jorg K (GMT+1) from comment #33)
> As long as we don't have minimise, Alt+M on a non-empty bucket becomes a
> no-op.
> Richard, any preference?

A no-op is good. Unfortunately the user can still hide the bucket with attachments with the splitter but this is not the focus of this patch.

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #35)
> I'd also think sometimes code
> contributors should be given a little bit of room for creativity / new
> patterns / own style if there's no real disadvantage.
I don't necessarily agree. As boring as it may sound, we do not want any special patterns, we want it all to be uniform since that's easier to maintain. Besides, there are other variations on your theme:
You could do
  function foo(obj) and call with {}, {event: theEvent}, {flag: "show"} or {event: theEvent, flag: "show"}.
Or
  function foo(...args) where the parameters are collected into an array.
All this might have its place, but for the 2-argument function being discussed here, it's overkill. And we don't want one corner to look like variation a) and another like variation b).

Finally, I really dislike this:
  for (let arg of arguments) {
    if (arg.target) {
      aEvent = arg;
    }
    else
      aAction = arg;
  }

Comment 39

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #38)
> (In reply to Thomas D. from comment #35)
> > I'd also think sometimes code
> > contributors should be given a little bit of room for creativity / new
> > patterns / own style if there's no real disadvantage.
> I don't necessarily agree. As boring as it may sound, we do not want any
> special patterns, we want it all to be uniform since that's easier to
> maintain.

I totally understand and appreciate the value of uniformity in code. Yet ironically, in this particular case, 100% uniformity will actually make it *harder* to maintain, as many callers will have to pass in extra empty arguments in the correct order, instead of just passing in the one argument needed in the context, with no practical benefit at all.

> Besides, there are other variations on your theme:
> You could do
>   function foo(obj) and call with {}, {event: theEvent}, {flag: "show"} or
> {event: theEvent, flag: "show"}.

Now (as you also said below) *that* looks much more overkill to me, compared to just calling the function with exactly the one argument which is needed, and not caring about any other arguments which aren't needed.

> Or
>   function foo(...args) where the parameters are collected into an array.

Again, the array would make it more complicated than necessary, harder to write, harder to read, harder to maintain.

> All this might have its place, but for the 2-argument function being
> discussed here, it's overkill. And we don't want one corner to look like
> variation a) and another like variation b).
> 
> Finally, I really dislike this:
>   for (let arg of arguments) {
>     if (arg.target) {
>       aEvent = arg;
>     }
>     else
>       aAction = arg;
>   }

Sorry, it's poorly formatted, should be this:

for (let arg of arguments) {
  if (arg.target)
    aEvent = arg;
  else
    aAction = arg;
}

Which is essentially a one-liner:

for (let arg of arguments) {if (arg.target) {aEvent = arg;} else {aAction = arg;}}

I think the fact that you don't like that is purely a matter of personal taste, because this *does* allow for maximum caller simplicity, which is easy to write, easy to read, hence imo the easiest to maintain.
De facto, apart from the one-liner and caller syntax, the only visible difference is in the function definition, whether it has ordered arguments passed in or not. No-one is ever going to use this function without reading the documentation or checking the inside of the function, where arguments are immediately explained and defined, using the traditional aArgument notation. And there's totally nothing extraordinary or "hard-to-maintain" about not having all necessary parameters in the function definition - we do that all the time with global variables used inside the function. So ymmv, but imho caller simplicity definitely wins in this matter of personal taste. As I've been writing this up, I've actually experienced the benefit...

Just my 2 cents.

Comment 40

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #39)
> (In reply to Jorg K (GMT+1) from comment #38)
> > (In reply to Thomas D. from comment #35)
> > > I'd also think sometimes code
> > > contributors should be given a little bit of room for creativity / new
> > > patterns / own style if there's no real disadvantage.
> > I don't necessarily agree. As boring as it may sound, we do not want any
> > special patterns, we want it all to be uniform since that's easier to
> > maintain.
> 
> I totally understand and appreciate the value of uniformity in code. Yet
> ironically, in this particular case, 100% uniformity will actually make it
> *harder* to maintain

Using your argument of 100% formal uniformity, one could also insist that all conditionals must use if, never switch. Imo that's the same type of thing like our scenario here: Using if always would be 100% uniform, but in a number of cases, it's just harder to write and read, hence harder to maintain. That's why we use if or switch interchangeably according to personal taste, whichever looks and fits best in a given context, even mixed. Same here: Whilst for most functions, having arguments passed in the traditional way will just work fine (esp. when at least one of the arguments is required, which isn't the case here), in this particular case, it just makes callers' and coder's life harder...

Comment 41

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #40)
> Using your argument of 100% formal uniformity, one could also insist that
> all conditionals must use if, never switch.

In a nutshell: 100% uniformity might be perfectionist's dream, but as a matter of fact, it's a myth. Definitely for Thunderbird's code base. And it's not even always helpful nor desirable if taken to the extremes.

Comment 42

[Thomas D. (:thomas8)]

Comment on attachment 8951539 [details] [diff] [review]
Addon-Patch V.1 (on top of patch 4.1): Implement view state machinery incl. minimized/restore; headerclick-restore broken, but Alt+M restore works

Wrt bucket.boxObject.width seemingly reporting false values:

1000 console.logs later, looks like I found the problem. Unfortunately it's an error in my code. This is what seems to happen:
- click on (minimized) bucket header, more precisely the "X attachments" label (especially when minimized so that the label is hard to miss; I'll have to figure out why even clicking where it looks besides the label still seems to trigger the label, maybe flex attribute)
- X attachment label has control="attachmentBucket", so a focus event gets sent first (which I overlooked)
- attachmentBucketOnFocus() calls attachmentBucketEnsureDefaultWidth()
- attachmentBucketEnsureDefaultWidth() sees small bucket and expands it to default width (180px)
- then attachmentBucketHeaderOnClick(evt) calls toggleAttachmentsPane(evt) which looks at the current size (already expanded from focus event), sees the header-click (from evt.target), and concludes it must minimize the bucket - bang - so now we're back to minimized, and it looks as if nothing had happened (Nullsummenspiel...).

So we need to disentangle this somehow.
It's complicated because we're covering a lot of different access methods and scenarios.

Comment 43

[Thomas D. (:thomas8)]

Attachment: Addon-Patch V.2 (on top of Patch 4.1): Implement view state machinery incl. minimize/restore (now working)

Note: Addon patch to be applied on top of patch attachment 8951215 [details] [diff] [review].

Yeah! Here's the fully functional view state machinery (minimize/restore), after fixing comment 42.
Console.logs are still included for reviewers to see what is happening.

Aceman, this is essentially ready so we could start talking about the code.

Jörg, Richard, Aceman, please play with this and offer feedback if this feels right now. Things to try:
- Try all steps with bucket of these sizes:
  - larger than default width
  - smaller than default width, but not minimized
  - minimized
- Alt+M keyboard shortcut, press multiple times for view state cycle/toggle
- Click on bucket header, multiple times (observe tooltip)
- Focus bucket via tab
- Click on bucket whitespace (with and without selection)
- use splitter to drag empty bucket smaller (up to minWidth, and up to 0)

From my side, currently there's only one nit which I'd like to improve:

1) When bucket is minimized (full/empty) without selection, click on whitespace will go into Attach File(s) dialog straight, which is too fast because imo the pane should be restored first before further action.
For empty bucket, maybe user just wants bigger drag target.
For full bucket, it's certainly wise to check first what's in the bucket before adding more.

Richard, on my windows it needs 27px bucket header min-height to be aligned with the line under Sender?

Comment 44

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #43)

> - Alt+M keyboard shortcut, press multiple times for view state cycle/toggle

Alt+M should also be tried with and without focus in bucket.

Comment 45

[Thomas D. (:thomas8)]

Another nit to be fixed:
2) Dragstart of attachment from minimized bucket must restore the bucket first. (mousedown/dragstart events)

Comment 46

[Thomas D. (:thomas8)]

More things to be tried:

In minimized state, if dragging splitters causes (dis)appearance of vertical toolbar, minimized bucket must automatically adjust to that.

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951902 [details] [diff] [review]
Addon-Patch V.2 (on top of Patch 4.1): Implement view state machinery incl. minimize/restore (now working)

I like the fact that a non-empty bucket can't be closed any more.

(In reply to Thomas D. from comment #43)
> - Alt+M keyboard shortcut, press multiple times for view state cycle/toggle
Unless I'm missing something (yes, patch applied, did a build), Alt+M on a bucket that is present and maximised does nothing, neither for full nor empty buckets. I thought for non-empty buckets it would minimise and empty ones close (or also minimise).

So is that an f+ or f-? I'll just cancel it for now.

Comment 48

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #47)
> Comment on attachment 8951902 [details] [diff] [review]
> Addon-Patch V.2 (on top of Patch 4.1): Implement view state machinery incl.
> minimize/restore (now working)
> 
> I like the fact that a non-empty bucket can't be closed any more.
> 
> (In reply to Thomas D. from comment #43)
> > - Alt+M keyboard shortcut, press multiple times for view state cycle/toggle
> Unless I'm missing something (yes, patch applied, did a build), Alt+M on a
> bucket that is present and maximised does nothing, neither for full nor
> empty buckets. I thought for non-empty buckets it would minimise and empty
> ones close (or also minimise).
> 
> So is that an f+ or f-? I'll just cancel it for now.

That would be f-.
This works for me, so it's either user error or something got lost/wrong in my patch.

Possible user error: When bucket is visible/maximized, empty or full, but focus NOT yet on bucket, first Alt+M will focus bucket (obviously, as we're presenting it as an access key). Next Alt+M should minimize full bucket, or close empty bucket, and put focus back into body. Works on my local install. Still doesn't work for you?

Comment 49

[Thomas D. (:thomas8)]

I fixed another nit:
Minimizing empty/full bucket will now move focus into body.

I'm having a hard time fixing nit 1) of comment 43, due to the way events fired, and I'm using so many of them to do something. I wish we could transform this into something more generic, maybe mousedown/keyup. Whenever the minimized bucket is touched in any way, it must restore first. Just onfocus isn't enough because user can shrink (less than default width, but not minimized) a focused bucket and we'd still want to restore that if he makes any move within the bucket.

When a click first causes a focus event, is there any way I can prevent the click event from executing from within the focus event? I tried event.stop(Immediate)Propagation(), both of them with no luck (as I'm probably just stopping the focus event, but not the underlying click event.

Comment 50

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #48)
> That would be f-.
> This works for me, so it's either user error or something got lost/wrong in
> my patch.
> Possible user error: When bucket is visible/maximized, empty or full, but
> focus NOT yet on bucket, first Alt+M will focus bucket (obviously, as we're
> presenting it as an access key). Next Alt+M should minimize full bucket, or
> close empty bucket, and put focus back into body. Works on my local install.
> Still doesn't work for you?
I've popped the patches, pushed them again, built agsin, hammering the keyboard with Alt+M doesn't minimise/hide the the bucket. Right, if in the body, it will focus the bucket.

If I clear the error console, then focus the compose window with the bucket open again, I see:
+++ bucketOnFocus() ++++  MsgComposeCommands.js:5558:3
+++++ ensureDefaultWidth: bucket.boxObject.width (before) = 180  MsgComposeCommands.js:5768:3
+++++ ensureDefaultWidth: bucket.boxObject.width (after) = 180  MsgComposeCommands.js:5778:3
--- updateAttachmentPane() --- bucket.boxObject.width = 180  MsgComposeCommands.js:4908:3
test(true): parseInt(bucket.boxObject.width) *180* > *20* bucket.minWidth || bucket.boxObject.width *180* == 0 (full header if true)  MsgComposeCommands.js:4909:3
Show full header: 0 attachments  MsgComposeCommands.js:4925:5
tooltip >> case bucketWidth (180, number) >= gBucketWidthDefault (180, number)  MsgComposeCommands.js:5805:7

The next Alt+M doesn't close the bucket but shows:
++++ attachmentBucketOnClick(evt) ++++  MsgComposeCommands.js:5621:3
+++++ ensureDefaultWidth: bucket.boxObject.width (before) = 180  MsgComposeCommands.js:5768:3
+++++ ensureDefaultWidth: bucket.boxObject.width (after) = 180  MsgComposeCommands.js:5778:3
onclick: test: parseInt(bucket.boxObject.width) *180* > *20* parseInt(bucket.minWidth)  MsgComposeCommands.js:5624:3
isEnabled - remove checkmarks: test(false): bucket.boxObject.width 180(number) <= 20(string)  MsgComposeCommands.js:698:9
+++++ ensureDefaultWidth: bucket.boxObject.width (before) = 180  MsgComposeCommands.js:5768:3
+++++ ensureDefaultWidth: bucket.boxObject.width (after) = 180  MsgComposeCommands.js:5778:3
--- updateAttachmentPane() --- bucket.boxObject.width = 180  MsgComposeCommands.js:4908:3
test(true): parseInt(bucket.boxObject.width) *180* > *20* bucket.minWidth || bucket.boxObject.width *180* == 0 (full header if true)  MsgComposeCommands.js:4909:3
Show full header: 0 attachments  MsgComposeCommands.js:4925:5
tooltip >> case bucketWidth (180, number) >= gBucketWidthDefault (180, number)  MsgComposeCommands.js:5805:7

Any subsequent Alt+M repeats the second block.

Maybe that helps. Also, Richard and Aceman can try it.

Comment 51

[Richard Marti (:Paenglab)]

Same here, ALT+M opens or moves the focus to the bucket but never closes it. Through the X or the menu it closes.

Comment 52

[Thomas D. (:thomas8)]

Oh sorry, I didn't update the xul file properly. New patch coming.

Comment 53

[Thomas D. (:thomas8)]

Attachment: Addon-Patch V.2.1 (on top of Patch 4.1): Implement view state machinery incl. minimize/restore (nitfix xul: now working; focus body after splitter-minimize)

In the last patch, I forgot to remove the control="attachmentBucket" attribute from attachmentBucketCount label, so alt+M was captured as an accesskey and not as a global shortcut key. That's why bucket could not be minimized/closed. So it should really work now, pls let me know.

Fixed: Minimizing empty/full bucket via splitter will now move focus into body, to avoid loss of focus for empty bucket, and as there's nothing you could possibly want to do with a minimized full bucket.

Pls feedback me on the current behaviour.

Nits of comment 43 and comment 45 still apply, probably requiring a significant rewrite.

Comment 54

[Thomas D. (:thomas8)]

Nit 3) (polish) Minimized bucket should auto-clear selection, as it doesn't make sense to select, then minimize, then act on the old selection later. Instead, selection in minimized bucket will just look odd and be error-prone when you re-visit the bucket. A bit tricky again as long as we expand bucket for select event, because clear-selection claims to fire 'select' event *before* clearing selection, which would end up in another Nullsummenspiel (zero-sum-game). So we'll have to come up with a better way.

Comment 55

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #50)
> (In reply to Thomas D. from comment #48)

> If I clear the error console, then focus the compose window with the bucket
> open again, I see:

Thank you for sharing error console, that's helpful as it's meant as a debugging tool.

> +++ bucketOnFocus() ++++  MsgComposeCommands.js:5558:3

> The next Alt+M doesn't close the bucket but shows:
> ++++ attachmentBucketOnClick(evt) ++++  MsgComposeCommands.js:5621:3

Fwiw, Alt+M certainly shouldn't/can't show onClick event in the logs.

> Any subsequent Alt+M repeats the second block.

Dito, that's unlikely/impossible.

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951982 [details] [diff] [review]
Addon-Patch V.2.1 (on top of Patch 4.1): Implement view state machinery incl. minimize/restore (nitfix xul: now working; focus body after splitter-minimize)

Nice. I like it now. Two comments:

An empty focused bucket has a focus ring (blue/grey border), a non-empty focused bucket doesn't have that. Is that intentional?

Repeated pressing of Alt+M restores the focus into the body, even to the previous cursor position. However, if the subject or an addressing field were focussed before, it would be nice to return there of possible.

Comment 57

[Richard Marti (:Paenglab)]

Comment on attachment 8951982 [details] [diff] [review]
Addon-Patch V.2.1 (on top of Patch 4.1): Implement view state machinery incl. minimize/restore (nitfix xul: now working; focus body after splitter-minimize)

Looks good, thanks.

And yes, it would be good, if the bucket could remember from which field it gets the focus.

Comment 58

[Jorg K (CEST = GMT+2)]

And the focus ring for the non-empty bucket?

Comment 59

[Richard Marti (:Paenglab)]

Then a attachmentitem has the focus.

Comment 60

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #56)
> Comment on attachment 8951982 [details] [diff] [review]
> Addon-Patch V.2.1 (on top of Patch 4.1): Implement view state machinery
> incl. minimize/restore (nitfix xul: now working; focus body after
> splitter-minimize)
> 
> Nice. I like it now. Two comments:

Thanks :)

> An empty focused bucket has a focus ring (blue/grey border), a non-empty
> focused bucket doesn't have that. Is that intentional?

Yes. Non-empty bucket always has a focus ring on one attachment item, having two focus rings (one on bucket, one on attachment) looks odd (KISS), irritating, and is conceptually wrong. Bucket-focus and item-focus are not the same. Windows Explorer also won't focus the entire file list when there's focus on one file.

> Repeated pressing of Alt+M restores the focus into the body, even to the
> previous cursor position. However, if the subject or an addressing field
> were focussed before, it would be nice to return there of possible.

I'll give that some second thoughts, but I'm not sure if restoring the previous focus is helpful. I think it's more likely for users to fill fields in tab order: sender, recipient, subject, then attachments. The next step in the sequence is msg body. My assumption is that when user is coming from address or subject field, those fields are done, so body is a safe guess for the next focus stop with a much higher chance of being useful. Going back has a high chance of breaking the most efficient workflow by requiring extra steps to proceed into body. Or maybe we don't know...

Comment 61

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #60)
> (In reply to Jorg K (GMT+1) from comment #56)
> > Repeated pressing of Alt+M restores the focus into the body, even to the
> > previous cursor position. However, if the subject or an addressing field
> > were focussed before, it would be nice to return there of possible.
> 
> I'll give that some second thoughts, but I'm not sure if restoring the
> previous focus is helpful.

We can't return to the previous focus because that makes minimizing the bucket via keyboard impossible for keyboard users:
- Alt+M from Subject -> focus (and restore) bucket
- Alt+M from bucket -> minimize bucket, return focus to Subject
- Press Tab to go to msg body (which does not have it's own access key): 1st Tab focuses attachment pane and - bang - attachment pane is restored again (but you've just deliberately minimized it, right?), 2nd Tab focuses body (and having to press Tab twice whereas chances are you were done with subject anyway before going into attachments also isn't nice). There's no reason to skip minimized bucket from Tab cycle, in fact that sounds like an easy way of keyboard-restoring rather than forcing keyboard user to go through Alt+M (no longer seen in the visible UI), or the menu. Minimized bucket is an exceptional and impaired state, so I'd definitely want any way of touching the bucket to restore it.

Comment 62

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #43)
> Created attachment 8951902 [details] [diff] [review]
> From my side, currently there's only one nit which I'd like to improve:
> 
> 1) When bucket is minimized (full/empty) without selection, click on
> whitespace will go into Attach File(s) dialog straight, which is too fast
> because imo the pane should be restored first before further action.
> For empty bucket, maybe user just wants bigger drag target.
> For full bucket, it's certainly wise to check first what's in the bucket
> before adding more.

I've tried a lot but I am failing to fix this in any straightforward way, and I want to avoid adding things like "skip-next-click" global variables. I've been suprised many times how events interact, e.g. this:
Merely *having* an onmousedown event routine prevents internal selection of an item under certain circumstances, as seen when DEL on a visibly selected item fails. I don't understand why because I'm not terminating any event bubbling in the onmousedown event and I'm not aware of changing selection (and selection looks correct in the UI), so why should this affect selection?

(In reply to Thomas D. from comment #45)
> Another nit to be fixed:
> 2) Dragstart of attachment from minimized bucket must restore the bucket
> first. (mousedown/dragstart events)

I have fixed this, just adding attachmentBucketEnsureDefaultWidth() before the ondragstart routine.

(In reply to Thomas D. from comment #54)
> Nit 3) (polish) Minimized bucket should auto-clear selection, as it doesn't
> make sense to select, then minimize, then act on the old selection later.
> Instead, selection in minimized bucket will just look odd and be error-prone
> when you re-visit the bucket. A bit tricky again as long as we expand bucket
> for select event, because clear-selection claims to fire 'select' event
> *before* clearing selection, which would end up in another Nullsummenspiel
> (zero-sum-game). So we'll have to come up with a better way.

I haven't tried this, same problem as with fixing 1) above.

Comment 63

[Thomas D. (:thomas8)]

Comment on attachment 8951982 [details] [diff] [review]
Addon-Patch V.2.1 (on top of Patch 4.1): Implement view state machinery incl. minimize/restore (nitfix xul: now working; focus body after splitter-minimize)

Per my comment 62, I can't fix 2 of the remaining nits, only restore-bucket-on-dragstart. So unless someone else has better ideas, I'm done here. So let's get it reviewed and landed please.

Comment 64

[Thomas D. (:thomas8)]

I don't know the deadlines for landing or uplifting this, but can we please ensure to land as much as possible of this timely so that it can ship with the next release version of TB? Complements new features like "Reorder Attachments" nicely as an all-round overhaul of attachment UX.

Comment 65

[Thomas D. (:thomas8)]

Jörg, Acema, what's the way forward to get this landed, especially strings before string freeze on 2018-03-11?

Comment 66

[Jorg K (CEST = GMT+2)]

I'm sure Aceman will review this soon.

Comment 67

[:aceman]

Comment on attachment 8951215 [details] [diff] [review]
Patch V. 4.1: (nitfixes) Fast-track working goodies w/o view state machinery

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

Thanks for doing the cleanup and improvements here.

I have played with this and have noticed this:
1.Alt-M is not toggling the attachment bucket (even when empty), i.e. it doesn't close it ever. Is that intentional?
2.When there are some attachments, the View->Attachment pane item is disabled (greyed out). But the checkbox besides it looks enabled. Is that a css/XUL bug?
3.Only when the attachment bucket is empty and focused, it does have a blue internal outline. When it is not empty, this outline is gone. Intentional?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +678,5 @@
> +    cmd_toggleAttachmentPane: {
> +      isEnabled: function() {
> +        let cmdToggleAttachmentPane =
> +          document.getElementById("cmd_toggleAttachmentPane");
> +        let bucket = document.getElementById("attachmentBucket");

GetMsgAttachmentElement()

@@ +4561,5 @@
> +  // on some platforms according to spec), make sure there's at least one item
> +  // set as currentItem which will be focused when listbox gets focus, because
> +  // currently we don't indicate focus on the listbox itself, assuming
> +  // there'll always be a focused attachment.
> +  let firstAddedAttachment = items[0];

Is it sure there is at least one item? Otherwise this may throw a JS warning. The addedAttachments.length > 0 check is below this code.

@@ +4563,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 >= 0)) {

< 0 ?

@@ +4774,5 @@
>  
>  function RemoveAllAttachments()
>  {
>    let bucket = document.getElementById("attachmentBucket");
> +  if (bucket.getRowCount < 1)

getRowCount is a function so getRowCount(). Or use .itemCount, also further down.

@@ +4783,4 @@
>    let removedAttachments = Components.classes["@mozilla.org/array;1"]
>                                       .createInstance(Components.interfaces.nsIMutableArray);
>  
> +  while (bucket.getRowCount()) {

This is not a boolean, so better would be itemCount > 0 .

@@ +4838,4 @@
>    document.getElementById("attachmentBucketSize").value =
> +      (count > 0) ? gMessenger.formatFileSize(gAttachmentsSize)
> +                  : "";
> +  document.getElementById("attachmentBucketCloseButton").collapsed = !!count;

count < 1 ?

@@ +4842,5 @@
> +
> +  attachmentBucketUpdateTooltips();
> +
> +  // If aShowPane argument is omitted, it's just updating, so we're done.
> +  if (arguments.length == 0)

I think (aShowPane === undefined) would be better.

@@ +4851,5 @@
>  }
>  
>  function RemoveSelectedAttachment()
>  {
>    let bucket = document.getElementById("attachmentBucket");

GetMsgAttachmentElement()

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

bucket.selectedCount == 0, as used below.

@@ +5183,5 @@
> + * If aAction parameter is omitted, auto-cycling of view states, bias for "show".
> + * Note: Forcing "hide" is not recommended as it may violate ux-error-prevention
> + * for full bucket in the current UI layout.
> + * Note: Parameters are optional (passing aEvent is recommended where possible)
> + * and their order doesn't matter.

What? Why?

@@ +5188,3 @@
>   *
> + * @param aAction {string} "show": show attachment pane
> + *                         "hide": hide attachment pane

You also seem to have a value of "focus" in the code.

@@ +5194,5 @@
>   */
> +function toggleAttachmentPane() {
> +  let aEvent;
> +  let aAction;
> +  let bucket = document.getElementById("attachmentBucket");

GetMsgAttachmentElement()

@@ +5201,5 @@
> +  let bucketHasFocus = (document.activeElement == bucket);
> +  let attachmentBucketSizer = document.getElementById("attachmentbucket-sizer");
> +
> +  for (let arg of arguments) {
> +    if (arg.target) {

Why do you need the arguments position-independent? This is unusual in our code.

@@ +5210,5 @@
> +  }
> +
> +  if (aEvent) {
> +    let clickTargetId = aEvent.explicitOriginalTarget.id;
> +    if (aEvent.explicitOriginalTarget.id == "menu_toggleAttachmentPane" &&

If you actually do not really need the event, just the id, you could pass that from the caller. You do not even want the id, you just want to distinguish if call was from the menu? If that menuitem works as a toggle, you could pass in a value of "toggle" (not omitted) and do the remaining deciding here as you have it now. If "toggle" actually is any different in meaning than omitted.

@@ +5380,2 @@
>        reorderAttachmentsPanel.hidePopup();
> +    } else if (bucket.itemCount) {

> 0

@@ +5380,3 @@
>        reorderAttachmentsPanel.hidePopup();
> +    } else if (bucket.itemCount) {
> +      if (bucket.selectedCount) {

> 0

@@ +5394,5 @@
> +
> +  if (aEvent.key == "Enter" && bucket.itemCount == 0) {
> +    // Enter on empty bucket to add file attachments, convenience
> +    // keyboard equivalent of single-click on bucket whitespace.
> +    goDoCommand('cmd_attachFile');

Double quotes.

@@ +5402,5 @@
> +    toggleAttachmentPane();
> +  }
> +}
> +
> +function attachmentBucketOnClick(event)

aEvent

@@ +5410,5 @@
> +  // - Otherwise, e.g. on a plain empty bucket, show 'Attach File(s)' dialog.
> +  if (attachmentsSelectedCount() == 0) {
> +    let boundTarget = document.getBindingParent(event.originalTarget);
> +    if (event.button == 0 && boundTarget && boundTarget.localName == "scrollbox")
> +      goDoCommand('cmd_attachFile');

Double quotes.

@@ +5421,5 @@
> +}
> +
> +
> +function attachmentBucketUpdateTooltips() {
> +  let bucket = document.getElementById("attachmentBucket");

GetMsgAttachmentElement() ?

@@ +5455,5 @@
>  
> +function attachmentBucketMarkEmptyBucket() {
> +  let attachmentBucket = GetMsgAttachmentElement();
> +  let bucketSizer = document.getElementById("attachmentbucket-sizer");
> +  if (attachmentBucket.getRowCount() > 0) {

itemCount

::: mail/components/compose/content/messengercompose.xul
@@ +151,5 @@
>    <command id="cmd_reorderAttachments"
>             oncommand="goDoCommand('cmd_reorderAttachments')" disabled="true"/>
> +  <command id="cmd_toggleAttachmentPane"
> +           label="&toggleAttachmentPaneCmd.label;"
> +           accesskey="&toggleAttachmentPaneCmd.accesskey;"

Why is all this on the command?

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +90,5 @@
>  <!ENTITY reorderAttachmentsCmd.key "x">
> +<!ENTITY toggleAttachmentPaneCmd.label "Attachment Pane">
> +<!-- LOCALIZATION NOTE (toggleAttachmentPaneCmd.accesskey):
> +     toggleAttachmentPaneCmd.accesskey and
> +     toggleAttachmentPaneCmd.key must be the same. -->

Why? Alt-m as a global hotkey is one thing, but in the menus it doesn't need to be the same accesskey.

Comment 68

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #67)
> 1.Alt-M is not toggling the attachment bucket (even when empty), i.e. it
> doesn't close it ever. Is that intentional?
Really? I complained about this and it got fixed, see comment #47 and fix in comment #53. Worked for me last time I tried.

Comment 69

[:aceman]

I only tried the first patch and this problem is mentioned to be solved by the other one.

Comment 70

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #67)
> Review of attachment 8951215 [details] [diff] [review]:
> -----------------------------------------------------------------

Thank you for looking into this!

> I have played with this and have noticed this:
> 1.Alt-M is not toggling the attachment bucket (even when empty), i.e. it
> doesn't close it ever. Is that intentional?

As Jörg said, works with the add-on patch.
If we fail to land the add-on patch, we could consider backporting this, although I remember vaguely that somehow this wasn't feasible without having the full state machinery of the addon patch.

> 2.When there are some attachments, the View->Attachment pane item is
> disabled (greyed out). But the checkbox besides it looks enabled. Is that a
> css/XUL bug?

Must be, the menuitem is realized as a checkbox so disabling the menuitem must disable the checkbox, too.

> 3.Only when the attachment bucket is empty and focused, it does have a blue
> internal outline. When it is not empty, this outline is gone. Intentional?

Yes, already discussed in comment 58, comment 59, comment 60. Non-empty: at least one item will always have focus ring, and we shouldn't create double focus rings, which is visually odd and functionally wrong (the bucket does NOT have full focus, but the item does).

> @@ +4561,5 @@
> > +  // on some platforms according to spec), make sure there's at least one item
> > +  // set as currentItem which will be focused when listbox gets focus, because
> > +  // currently we don't indicate focus on the listbox itself, assuming
> > +  // there'll always be a focused attachment.
> > +  let firstAddedAttachment = items[0];
> 
> Is it sure there is at least one item? Otherwise this may throw a JS
> warning. The addedAttachments.length > 0 check is below this code.

This can't throw a warning:
  let items = [];
  let firstAddedAttachment = items[0];
  if (firstAddedAttachment) {
    if (!(bucket.currentIndex >= 0)) {

Even if there's no item, items[0] returns undefined, then I check for if(undefined) which returns false so the index-fixing isn't done. But maybe if I move it down under the addedAttachments.length > 0 check, we could eliminate one condition.

> @@ +4563,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 >= 0)) {
> 
> < 0 ?

Already discussed, and mentioned in code comment, currentIndex on some platforms can be undefined, so < 0 check would fail.

> @@ +4774,5 @@
> >  
> >  function RemoveAllAttachments()
> >  {
> >    let bucket = document.getElementById("attachmentBucket");
> > +  if (bucket.getRowCount < 1)
> 
> getRowCount is a function so getRowCount(). Or use .itemCount, also further
> down.

Yeah, looking at getRowCount() and itemCount in listbox.xml, they should have the same performance as both of them call the internal getRowCount of the XUL thing. But itemCount looks nicer, more readable.

> @@ +4783,4 @@
> >    let removedAttachments = Components.classes["@mozilla.org/array;1"]
> >                                       .createInstance(Components.interfaces.nsIMutableArray);
> >  
> > +  while (bucket.getRowCount()) {
> 
> This is not a boolean, so better would be itemCount > 0 .

Why? bucket.getRowCount() returns a truthy or falsy value which is correctly evaluated by while().
But we can make it while(bucket.itemCount).

> @@ +4838,4 @@
> >    document.getElementById("attachmentBucketSize").value =
> > +      (count > 0) ? gMessenger.formatFileSize(gAttachmentsSize)
> > +                  : "";
> > +  document.getElementById("attachmentBucketCloseButton").collapsed = !!count;
> 
> count < 1 ?

Yeah but does that make difference?
And then, why not make it count == 0? That would make it more readable.

> @@ +4842,5 @@
> > +
> > +  attachmentBucketUpdateTooltips();
> > +
> > +  // If aShowPane argument is omitted, it's just updating, so we're done.
> > +  if (arguments.length == 0)
> 
> I think (aShowPane === undefined) would be better.

Yeah, maybe. More future-proof if we ever wanted to add other arguments.

> > +  if (bucket.selectedItems.length == 0)
> 
> bucket.selectedCount == 0, as used below.

Existing code, yeah, we could change that. What about performance?

> @@ +5183,5 @@
> > + * If aAction parameter is omitted, auto-cycling of view states, bias for "show".
> > + * Note: Forcing "hide" is not recommended as it may violate ux-error-prevention
> > + * for full bucket in the current UI layout.
> > + * Note: Parameters are optional (passing aEvent is recommended where possible)
> > + * and their order doesn't matter.
> 
> What? Why?

Just what I said, parameters are optional and their order is irrelevant. What's hard to understand?

> @@ +5188,3 @@
> >   *
> > + * @param aAction {string} "show": show attachment pane
> > + *                         "hide": hide attachment pane
> 
> You also seem to have a value of "focus" in the code.

Indeed, should be added to comment.

> @@ +5201,5 @@
> > +  let bucketHasFocus = (document.activeElement == bucket);
> > +  let attachmentBucketSizer = document.getElementById("attachmentbucket-sizer");
> > +
> > +  for (let arg of arguments) {
> > +    if (arg.target) {
> 
> Why do you need the arguments position-independent? This is unusual in our
> code.

It might be unusual (rarely used), but it actually simplifies callers and makes them easier to write, read, and understand what is going on. As we need to call this function with only aEvent, and with only aAction, I see no point in passing empty arguments when it can be avoided. For details (discussed with Jörg), see my comment 30. Maybe sometimes we can be creative and have a slight variation in argument passing style if that's helpful for caller simplicity and readability.

> @@ +5210,5 @@
> > +  }
> > +
> > +  if (aEvent) {
> > +    let clickTargetId = aEvent.explicitOriginalTarget.id;
> > +    if (aEvent.explicitOriginalTarget.id == "menu_toggleAttachmentPane" &&
> 
> If you actually do not really need the event, just the id, you could pass
> that from the caller. You do not even want the id, you just want to
> distinguish if call was from the menu? If that menuitem works as a toggle,
> you could pass in a value of "toggle" (not omitted) and do the remaining
> deciding here as you have it now. If "toggle" actually is any different in
> meaning than omitted.

I'll look into that.

> @@ +5380,2 @@
> >        reorderAttachmentsPanel.hidePopup();
> > +    } else if (bucket.itemCount) {
> 
> > 0

Why write more if it works correctly with less? itemCount is truthy for > 0, and falsy for 0. So this evaluates correctly.

> @@ +5380,3 @@
> >        reorderAttachmentsPanel.hidePopup();
> > +    } else if (bucket.itemCount) {
> > +      if (bucket.selectedCount) {
> 
> > 0

Dito.

> ::: mail/components/compose/content/messengercompose.xul
> @@ +151,5 @@
> >    <command id="cmd_reorderAttachments"
> >             oncommand="goDoCommand('cmd_reorderAttachments')" disabled="true"/>
> > +  <command id="cmd_toggleAttachmentPane"
> > +           label="&toggleAttachmentPaneCmd.label;"
> > +           accesskey="&toggleAttachmentPaneCmd.accesskey;"
> 
> Why is all this on the command?

Why not? I think that's better because the label will then be available for any other (addon) consumers of the same command.

> ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> @@ +90,5 @@
> >  <!ENTITY reorderAttachmentsCmd.key "x">
> > +<!ENTITY toggleAttachmentPaneCmd.label "Attachment Pane">
> > +<!-- LOCALIZATION NOTE (toggleAttachmentPaneCmd.accesskey):
> > +     toggleAttachmentPaneCmd.accesskey and
> > +     toggleAttachmentPaneCmd.key must be the same. -->
> 
> Why? Alt-m as a global hotkey is one thing, but in the menus it doesn't need
> to be the same accesskey.

Good catch, I meant that it must be same as attachment header access key, which is attachments.accesskey = "m"

Comment 71

[:aceman]

(In reply to Thomas D. from comment #70)
> > I have played with this and have noticed this:
> > 1.Alt-M is not toggling the attachment bucket (even when empty), i.e. it
> > doesn't close it ever. Is that intentional?
> 
> As Jörg said, works with the add-on patch.
> If we fail to land the add-on patch, we could consider backporting this,
> although I remember vaguely that somehow this wasn't feasible without having
> the full state machinery of the addon patch.

OK.
 
> This can't throw a warning:
>   let items = [];
>   let firstAddedAttachment = items[0];
>   if (firstAddedAttachment) {
>     if (!(bucket.currentIndex >= 0)) {
> 
> Even if there's no item, items[0] returns undefined, then I check for
> if(undefined) which returns false so the index-fixing isn't done. But maybe
> if I move it down under the addedAttachments.length > 0 check, we could
> eliminate one condition.

It would be safer.
 
> > @@ +4563,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 >= 0)) {
> > 
> > < 0 ?
> 
> Already discussed, and mentioned in code comment, currentIndex on some
> platforms can be undefined, so < 0 check would fail.

Ah, ok.
 
> > @@ +4783,4 @@
> > >    let removedAttachments = Components.classes["@mozilla.org/array;1"]
> > >                                       .createInstance(Components.interfaces.nsIMutableArray);
> > >  
> > > +  while (bucket.getRowCount()) {
> > 
> > This is not a boolean, so better would be itemCount > 0 .
> 
> Why? bucket.getRowCount() returns a truthy or falsy value which is correctly
> evaluated by while().
> But we can make it while(bucket.itemCount).

I don't trust this as x.y in JS is also used just to check for existence of property y in x. If we use it to fetch its value and even evaluate it as a boolean at the same time, I find that semantically ugly.
 
> > @@ +4838,4 @@
> > > +  document.getElementById("attachmentBucketCloseButton").collapsed = !!count;
> > 
> > count < 1 ?
> 
> Yeah but does that make difference?
> And then, why not make it count == 0? That would make it more readable.

As you wish, just not !!count, which is again forcing an int into a bool.
 
> > > +  if (bucket.selectedItems.length == 0)
> > 
> > bucket.selectedCount == 0, as used below.
> 
> Existing code, yeah, we could change that. What about performance?

It is internally implemented as .selectedItems.length :)
 
> > @@ +5201,5 @@
> > > +  let bucketHasFocus = (document.activeElement == bucket);
> > > +  let attachmentBucketSizer = document.getElementById("attachmentbucket-sizer");
> > > +
> > > +  for (let arg of arguments) {
> > > +    if (arg.target) {
> > 
> > Why do you need the arguments position-independent? This is unusual in our
> > code.
> 
> It might be unusual (rarely used), but it actually simplifies callers and
> makes them easier to write, read, and understand what is going on. As we
> need to call this function with only aEvent, and with only aAction, I see no
> point in passing empty arguments when it can be avoided. For details
> (discussed with Jörg), see my comment 30.

I see, Jorg also didn't like it ;)

> Maybe sometimes we can be creative
> and have a slight variation in argument passing style if that's helpful for
> caller simplicity and readability.

I don't think it increases readability if you just pass some event (click on a menuitem) which doesn't in any way convey which state you want to toggle to. You determine that in another way, you just want to know you were called from the menu. So maybe you just can have a "toggle" action for it and have one argument.
 
> > @@ +5380,2 @@
> > >        reorderAttachmentsPanel.hidePopup();
> > > +    } else if (bucket.itemCount) {
> > 
> > > 0
> 
> Why write more if it works correctly with less? itemCount is truthy for > 0,
> and falsy for 0. So this evaluates correctly.

Because I don't like these truthy/falsy pretenders :) But if you drag in some JS expert that likes this syntax, I may accept it ;)
 
> > ::: mail/components/compose/content/messengercompose.xul
> > > +  <command id="cmd_toggleAttachmentPane"
> > > +           label="&toggleAttachmentPaneCmd.label;"
> > > +           accesskey="&toggleAttachmentPaneCmd.accesskey;"
> > 
> > Why is all this on the command?
> 
> Why not? I think that's better because the label will then be available for
> any other (addon) consumers of the same command.

And what if it doesn't want the label and accesskey? We do not seem to use this on other commands. Yes, we use to inherit the state (like "disabled" or "checked") but not labels as those may be different at different places in the UI.

Comment 72

[Thomas D. (:thomas8)]

Attachment: Patch V. 4.2: (nitfixes) Fast-track working goodies w/o view state machinery

This addresses comment #67 (and the addon patch will need to be updated accordingly).

(In reply to :aceman from comment #71)
> (In reply to Thomas D. from comment #70)
> I don't think it increases readability if you just pass some event (click on
> a menuitem) which doesn't in any way convey which state you want to toggle
> to. You determine that in another way, you just want to know you were called
> from the menu. So maybe you just can have a "toggle" action for it and have
> one argument.

That opens another can of worms, and spoils the beauty and default format for even more callers.
There's nothing special about passing in the event which triggered the command and determine the right action from inside the command function, that's often seen in our code, e.g. Shift-modifier of cmd_reply and friends. In fact, that's good style because it allows callers to be generic with command="cmd_id" attribute. Your favored solution would require hardwiring several places in XUL with oncommand="toggleAttachmentPane('toggle');" which is bad style and undesired for the command logic abstraction. Worse, as menuClick and headerClick need different behaviour in the addon patch, we end up having even more flavors like 'toggleMenu' and 'toggleHeader'. This function is a one-for-all swiss knife doing a lot of different things depending on circumstances, i.e. it contains the entire attachment pane view state machinery; that's complex enough so callers should be as simple as possible. Given that, I think it's quite irrelevant and totally inconsequential if we have explicit arguments in the function definition or if we retrieve them with a one-liner inside the function.

> > > @@ +5380,2 @@
> > > >        reorderAttachmentsPanel.hidePopup();
> > > > +    } else if (bucket.itemCount) {
> > > 
> > > > 0
> > 
> > Why write more if it works correctly with less? itemCount is truthy for > 0,
> > and falsy for 0. So this evaluates correctly.
> 
> Because I don't like these truthy/falsy pretenders :) But if you drag in
> some JS expert that likes this syntax, I may accept it ;)

I admit your style is easier to read, but I see no fault with the truthy/falsy pretenders as they are quite predictable ;)

> > > ::: mail/components/compose/content/messengercompose.xul
> > > > +  <command id="cmd_toggleAttachmentPane"
> > > > +           label="&toggleAttachmentPaneCmd.label;"
> > > > +           accesskey="&toggleAttachmentPaneCmd.accesskey;"
> > > 
> > > Why is all this on the command?
> > 
> > Why not? I think that's better because the label will then be available for
> > any other (addon) consumers of the same command.
> 
> And what if it doesn't want the label and accesskey? We do not seem to use
> this on other commands. Yes, we use to inherit the state (like "disabled" or
> "checked") but not labels as those may be different at different places in
> the UI.

You are right.

Comment 73

[Thomas D. (:thomas8)]

Attachment: Patch V. 4.3: (nitfixes) Fast-track working goodies w/o view state machinery

Fix some nits in last patch.

For review, pls refer to my comment 72.

Caveat: The new string comments assume that we'll land the addon patch with full state machinery, too...

Comment 74

[:aceman]

Comment on attachment 8956364 [details] [diff] [review]
Patch V. 4.3: (nitfixes) Fast-track working goodies w/o view state machinery

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

Thanks, this is almost done.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +679,5 @@
> +      isEnabled: function() {
> +        let cmdToggleAttachmentPane =
> +          document.getElementById("cmd_toggleAttachmentPane");
> +        let bucket = GetMsgAttachmentElement();
> +        let bucketWidth = bucket.boxObject.width;

Do you really need to get the width here (and multiple places below)? Can't you check .collapsed or similar? I want to avoid future problems when e.g. the width may stay at ~1px or so even when collapsed.

@@ +4780,2 @@
>    let removedAttachments = Components.classes["@mozilla.org/array;1"]
>                                       .createInstance(Components.interfaces.nsIMutableArray);

The patch will need to change all Components.classes to Cc and Components.interfaces to Ci to unbitrot it. Also the dot in .createInstance need to then be aligned under the [ of previous line.
But we can do that at landing time if there are no other changes.

@@ +4780,5 @@
>    let removedAttachments = Components.classes["@mozilla.org/array;1"]
>                                       .createInstance(Components.interfaces.nsIMutableArray);
>  
> +  while (bucket.itemCount > 0) {
> +    let item = bucket.removeItemAt(bucket.getRowCount() - 1);

.itemCount

@@ +4783,5 @@
> +  while (bucket.itemCount > 0) {
> +    let item = bucket.removeItemAt(bucket.getRowCount() - 1);
> +    if (item.attachment.size != -1) {
> +      gAttachmentsSize -= item.attachment.size;
> +      updateAttachmentPane("show");

Do you want to update this for every attachment, or only after the whole batch is done (as in the original code)?

@@ +4830,2 @@
>    let words = getComposeBundle().getString("attachmentCount");
> +  let count = bucket.getRowCount();

.itemCount ?

@@ +4864,5 @@
> +  for (let i = bucket.selectedCount - 1; i >= 0; i--) {
> +    let item = bucket.removeItemAt(bucket.getIndexOfItem(bucket.getSelectedItem(i)));
> +    if (item.attachment.size != -1) {
> +      gAttachmentsSize -= item.attachment.size;
> +      updateAttachmentPane("show");

The same question.

::: mail/components/compose/content/messengercompose.xul
@@ +151,5 @@
>    <command id="cmd_reorderAttachments"
>             oncommand="goDoCommand('cmd_reorderAttachments')" disabled="true"/>
> +  <command id="cmd_toggleAttachmentPane"
> +           label="&toggleAttachmentPaneCmd.label;"
> +           accesskey="&toggleAttachmentPaneCmd.accesskey;"

So the label and accesskey is still here?

Comment 75

[:aceman]

Magnus, can you decide here on the mixed arguments to toggleAttachmentPane() in the patch? We do not like it much (as in comment 29 and comment 67), but is there any other reason this shouldn't be done?

Comment 76

[Jorg K (CEST = GMT+2)]

Attachment: Part 1: Patch V. 4.3 - rebased

I've rebased this "as a service" in case it could be useful.

Comment 77

[Jorg K (CEST = GMT+2)]

Part 2 is harder to rebase and has a lot more rejects. Also, that needs to be cleaned up a bit to remove the console.log() calls. If we're running out of time, I suggest to land the strings from Part 2 with Part 1:
+attachmentBucketHeaderShowTooltip=Show attachment pane
+attachmentBucketHeaderMinimizeTooltip=Minimize attachment pane
+attachmentBucketHeaderRestoreTooltip=Restore attachment pane

Comment 78

[Magnus Melin [:mkmelin]]

Well the argument is already a string, so I think it's ok to add another third supported case if needed.

(But from the comments, I don't understand what "minimized" means.)

Also, please prefer real JavaScript types in the documentation. {String}, {Object} and so on.

Comment 79

[Jorg K (CEST = GMT+2)]

Hmm, that answer is pretty unclear. The question is whether to have
  function toggleAttachmentPane()
with no arguments being called with string or (event) object (or both?) and digging out its arguments like so:
+  for (let arg of arguments) {
+    if (arg.target) {
+      aEvent = arg;
+    }
+    else
+      aAction = arg;
+  }

Comment 80

[Magnus Melin [:mkmelin]]

Jörg pointed out I misunderstood the question (sorry!). 

And yes I agree with others 

+  for (let arg of arguments) {
+    if (arg.target) {
+      aEvent = arg;
+    }
+    else
+      aAction = arg;
+  }

This is simply not a good idea. I don't like using mixed typed arguments to start with, and without even declaring what input arguments are expected it's even worse. Don't do it.

Comment 81

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #80)
> Jörg pointed out I misunderstood the question (sorry!). 
> 
> And yes I agree with others 
> 
> +  for (let arg of arguments) {
> +    if (arg.target) {
> +      aEvent = arg;
> +    }
> +    else
> +      aAction = arg;
> +  }
> 
> This is simply not a good idea. I don't like using mixed typed arguments

You mean you don't like it when a function has one argument of type string and another one of another type?
Or you don't like it when one argument might have different types?

> to start with, and without even declaring what input arguments are expected
> it's even worse.

The expected input arguments are declared in the function comment, and they are self-explanatory from context (using string like "show" sometimes and passing aEvent sometimes to get the target and do smart auto-toggle of attachment pane view states according to the target (header click expands small bucket, but menu click will minimize/hide the bucket). I don't see how reading the function comment would be harder than reading the function definition, and either way you still have to read the function to understand what is really being done. A simple one-liner conditional to tell the argument type apart allows for all callers to be minimalist and just pass what's needed, which makes writing and reading them easier.

What hasn't been mentioned in the question to you is the fact that callers only ever need one type of argument, either the string or the event, so hard-wiring both argument types in the function will force many callers into passing a useless empty argument, which imho is hard to write and hard to read.

We haven't used this strategy yet in our code, but I think that's just a case of conservative expectations, and I maintain that I can't see anything bad in smart reading of argument type inside the function, so I'm still failing to see why I should mess up my callers with empty arguments. So maybe someone else wants to do that degrading of intelligent code for me...

Comment 82

[Jorg K (CEST = GMT+2)]

I think Magnus was pretty clear, like the rest of us, he doesn't like the "variable argument" function.

Looking at the patches I can confirm that the function only ever takes one argument or none. So if you don't want to "mess up" the callers, you could do two functions which call a common worker:
function toggleAttachmentPaneWithEvent(aEvent) {
  toggleAttachmentPane(aEvent, null);
}

function toggleAttachmentPaneWithAction(aAction) {
  toggleAttachmentPane(null, aAction);
}

Or maybe as a compromise do
function toggleAttachmentPane(aArg) {
  let action = null;
  let event = null;
  if (typeof(aArg) == "string") // or should that be ===?
    action = aArg;
  else if (typeof(aArg) == "object")
    event = aArg.
I think that looks simpler than your loop and detecting your even on arg.target.

In order for this not to miss the string cut off date, I will land the strings from both patches tonight after 6 PM (GMT+1) and also change attachButton.tooltip to attachButton.tooltip2 in the existing code.

Comment 83

[Jorg K (CEST = GMT+2)]

(Unintentionally added NI, how did that happen?)

Comment 84

[:aceman]

Attachment: Part 1 - Patch V. 4.4: (more polish/nitfixes) Fast-track backend and UX goodies w/o view state machinery [LANDED in comment 87]

In this version I fixed all the nits from my last review, including the event argument to toggleAttachmentPane. I converted it to the "toggle" value, which will be the same as omitted value.

It seems that changing the bucket.boxObject.width == 0 to attachmentBox.collapsed also fixed the Alt-M not closing the pane/bucket when empty. Which was supposedly to be fixed by the addon patch. So this may support my uneasy feeling about relying on .width for anything.

Can you guys check if this still does what was intented?

So this would be ready to land as a whole, even if the addon patch is landed weeks later. I haven't really looked what that part wants to do yet.
And I will need to look carefully as it relies on bucket.width for many critical parts.

Comment 85

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957898 [details] [diff] [review]
Part 1 - Patch V. 4.4: (more polish/nitfixes) Fast-track backend and UX goodies w/o view state machinery [LANDED in comment 87]

This works well enough now. Alt+M even closes an empty bucket now. I'd still like to see part 2 to avoid closing a full bucket with the slider.

Thomas, this is landing now, no offence intended. Please address any issues you might have with Aceman's work in a follow-up.

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9872aea0db40
View state machinery for cmd_toggleAttachmentPane, now incl. minimize/restore (strings only). r=aceman, ui-r=Paenglab

Comment 87

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/52e0316a7f8e
Backed out 2 changesets for landing without a bug number. a=backout
https://hg.mozilla.org/comm-central/rev/7c7c2c56fc37
Implement cmd_toggleAttachmentPane (Alt+M), cmd_removeAllAttachments, polish attachment bucket behaviour and keyboard access, code cleanup. r=aceman, ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/4b5c6a135ad5
View state machinery for cmd_toggleAttachmentPane, now incl. minimize/restore (strings only). r=aceman, ui-r=Paenglab DONTBUILD

Comment 88

[Jorg K (CEST = GMT+2)]

I had to back this out since the first part didn't have a bug number in the commit message :-(((((

I landed the strings from the second patch although "attachmentBucketHeaderShowTooltip" is not used in the patch. So we can either use it or remove it.

Comment 89

[Jorg K (CEST = GMT+2)]

Looks like this broke two tests:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-focus.js | test-focus.js::test_f6_attachment
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-focus.js | test-focus.js::test_ctrl_tab_attachment

Comment 90

[:aceman]

Attachment: fix test

Comment 91

[Jorg K (CEST = GMT+2)]

Attachment: 1428631-test.patch

As discussed via IRC, maybe better to set the focus directly.

Comment 92

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #89)
> Looks like this broke two tests:
> TEST-UNEXPECTED-FAIL |
> /builds/worker/workspace/build/tests/mozmill/composition/test-focus.js |
> test-focus.js::test_f6_attachment

Yes, and it probably breaks them because the behaviour of Alt+M access key / shortcut key is now wrong, because Aceman flattened the function and removed the distinction between menu click and shortcut key...
Another candidate for breaking these tests is bug 682424.

Comment 93

[Jorg K (CEST = GMT+2)]

Attachment: 1428631-test.patch (v2) [LANDED in comment 94]

Another version as per IRC.

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/59c0f6e4825c
Follow-up: fix composer focus tests. r=jorgk

Comment 95

[:aceman]

(In reply to Thomas D. from comment #92)
> Yes, and it probably breaks them because the behaviour of Alt+M access key /
> shortcut key is now wrong, because Aceman flattened the function and removed
> the distinction between menu click and shortcut key...

The test does not use these. It failed because after adding attachments (even by non-interactive JS in the test), the bucket is forcefully focused which the test didn't expect.

But I would like to know what you mean here and why there should be a difference.
At least we got the main part of this in with strings and now we can polish it.

> Another candidate for breaking these tests is bug 682424.

It didn't because Contacts sidebar isn't enabled in the test. But we found we could actually add that to the test so Jorg kindly filed the bug 1444771.

Comment 96

[:aceman]

Comment on attachment 8957971 [details] [diff] [review]
1428631-test.patch (v2) [LANDED in comment 94]

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

Thanks.

Comment 97

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #95)
> (In reply to Thomas D. from comment #92)
> > the behaviour of Alt+M access key /
> > shortcut key is now wrong, because Aceman flattened the function and removed
> > the distinction between menu click and shortcut key...

> But I would like to know what you mean here and why there should be a
> difference.

This bug "overloads" the existing Alt+M access key to /also/ work as a shortcut key for attachment pane's view state machinery. The access key is displayed in the attachment pane header: "10 attach_m_ents". The identical shortcut key is displayed in the menu: "View > Attachment Pane  Alt+M". Technically we disconnect the access key and just listen for the shortcut key (with add-on patch at least), then handle everything from there. But we're NOT removing the access key functionality (i.e.: focus bucket), because it's obviously useful and needed. I.e., whenever focus is NOT in the attachment pane, the first usage of Alt+M must focus and restore the attachment pane (access key -> give access to the pane). You have removed that functionality for the empty bucket, because you dropped (event) parameter from the command, and hence you can no longer check where the event comes from, so you can't handle the menu click differently from header click, as you had to remove the following check for "hide":
> if (aEvent.explicitOriginalTarget.id == "menu_toggleAttachmentPane" &&

Empty bucket must definitely be accessible via access key first if it's not focused yet, because we must assume that user kept it open for the very reason of adding attachments (otherwise we've made it easy to close it, using [x], [Esc] key, or Alt+M on focused empty bucket (e.g. after deleting all attachments).
To rephrase again, it's impossible to get an empty bucket without (initial) focus, so if user has deliberately de-focused the empty bucket without closing it, we must assume that he still wants to use that empty bucket, so it's counter-productive to close empty bucket on Alt+M if focus is NOT on the bucket.
To rephrase again, whenever the bucket is shown, the access key is displayed, so for the first use of Alt+M we must first honor the traditional access key behaviour of just focusing the element (ux-error-prevention), and user can rightly expect the access key to work and just focus it (as opposed to hiding/minimizing).

When bucket DOES HAVE focus, there's no reason to use/expect the access key to focus the bucket (as it's already focused), that's when Alt+M is now free for other tricks like hiding empty bucket, or minimizing full bucket.

Sounds complicated, but it's actually quite simple and straight forward, and pressing Alt+M more than once will always conveniently cycle through the most relevant view states (for Alt+M, we're skipping minimized state of empty, focused bucket, which is only available via mouse click on the header of empty bucket).

> At least we got the main part of this in with strings and now we can polish
> it.

OK, that's good.

> > Another candidate for breaking these tests is bug 682424.
> 
> It didn't because Contacts sidebar isn't enabled in the test. But we found
> we could actually add that to the test so Jorg kindly filed the bug 1444771.

Thanks, Jörg, also for landing Part 1 and the strings of Part 2!

Comment 98

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #97)
> I.e., whenever focus is NOT in the
> attachment pane, the first usage of Alt+M must focus and restore the
> attachment pane (access key -> give access to the pane). You have removed
> that functionality for the empty bucket, because you dropped (event)
> parameter from the command, and hence you can no longer check where the
> event comes from, so you can't handle the menu click differently from header
> click, as you had to remove the following check for "hide":
> > if (aEvent.explicitOriginalTarget.id == "menu_toggleAttachmentPane" &&

Correction: Without event parameter, and target derived from that, you can no longer handle the menu click event differently from the access/shortcut key Alt+M keypress event as I did. Fyi, in this iteration - Part 1 - the attachment pane header click uses toggleAttachmentPane("focus"), so that's a straightforward code path skipping the "toggle" conditional.

Your next way out is to eliminate the convenient command="cmd_toggleAttachmentPane" attributes and use oncommand="..." with special function calls with special parameters everywhere, which will get worse for the full state machinery when clicking the header does something else again (as already described in detail in my comment 72).

> Empty bucket must definitely be accessible via access key first if it's not
> focused yet

Comment 99

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #82)
> I think Magnus was pretty clear, like the rest of us, he doesn't like the
> "variable argument" function.

I appreciate, accept and understand that you all don't like it, only I haven't heard any objective argument as to *why* my alternative usage would be bad or inferior, given that it actually does streamline and simplify all callers at the cost of a one-line conditional inside the function. I maintain that no function can be understood without reading it, so I'm failing to see the big difference between reading about arguments in the function comment and the one-line conditional or reading about them in the function definition line (which also won't tell you how arguments will be used inside the function, and fruthermore it won't even tell you which other external variables might play into the function, so really just seeing function(foo, bar) doesn't mean much...)

> Looking at the patches I can confirm that the function only ever takes one
> argument or none. So if you don't want to "mess up" the callers, you could
> do two functions which call a common worker:
> function toggleAttachmentPaneWithEvent(aEvent) {
>   toggleAttachmentPane(aEvent, null);
> }
> 
> function toggleAttachmentPaneWithAction(aAction) {
>   toggleAttachmentPane(null, aAction);
> }

That works, but seems to violate the KISS principle much more than my one-liner inside the function...

> Or maybe as a compromise do
> function toggleAttachmentPane(aArg) {
>   let action = null;
>   let event = null;
>   if (typeof(aArg) == "string") // or should that be ===?
>     action = aArg;
>   else if (typeof(aArg) == "object")
>     event = aArg.
> I think that looks simpler than your loop and detecting your event on
> arg.target.

That looks good to me, I'll be happy to go for that compromise which makes the passed argument more explicit while keeping callers clean and simple. I think we can simplify this even a bit more:

>   if (typeof(aArg) == "string") // or should that be ===?
>     action = aArg;
>   else if (aArg)
>     event = aArg.

We're not expecting any other argument types, and passing them would fail anyway, so no need to explicitly check for object type, after we've eliminated string type and undefined, it must be the event.

Any reason why typeof is better than arg.target? Otherwise we could use this:

if (aArg) {
  if (arg.target)
    event = aArg;
  else
    action = aArg;
}

Or this:

if (aArg && arg.target)
  event = aArg;
else if (aArg)
  action = aArg;

Or this:

let event = aArg && aArg.target ? aArg : null;
let action = aArg && event ? null : aArg;

Or this:

if (aArg) {
  let event = aArg.target ? aArg : null;
  let action = event ? null : aArg;
}

Or this:

if (aArg) {
  let action = typeof(aArg) == "string" ? aArg : null;
  let event = action ? null : aArg;
}

Whatever...

> In order for this not to miss the string cut off date, I will land the
> strings from both patches tonight after 6 PM (GMT+1) and also change
> attachButton.tooltip to attachButton.tooltip2 in the existing code.

Thank you very much, that's helpful and forthcoming!

Comment 100

[Jorg K (CEST = GMT+2)]

We have a couple of weeks now for part 2, so let's make it happen! We can sort out the details there.

Comment 101

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #99)
> (In reply to Jorg K (GMT+1) from comment #82)
> > I think Magnus was pretty clear, like the rest of us, he doesn't like the
> > "variable argument" function.
> 
> I appreciate, accept and understand that you all don't like it, only I
> haven't heard any objective argument as to *why* my alternative usage would
> be bad or inferior, given that it actually does streamline and simplify all
> callers at the cost of a one-line conditional inside the function.

Because magic is bad, and usually hard to maintain. 
Look at it the other way - why would the language even have named arguments at all, when you can do it the way you propose. 

Also be aware arguments work differently in strict mode...

> I maintain that no function can be understood without reading it, 

Then it is poorly documented, or does something strange.

> > Looking at the patches I can confirm that the function only ever takes one
> > argument or none. So if you don't want to "mess up" the callers, you could
> > do two functions which call a common worker:
> > function toggleAttachmentPaneWithEvent(aEvent) {
> >   toggleAttachmentPane(aEvent, null);
> > }

But it's more clear to just call toggleAttachmentPane(aEvent, null); and toggleAttachmentPane(null, aAction);

That way it's easier to understand what callers are doing when you try to figure this out.

Comment 102

[Thomas D. (:thomas8)]

Attachment: Part 2 - Patch V.3: Implement view state machinery incl. minimize/restore (rebased, improved, debug version with console.logs and test code)

As Jörg says, let's get it done... :-)

Richard, the UX of this is practically unchanged (I hope) since your last ui-r+.
But I'm asking for ui-r again so that you may please check if Aceman and I have broken anything in the process, as there has been considerable code churn here.

There's one minimal UX change which alleviates nit #1 mentioned before (end of comment 43):
- when clicking "Attach" button, a small or minimized full bucket will be restored to default width (at least; otherwise user's last custom width).
- why? Before adding more attachments, it's certainly helpful/required to see what's already in the bucket!
- nice side effect: when clicking on whitespace in full/empty bucket, it will also restore before adding more (somewhat alleviating nit #1 that file picker jumps out immediately when clicking whitespace of minimized bucket, instead of perhaps just restoring the bucket on first click and getting file picker only on second click. Otoh, that's one more click, so maybe this is acceptable).

Aceman:

1) When we're iterating to add/remove items from a visible bucket, number and size of attachments in the header must be updated immediately to reflect reality in the UI at any time, wrt correctness and in case of errors/delays when adding/removing. Updating attachment pane header isn't expensive, and I've optimized respective calls to use "show" only once as required and avoid double updating. Iirc, updating was done inside the iteration before this bug, so we shouldn't change that.

2) updateAttachmentPane("show/hide") is no longer used, can I remove the parameter from that function? Reason: with "show", it was calling toggleAttachmentPane("show"), which in turn calls updateAttachmentPane() again without parameters. Hence I replaced respective calls with toggleAttachmentPane("show") which runs show and update exactly once. I never liked the combination of update and show, these are two different things.

3) I've used Jörg's compromise proposal of comment 82 with explicit function parameter, combining the best of both worlds, transparency AND minimalist callers. We need event to know where the user action is coming from, there are subtle differences between menu click, header click, and Alt+M, to match user's legitimate expectations when using them (and the "toggle" approach did not cover that, along comment 97). Without event, we'd have to make all callers more complex/specific and deviate from the regular, generic command="cmd_id" implementation.

Comment 103

[Thomas D. (:thomas8)]

Comment on attachment 8957898 [details] [diff] [review]
Part 1 - Patch V. 4.4: (more polish/nitfixes) Fast-track backend and UX goodies w/o view state machinery [LANDED in comment 87]

Thank you for fixing some of the remaining nits.

Unfortunately, the "toggle" approach doesn't fully cover the desired behaviour, and it would become even more complicated now with the full state machinery.
For details, see comment 97, comment 98, and comment 102 with new patch where I modified some of this.

Same for .collapsed, the full state machinery isn't binary so definitely needs bucket.width to work as designed, unless if we rewrite the whole thing and save the current view state somewhere else.

Comment 104

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #102)
> Created attachment 8958397 [details] [diff] [review]

> There's one minimal UX change which alleviates nit #1 mentioned before (end
> of comment 43):
> - when clicking "Attach" button, a small or minimized full bucket will be
> restored to default width (at least; otherwise user's last custom width).

Nit: Maybe I could try to make it so that we only restore to default width for this case, because restoring custom width larger than default width isn't very helpful as it's likely to be hidden behind the file picker dialogue. Then we need to decide if we want to remember user's larger custom width (2 clicks on header to retrieve that: 1) default -> minimized; 2) minimized -> large custom width) or we take default width as new custom width and forget user's larger custom width. Thoughts?

Comment 105

[:aceman]

Comment on attachment 8958397 [details] [diff] [review]
Part 2 - Patch V.3: Implement view state machinery incl. minimize/restore (rebased, improved, debug version with console.logs and test code)

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

I need to see what this is actually wanting to do. When does it minimize/restore and how can the user change the width of the bucket.
It seems to me again to be too much code for some eye candy.
I have gone through all the comments mentioning "minimize", but can't really find what it actually is.
Does the user make the bucket very small using the splitter? What is that good for and why we want to even enhance the experience of that?
Currenly with one recipient per line, there is a ton of free space for the bucket :)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4871,5 @@
> +  // When user is about to move the splitter, start listening for mouse moves
> +  // and keep checking if the bucket has been minimized so as to update the
> +  // minimized bucket header accordingly.
> +  document.addEventListener("mousemove", attachmentPaneSizerOnMouseMove, true);
> +  document.addEventListener("mouseup", attachmentPaneSizerOnMouseUp, {capture: false, once: true});

Can't you set up the listeners once at startup?

@@ +4878,5 @@
> +function attachmentPaneSizerOnMouseMove()
> +{
> +  // Using attachmentbucket-sizer splitter can minimize the attachment bucket,
> +  // and then we want to update the header of the bucket accordingly.
> +  updateAttachmentPane();

Do you really need to do this for every move when user didn't release the mouse yet? Responsive design, I know ;)

@@ +4927,5 @@
> +  // words). Empty bucket can't have scrollbars so exact scrollbar width doesn't
> +  // matter, but we need to give the splitter-dragging user a landing range for
> +  // getting the mini header as empty bucket collapses when dragging beyond
> +  // gBucketMinWidthDefault.
> +  if (count && bucketWidth > parseInt(bucket.minWidth) ||

This would be more readable with some brackets added. count && x || !count? I could guess '||' has lower priority than '&&', but still...

@@ +4928,5 @@
> +  // matter, but we need to give the splitter-dragging user a landing range for
> +  // getting the mini header as empty bucket collapses when dragging beyond
> +  // gBucketMinWidthDefault.
> +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> +      !count && bucketWidth > gBucketMinWidthDefault + 17 ||

What is this magic value of 17 and how did you arrive at it? I surely do not like it.

@@ +4949,5 @@
> +      (count > 0 || parseInt(bucket.boxObject.width) == parseInt(bucket.minWidth))
> +    ? gMessenger.formatFileSize(gAttachmentsSize)
> +    : "";
> +  document.getElementById("attachmentBucketCloseButton").collapsed =
> +    (count == 0) ? false : true;

Why isn't (count != 0) enough?

@@ +4980,5 @@
>    for (let i = bucket.selectedCount - 1; i >= 0; i--) {
>      let item = bucket.removeItemAt(bucket.getIndexOfItem(bucket.getSelectedItem(i)));
>      if (item.attachment.size != -1) {
>        gAttachmentsSize -= item.attachment.size;
> +      updateAttachmentPane();

You really want this animation of user-visible decreasing size counter, slowing down deleting 100s of attachments? :)

@@ +5354,5 @@
> +  // (next step: hide), but "not yet fully visible" for headerClick and shortcut
> +  // key operation (next step: show).
> +
> +  switch (true) {
> +    case !!aAction:

More syntax experiments? :) You would get a fat warning/error from a proper C++ compiler that the case values are not in the range of the switch variable, or that you do not cover all the possible values with cases (e.g. in case of an enum).
It seems you are just misusing switch instead of 'if () elseif' cascades :)

@@ +5747,5 @@
> +  bucket.style.overflow = originalOverflow;
> +*/
> +}
> +
> +function attachmentBucketUpdateMinWidth() {

Why is this function needed?
can't we just set the minwidth of the bucket to some 4em which could fit the icon and 1-2 leading chars of the attachment and not care whether scrollbars are shown? If they are not, we get to se 2 more chars.
Or is this particularly tuned to only ever shown the icon, so you need to tune the size to scrollerSize (if any) + 16px for icon? But I do not see any such constant here (of course you would be getting icon size from the css).

@@ +5809,5 @@
> +  let defaultWidth = parseFloat(defaultWidthCSS);
> +  let defaultFontSize;
> +  if (defaultWidthCSS.toLowerCase().includes("em")) {
> +    defaultFontSize = parseFloat(getComputedStyle(bucket).fontSize);
> +    defaultWidth = defaultFontSize * defaultWidth;

What? Doesn't the style engine give you the px value directly with some call?

@@ +5881,5 @@
>    let bucketSizer = document.getElementById("attachmentbucket-sizer");
>    if (attachmentBucket.itemCount > 0) {
>      attachmentBucket.removeAttribute("empty");
> +    // Restore default minWidth which we adjusted below if still applied.
> +    if (attachmentBucket.minWidth == gBucketMinWidthDefault - 4) {

Magic values again?

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +999,5 @@
>  }
>  
>  function awSizerMouseMove()
>  {
> +  // Using awSizer splitter can toggle vertical toolbar in attachment bucket,

You mean scrollbar?

::: mail/themes/windows/mail/compose/messengercompose.css
@@ +12,5 @@
>  @namespace html url("http://www.w3.org/1999/xhtml");
>  
> +#attachments-header-box {
> +  overflow:hidden;
> +  min-height: 27px;

Why 27px?

Comment 106

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #105)
> Comment on attachment 8958397 [details] [diff] [review]
> Part 2 - Patch V.3: Implement view state machinery incl. minimize/restore
> (rebased, improved, debug version with console.logs and test code)
> 
> Review of attachment 8958397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I need to see what this is actually wanting to do. When does it
> minimize/restore and how can the user change the width of the bucket.
> It seems to me again to be too much code for some eye candy.

There's plenty of outcommented code, plenty of console.logs, and plenty of comments to explain what we're doing. Remove all of those, and the remaining code is actually very little.
I have improved a lot of different UX aspects here... E.g. this offers a significant improvement of ux-efficiency especially for keyboard-centric users, and one key idea suggested and promoted by Jörg is ux-error-prevention: To avoid the current scenario where users can collapse full bucket and then forget that there are attachments when sending, which can cause massive breach of privacy / data leaks. This is definitely more than eye candy.
And honestly, as we have very little to offer in terms of ux improvements, I think even a little bit of eye candy would be more than welcome.

Please let's not get into the habit of dismissing any improvement just because it needs a bit of code. If we want to strive for awesome UX, we can't keep counting lines. But as I said, it's actually few lines for all the improvements which I'm shipping.

> I have gone through all the comments mentioning "minimize", but can't really
> find what it actually is.
> Does the user make the bucket very small using the splitter? What is that
> good for and why we want to even enhance the experience of that?

I think this has all been explained, discussed, tried and tested already many times in this bug by me and others so I won't repeat all that and then get blamed for lengthy comments... ;-)

> Currenly with one recipient per line, there is a ton of free space for the
> bucket :)

Pls consult with Jörg who requested this feature in bug 1427092 comment 18, and Richard who also proposed it in this bug 1428631 comment 19... You've been on board all along, isn't it a bit late now to start discarding the idea?
Let's be aware that not everybody is running on big screens. Even with big screen, I can shrink TB to share the screen with some other app and immediately space becomes an issue. 

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +4871,5 @@
> > +  // When user is about to move the splitter, start listening for mouse moves
> > +  // and keep checking if the bucket has been minimized so as to update the
> > +  // minimized bucket header accordingly.
> > +  document.addEventListener("mousemove", attachmentPaneSizerOnMouseMove, true);
> > +  document.addEventListener("mouseup", attachmentPaneSizerOnMouseUp, {capture: false, once: true});
> 
> Can't you set up the listeners once at startup?

Why waste resources to listen all the time when listening while splitter is dragged is just enough?

> @@ +4878,5 @@
> > +function attachmentPaneSizerOnMouseMove()
> > +{
> > +  // Using attachmentbucket-sizer splitter can minimize the attachment bucket,
> > +  // and then we want to update the header of the bucket accordingly.
> > +  updateAttachmentPane();
> 
> Do you really need to do this for every move when user didn't release the
> mouse yet? Responsive design, I know ;)

Yes. So you know the answer.

> @@ +4927,5 @@(In reply to :aceman from comment #105)
> Comment on attachment 8958397 [details] [diff] [review]
> Part 2 - Patch V.3: Implement view state machinery incl. minimize/restore
> (rebased, improved, debug version with console.logs and test code)
> 
> Review of attachment 8958397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I need to see what this is actually wanting to do. When does it
> minimize/restore and how can the user change the width of the bucket.
> It seems to me again to be too much code for some eye candy.

There's plenty of outcommented code, plenty of console.logs, and plenty of comments to explain what we're doing. Remove all of those, and the remaining code is actually very little.
I have improved a lot of different UX aspects here... E.g. this offers a significant improvement of ux-efficiency especially for keyboard-centric users, and one key idea suggested and promoted by Jörg is ux-error-prevention: To avoid the current scenario where users can collapse full bucket and then forget that there are attachments when sending, which can cause massive breach of privacy / data leaks. This is definitely more than eye candy.
And honestly, as we have very little to offer in terms of ux improvements, I think even a little bit of eye candy would be more than welcome.

Please let's not get into the habit of dismissing any improvement just because it needs a bit of code. If we want to strive for awesome UX, we can't keep counting lines. But as I said, it's actually few lines for all the improvements which I'm shipping.

> I have gone through all the comments mentioning "minimize", but can't really
> find what it actually is.
> Does the user make the bucket very small using the splitter? What is that
> good for and why we want to even enhance the experience of that?

I think this has all been explained, discussed, tried and tested already many times in this bug by me and others so I won't repeat all that and then get blamed for lengthy comments... ;-)

> Currenly with one recipient per line, there is a ton of free space for the
> bucket :)

Pls consult with Jörg who requested this feature in bug 1427092 comment 18, and Richard who also proposed it in this bug 1428631 comment 19... You've been on board all along, isn't it a bit late now to start discarding the idea?
Let's be aware that not everybody is running on big screens. Even with big screen, I can shrink TB to share the screen with some other app and immediately space becomes an issue. 

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +4871,5 @@
> > +  // When user is about to move the splitter, start listening for mouse moves
> > +  // and keep checking if the bucket has been minimized so as to update the
> > +  // minimized bucket header accordingly.
> > +  document.addEventListener("mousemove", attachmentPaneSizerOnMouseMove, true);
> > +  document.addEventListener("mouseup", attachmentPaneSizerOnMouseUp, {capture: false, once: true});
> 
> Can't you set up the listeners once at startup?

Why waste resources to listen all the time when listening while splitter is dragged is just enough?

> @@ +4878,5 @@
> > +function attachmentPaneSizerOnMouseMove()
> > +{
> > +  // Using attachmentbucket-sizer splitter can minimize the attachment bucket,
> > +  // and then we want to update the header of the bucket accordingly.
> > +  updateAttachmentPane();
> 
> Do you really need to do this for every move when user didn't release the
> mouse yet? Responsive design, I know ;)

Yes. So you know the answer.

> @@ +4927,5 @@
> > +  // words). Empty bucket can't have scrollbars so exact scrollbar width doesn't
> > +  // matter, but we need to give the splitter-dragging user a landing range for
> > +  // getting the mini header as empty bucket collapses when dragging beyond
> > +  // gBucketMinWidthDefault.
> > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> 
> This would be more readable with some brackets added. count && x || !count?
> I could guess '||' has lower priority than '&&', but still...

Well, I think Jörg was telling me to skip any superfluous brackets the other day...
It's not about guessing, precedence rules are public and clear and certainly won't ever change.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence
But yeah, we could add some brackets here...
I also doubt anyone is going to read this code again once it's there up and running...

> @@ +4928,5 @@
> > +  // matter, but we need to give the splitter-dragging user a landing range for
> > +  // getting the mini header as empty bucket collapses when dragging beyond
> > +  // gBucketMinWidthDefault.
> > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> > +      !count && bucketWidth > gBucketMinWidthDefault + 17 ||
> 
> What is this magic value of 17 and how did you arrive at it? I surely do not
> like it.

17 happens to be the default width of a vertical toolbar, but as I tried to explain in the code comment, the exact width doesn't matter here because it's just about allowing the user a bit of fuzzy landing range when minimizing an empty bucket. Without fuzzy landing, there'll be a sharp cut-off where the bucket suddenly disappears without any prior notice / hint in the UI.

> @@ +4949,5 @@
> > +      (count > 0 || parseInt(bucket.boxObject.width) == parseInt(bucket.minWidth))
> > +    ? gMessenger.formatFileSize(gAttachmentsSize)
> > +    : "";
> > +  document.getElementById("attachmentBucketCloseButton").collapsed =
> > +    (count == 0) ? false : true;
> 
> Why isn't (count != 0) enough?

Oh yes, now I see what you mean.

> @@ +4980,5 @@
> >    for (let i = bucket.selectedCount - 1; i >= 0; i--) {
> >      let item = bucket.removeItemAt(bucket.getIndexOfItem(bucket.getSelectedItem(i)));
> >      if (item.attachment.size != -1) {
> >        gAttachmentsSize -= item.attachment.size;
> > +      updateAttachmentPane();
> 
> You really want this animation of user-visible decreasing size counter,
> slowing down deleting 100s of attachments? :)

I think it was there before, and I think 100 attachments is an unlikely edge case scenario, and even with that this functions isn't expensive so I

> @@ +5354,5 @@
> > +  // (next step: hide), but "not yet fully visible" for headerClick and shortcut
> > +  // key operation (next step: show).
> > +
> > +  switch (true) {
> > +    case !!aAction:
> 
> More syntax experiments? :) You would get a fat warning/error from a proper
> C++ compiler that the case values are not in the range of the switch
> variable, or that you do not cover all the possible values with cases (e.g.
> in case of an enum).
> It seems you are just misusing switch instead of 'if () elseif' cascades :)
> 
> @@ +5747,5 @@
> > +  bucket.style.overflow = originalOverflow;
> > +*/
> > +}
> > +
> > +function attachmentBucketUpdateMinWidth() {
> 
> Why is this function needed?
> can't we just set the minwidth of the bucket to some 4em which could fit the
> icon and 1-2 leading chars of the attachment and not care whether scrollbars
> are shown? If they are not, we get to se 2 more chars.
> Or is this particularly tuned to only ever shown the icon, so you need to
> tune the size to scrollerSize (if any) + 16px for icon? But I do not see any
> such constant here (of course you would be getting icon size from the css).
> 
> @@ +5809,5 @@
> > +  let defaultWidth = parseFloat(defaultWidthCSS);
> > +  let defaultFontSize;
> > +  if (defaultWidthCSS.toLowerCase().includes("em")) {
> > +    defaultFontSize = parseFloat(getComputedStyle(bucket).fontSize);
> > +    defaultWidth = defaultFontSize * defaultWidth;
> 
> What? Doesn't the style engine give you the px value directly with some call?
> 
> @@ +5881,5 @@
> >    let bucketSizer = document.getElementById("attachmentbucket-sizer");
> >    if (attachmentBucket.itemCount > 0) {
> >      attachmentBucket.removeAttribute("empty");
> > +    // Restore default minWidth which we adjusted below if still applied.
> > +    if (attachmentBucket.minWidth == gBucketMinWidthDefault - 4) {
> 
> Magic values again?
> 
> ::: mail/components/compose/content/addressingWidgetOverlay.js
> @@ +999,5 @@
> >  }
> >  
> >  function awSizerMouseMove()
> >  {
> > +  // Using awSizer splitter can toggle vertical toolbar in attachment bucket,
> 
> You mean scrollbar?
> 
> ::: mail/themes/windows/mail/compose/messengercompose.css
> @@ +12,5 @@
> >  @namespace html url("http://www.w3.org/1999/xhtml");
> >  
> > +#attachments-header-box {
> > +  overflow:hidden;
> > +  min-height: 27px;
> 
> Why 27px?


> > +  // words). Empty bucket can't have scrollbars so exact scrollbar width doesn't
> > +  // matter, but we need to give the splitter-dragging user a landing range for
> > +  // getting the mini header as empty bucket collapses when dragging beyond
> > +  // gBucketMinWidthDefault.
> > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> 
> This would be more readable with some brackets added. count && x || !count?
> I could guess '||' has lower priority than '&&', but still...

Well, I think Jörg was telling me to skip any superfluous brackets the other day...
It's not about guessing, precedence rules are public and clear and certainly won't ever change.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence
But yeah, we could add some brackets here...
I also doubt anyone is going to read this code again once it's there up and running...

> @@ +4928,5 @@
> > +  // matter, but we need to give the splitter-dragging user a landing range for
> > +  // getting the mini header as empty bucket collapses when dragging beyond
> > +  // gBucketMinWidthDefault.
> > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> > +      !count && bucketWidth > gBucketMinWidthDefault + 17 ||
> 
> What is this magic value of 17 and how did you arrive at it? I surely do not
> like it.

17 happens to be the default width of a vertical toolbar, but as I tried to explain in the code comment, the exact width doesn't matter here because it's just about allowing the user a bit of fuzzy landing range when minimizing an empty bucket. Without fuzzy landing, there'll be a sharp cut-off where the bucket suddenly disappears without any prior notice / hint in the UI.

> @@ +4949,5 @@
> > +      (count > 0 || parseInt(bucket.boxObject.width) == parseInt(bucket.minWidth))
> > +    ? gMessenger.formatFileSize(gAttachmentsSize)
> > +    : "";
> > +  document.getElementById("attachmentBucketCloseButton").collapsed =
> > +    (count == 0) ? false : true;
> 
> Why isn't (count != 0) enough?

Oh yes, now I see what you mean.

> @@ +4980,5 @@
> >    for (let i = bucket.selectedCount - 1; i >= 0; i--) {
> >      let item = bucket.removeItemAt(bucket.getIndexOfItem(bucket.getSelectedItem(i)));
> >      if (item.attachment.size != -1) {
> >        gAttachmentsSize -= item.attachment.size;
> > +      updateAttachmentPane();
> 
> You really want this animation of user-visible decreasing size counter,
> slowing down deleting 100s of attachments? :)

I think it was there before, and I think 100 attachments is an unlikely edge case scenario, and even with that I wouldn't think that this functions is expensive enough to cause noticable delays. I might be wrong. I'd rather optimize the updateAttachmentPane() function to be faster than removing it here.

> @@ +5354,5 @@
> > +  // (next step: hide), but "not yet fully visible" for headerClick and shortcut
> > +  // key operation (next step: show).
> > +
> > +  switch (true) {
> > +    case !!aAction:
> 
> More syntax experiments? :) You would get a fat warning/error from a proper
> C++ compiler that the case values are not in the range of the switch
> variable, or that you do not cover all the possible values with cases (e.g.
> in case of an enum).
> It seems you are just misusing switch instead of 'if () elseif' cascades :)

This is correct javascript, working well, reliable, concise and easy to read. I don't and I can't care about C++ compilers when I'm writing javascript. No misuse here, just intelligent and more readable notation.
Try to do without - given the lack of labelled goto statements in javascript, this would require really odd workarounds to skip the entire switch. And nested if/else are harder to write, harder to read, and not helping anyone.
So I don't see the point of criticizing this.
That said, would you prefer the following?

if (!aAction) {
  switch (true) {
  ...
  ...
  ...
  }
}

> @@ +5747,5 @@
> > +  bucket.style.overflow = originalOverflow;
> > +*/
> > +}
> > +
> > +function attachmentBucketUpdateMinWidth() {
> 
> Why is this function needed?
> can't we just set the minwidth of the bucket to some 4em which could fit the
> icon and 1-2 leading chars of the attachment and not care whether scrollbars
> are shown? If they are not, we get to se 2 more chars.

I've tried that and it looked very amateurish. Also explained in code comments.

> Or is this particularly tuned to only ever shown the icon, so you need to
> tune the size to scrollerSize (if any) + 16px for icon? But I do not see any
> such constant here (of course you would be getting icon size from the css).

Yes, it's finetuned to only ever show the icons, not more, not less.
I'm using 2 em which is icon size with needed margins, but if there's a better way to set the minWidth, pls advise.
Then I'm calculating the scrollbar width and auto-balancing that out when it appears or disappears.
I've tried very hard to find an easier way of getting the scrollbar width (see outcommented code), but failed radically. Those anonymous elements are very hard to access, even using their special methods.

> @@ +5809,5 @@
> > +  let defaultWidth = parseFloat(defaultWidthCSS);
> > +  let defaultFontSize;
> > +  if (defaultWidthCSS.toLowerCase().includes("em")) {
> > +    defaultFontSize = parseFloat(getComputedStyle(bucket).fontSize);
> > +    defaultWidth = defaultFontSize * defaultWidth;
> 
> What? Doesn't the style engine give you the px value directly with some call?

I couldn't find a better way - can you?

> @@ +5881,5 @@
> >    let bucketSizer = document.getElementById("attachmentbucket-sizer");
> >    if (attachmentBucket.itemCount > 0) {
> >      attachmentBucket.removeAttribute("empty");
> > +    // Restore default minWidth which we adjusted below if still applied.
> > +    if (attachmentBucket.minWidth == gBucketMinWidthDefault - 4) {
> 
> Magic values again?

Same purpose as above, an arbitrary fuzzy landing zone for dragging the splitter so that the empty bucket does not disappear suddenly before it's correctly minimized. Otherwise it's hard for humans to drag this pixel-perfect.

> ::: mail/components/compose/content/addressingWidgetOverlay.js
> @@ +999,5 @@
> >  }
> >  
> >  function awSizerMouseMove()
> >  {
> > +  // Using awSizer splitter can toggle vertical toolbar in attachment bucket,
> 
> You mean scrollbar?

Yes, I mixed the terms several times.

> ::: mail/themes/windows/mail/compose/messengercompose.css
> @@ +12,5 @@
> >  @namespace html url("http://www.w3.org/1999/xhtml");
> >  
> > +#attachments-header-box {
> > +  overflow:hidden;
> > +  min-height: 27px;
> 
> Why 27px?

That's what makes it look in line with the visible line below the sender, and same height as contacts side bar header.

Comment 107

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #106)
Sorry for the mess in comment #106, don't know what happened but somehow it ended up as duplicated text...

Comment 108

[Richard Marti (:Paenglab)]

Comment on attachment 8958397 [details] [diff] [review]
Part 2 - Patch V.3: Implement view state machinery incl. minimize/restore (rebased, improved, debug version with console.logs and test code)

ui-r- because you reset the users chosen attachment pane width. The user has maybe chosen this to always see the full attachment and he has plenty of space because he uses it fullscreen on a wide monitor or what reason ever. Because of this, we shouldn't reset this.

The other parts looks as they aren't changed. Now only tested on Windows.

Comment 109

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #108)
> Comment on attachment 8958397 [details] [diff] [review]
> Part 2 - Patch V.3: Implement view state machinery incl. minimize/restore
> (rebased, improved, debug version with console.logs and test code)
> 
> ui-r- because you reset the users chosen attachment pane width. The user has
> maybe chosen this to always see the full attachment and he has plenty of
> space because he uses it fullscreen on a wide monitor or what reason ever.
> Because of this, we shouldn't reset this.

No problem, will fix that in the next iteration; you can try it by just adding this line:

> function attachmentBucketInitializeWidthGlobals() {
> ...
>   // Initialize attachment bucket's user width to persisted box width.
>   gBucketWidthUser = attachmentsBox.width;
>   console.log('gBucketWidthUser = attachmentsBox.width = ' + gBucketWidthUser);
>
>
>   // Remember attachment bucket's default min-width from css.

Comment 110

[Jorg K (CEST = GMT+2)]

Should we rename updateAttachmentPane() back to UpdateAttachmentBucket() since it breaks at least one popular add-on?

Comment 111

[Jorg K (CEST = GMT+2)]

A Google search turned up:
Enigmail needs to call UpdateAttachmentBucket(true) to update the attachment count and total size. So I'm renaming it back before we get into more trouble.

Comment 112

[Jorg K (CEST = GMT+2)]

Attachment: 1428631-UpdateAttachmentBucket.patch [LANDED in comment 114]

Comment 113

[:aceman]

Comment on attachment 8959466 [details] [diff] [review]
1428631-UpdateAttachmentBucket.patch [LANDED in comment 114]

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

Yeah, thanks for finding out that due to the changed arguments it needs to be a wrapper function, not just a rename.
Enigmail could probably fix this fast as an active addon, but others could be problematic.

Comment 114

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/632269352274
Follow-up: Reinstate UpdateAttachmentBucket() which is needed by add-ons. r=aceman

Comment 115

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959466 [details] [diff] [review]
1428631-UpdateAttachmentBucket.patch [LANDED in comment 114]

Beta (TB 60) for this reinstatement:
https://hg.mozilla.org/releases/comm-beta/rev/2c9749f769d899e14a38fc027551f541c3112a98

I've tested it with the image resizer add-on, appears to the working ;-)

Comment 116

[Thomas D. (:thomas8)]

Attachment: Part 2 - Patch V.3.1: Implement view state machinery incl. minimize/restore (nitfixes, removed console.logs and test code)

Removed all the console.logs and dead code for a much leaner patch.
Address Aceman's review comment 105 per comment 106.
Nitfix Richard's ui-r comment 108.

Comment 117

[Thomas D. (:thomas8)]

Comment on attachment 8959669 [details] [diff] [review]
Part 2 - Patch V.3.1: Implement view state machinery incl. minimize/restore (nitfixes, removed console.logs and test code)

That went wrong.

Comment 118

[Thomas D. (:thomas8)]

Attachment: Part 2 - Patch V.3.2: Implement view state machinery incl. minimize/restore (rebased, nitfixes, removed console.logs and test code)

Rebased.
Removed all the console.logs and dead code for a much leaner patch.
Address Aceman's review comment 105 per comment 106.
Nitfix Richard's ui-r comment 108.

Comment 119

[:aceman]

(In reply to Thomas D. from comment #106)
> (In reply to :aceman from comment #105)
> And honestly, as we have very little to offer in terms of ux improvements, I
> think even a little bit of eye candy would be more than welcome.

Sure, but not with 10 hooks everywhere throughout the code to get some tiny effect.
 
> Please let's not get into the habit of dismissing any improvement just
> because it needs a bit of code. If we want to strive for awesome UX, we
> can't keep counting lines. But as I said, it's actually few lines for all
> the improvements which I'm shipping.

A bit of code is fine. But it is always a tradeoff that we need to judge. You know we are mostly in maintenance mode and the amount of code we need to manage matters.

> I think this has all been explained, discussed, tried and tested already
> many times in this bug by me and others so I won't repeat all that and then
> get blamed for lengthy comments... ;-)

Somehow I haven't found the explanation.

> > Currenly with one recipient per line, there is a ton of free space for the
> > bucket :)
> Pls consult with Jörg who requested this feature in bug 1427092 comment 18,
> and Richard who also proposed it in this bug 1428631 comment 19... You've
> been on board all along, isn't it a bit late now to start discarding the
> idea?

Good, then I can let Jorg review this :)

> Let's be aware that not everybody is running on big screens. Even with big
> screen, I can shrink TB to share the screen with some other app and
> immediately space becomes an issue. 

OK. But if you minimized the bucket and then click it again (or add an attachment), it expands back. So what is the space saving?
 
> > ::: mail/components/compose/content/MsgComposeCommands.js
> > @@ +4871,5 @@
> > > +  document.addEventListener("mousemove", attachmentPaneSizerOnMouseMove, true);
> > > +  document.addEventListener("mouseup", attachmentPaneSizerOnMouseUp, {capture: false, once: true});
> > 
> > Can't you set up the listeners once at startup?
> 
> Why waste resources to listen all the time when listening while splitter is
> dragged is just enough?

But it would simplify the code :) Also setting up and removing listeners takes some resources at a place where you do not want to cause more visual non-smoothness of dragging due to running JS.

> > @@ +4878,5 @@
> > > +function attachmentPaneSizerOnMouseMove()
> > > +{
> > > +  // Using attachmentbucket-sizer splitter can minimize the attachment bucket,
> > > +  // and then we want to update the header of the bucket accordingly.
> > > +  updateAttachmentPane();
> > 
> > Do you really need to do this for every move when user didn't release the
> > mouse yet? Responsive design, I know ;)
> 
> Yes. So you know the answer.

Does not mean I agree with it. Calling it in attachmentPaneSizerOnMouseUp() would be fine with me.
The only difference is that the 3 dots from 'attachment' vanish while the user has hit the minimum width, but still holding the mouse. In my version they would vanish after he releases the mouse.
If there were some 100px region where the labels would change already, but the user could drag further, there would be a sense. But when the change happens only when the bucket is at the minimum size, calling the function seems unneeded.
 
> > @@ +4927,5 @@
> > > +  // words). Empty bucket can't have scrollbars so exact scrollbar width doesn't
> > > +  // matter, but we need to give the splitter-dragging user a landing range for
> > > +  // getting the mini header as empty bucket collapses when dragging beyond
> > > +  // gBucketMinWidthDefault.
> > > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> > 
> > This would be more readable with some brackets added. count && x || !count?
> > I could guess '||' has lower priority than '&&', but still...
> 
> Well, I think Jörg was telling me to skip any superfluous brackets the other
> day...
> It's not about guessing, precedence rules are public and clear and certainly
> won't ever change.

Yeah, but when we need to think in several different languages...

> But yeah, we could add some brackets here...

Please do.

> I also doubt anyone is going to read this code again once it's there up and
> running...

This isn't the best attitude ;)
 
> > @@ +4928,5 @@
> > > +  // matter, but we need to give the splitter-dragging user a landing range for
> > > +  // getting the mini header as empty bucket collapses when dragging beyond
> > > +  // gBucketMinWidthDefault.
> > > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> > > +      !count && bucketWidth > gBucketMinWidthDefault + 17 ||
> > 
> > What is this magic value of 17 and how did you arrive at it? I surely do not
> > like it.
> 
> 17 happens to be the default width of a vertical toolbar,

I think Paenglab would know better whether this is safe to assume. There can be different themes or spaced up UI (hi DPI) where the size would be different.
You must get the value from the widget, not hard-code it here. 
 
> > @@ +5354,5 @@
> > > +  // (next step: hide), but "not yet fully visible" for headerClick and shortcut
> > > +  // key operation (next step: show).
> > > +
> > > +  switch (true) {
> > > +    case !!aAction:
> > 
> > More syntax experiments? :) You would get a fat warning/error from a proper
> > C++ compiler that the case values are not in the range of the switch
> > variable, or that you do not cover all the possible values with cases (e.g.
> > in case of an enum).
> > It seems you are just misusing switch instead of 'if () elseif' cascades :)
> 
> This is correct javascript, working well, reliable, concise and easy to
> read. I don't and I can't care about C++ compilers when I'm writing
> javascript.

The Javascript compiler is getting stricter to allow it to get faster.

> No misuse here, just intelligent and more readable notation.
> Try to do without - given the lack of labelled goto statements in
> javascript, this would require really odd workarounds to skip the entire
> switch. And nested if/else are harder to write, harder to read, and not
> helping anyone.

I see no advantage of using this hack instead of proper else if branches. No gotos or workarounds needed.

> So I don't see the point of criticizing this.
> That said, would you prefer the following?
> 
> if (!aAction) {
>   switch (true) {
>   ...
>   ...
>   ...
>   }
> }

NO, I want to kill the switch.
And if you really want to remove the 'if (!aAction) { }' indentation level, you can do:
if (aAction) {
 // do nothing
} else if (bucketWidth == parseFloat(bucket.minWidth)) {
...
} else if { ... }

> > Or is this particularly tuned to only ever shown the icon, so you need to
> > tune the size to scrollerSize (if any) + 16px for icon? But I do not see any
> > such constant here (of course you would be getting icon size from the css).
> 
> Yes, it's finetuned to only ever show the icons, not more, not less.
> I'm using 2 em which is icon size with needed margins, but if there's a
> better way to set the minWidth, pls advise.
> Then I'm calculating the scrollbar width and auto-balancing that out when it
> appears or disappears.
> I've tried very hard to find an easier way of getting the scrollbar width
> (see outcommented code), but failed radically. Those anonymous elements are
> very hard to access, even using their special methods.

Why would it be useful to only show the icon?
Notice some attachments even have no icon.
Even then, your 2em do not work for what you want. It shows the icon and also some pixels of the highlight of the selected item.

> > @@ +5809,5 @@
> > > +  let defaultWidth = parseFloat(defaultWidthCSS);
> > > +  let defaultFontSize;
> > > +  if (defaultWidthCSS.toLowerCase().includes("em")) {
> > > +    defaultFontSize = parseFloat(getComputedStyle(bucket).fontSize);
> > > +    defaultWidth = defaultFontSize * defaultWidth;
> > 
> > What? Doesn't the style engine give you the px value directly with some call?
> 
> I couldn't find a better way - can you?

Why not GetMsgAttachmentElement().width or similar?

> > @@ +5881,5 @@
> > >    let bucketSizer = document.getElementById("attachmentbucket-sizer");
> > >    if (attachmentBucket.itemCount > 0) {
> > >      attachmentBucket.removeAttribute("empty");
> > > +    // Restore default minWidth which we adjusted below if still applied.
> > > +    if (attachmentBucket.minWidth == gBucketMinWidthDefault - 4) {
> > 
> > Magic values again?
> 
> Same purpose as above, an arbitrary fuzzy landing zone for dragging the
> splitter so that the empty bucket does not disappear suddenly before it's
> correctly minimized. Otherwise it's hard for humans to drag this
> pixel-perfect.

I do not understand. When would the bucket disappear? The patch enforces the 2em minimum size. Am I missing something?
 
> > ::: mail/themes/windows/mail/compose/messengercompose.css
> > @@ +12,5 @@
> > >  @namespace html url("http://www.w3.org/1999/xhtml");
> > >  
> > > +#attachments-header-box {
> > > +  overflow:hidden;
> > > +  min-height: 27px;
> > 
> > Why 27px?
> 
> That's what makes it look in line with the visible line below the sender,
> and same height as contacts side bar header.

OK, then add a comment referencing the 27px value at the other element and saying they must be the same. Otherwise theme authors may mess it up.

Comment 120

[Jorg K (CEST = GMT+2)]

Hmm, I don't really have the time to read through all the arguments, so let me just state what I'd like to see.

Alt+M is great right now, it makes the bucket show and empty buckets disappear.
With a non-empty bucket it should just shrink the bucket down to a fixed minimum size, no trickery taking scrollbars into account.

The manual slider should just be able to reduce the width to the minimum size when the bucket is non-empty.

Can we have a simple solution like this within a week so we can ship that in beta 2?

Comment 121

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: without scrollbar trickery - a) scrollbar hiding icons, b) cut-off pane content and header

(In reply to Jorg K (GMT+1) from comment #120)
> Hmm, I don't really have the time to read through all the arguments, so let
> me just state what I'd like to see.
> 
> Alt+M is great right now, it makes the bucket show and empty buckets
> disappear.
> With a non-empty bucket it should just shrink the bucket down to a fixed
> minimum size, no trickery taking scrollbars into account.

It's unfortunate that by not reading my comments, you're forcing me to repeat myself, then people claim that my comments are lengthy and repetitive... 

As mentioned before, not having scrollbar trickery makes us look like amateurs who don't know what they are doing, with cut-off hence useless strings (pane content and pane header), and preventing the user from properly minimizing the bucket to icon-size only, as suggested by Richard. Without trickery, we can't allow shrinking to actual icon-width, because user can then get into a state where the vertical scrollbar covers 80% of the icons, which is ugly and unprofessional again. The net result is an unbaked feature going only half-way - nicht Fisch, nicht Fleisch. Please check out the attached screenshot which depicts both problems. I don't want to be associated with making TB UI a laughing stock, i.e. without the scrollbar trickery, I may have to abandon my patch here.

> Can we have a simple solution like this within a week so we can ship that in
> beta 2?

I have offered a perfectly working patch which has Richard's ui-r+.
So if accepted by review, this is ready to ship immediately.

I'm surprised we suddenly seem to have new unwritten regulations that we can't add a bit of code to fine-tune the UI/UX, and if taken as an absolute, I think that's taking us in the wrong direction, allowing only primitive and non-polished UI/UX. I'm even more surprised that TB out of all applications believes it can afford that attitude, given that the number of UX-improvements is pretty low anyway, as we're mostly just doing maintenance, as Aceman correctly points out.

I can totally appreciate that not everyone would be willing to go that extra mile of polishing the UX, but I think refusing polished UX when it's offered does not make sense. I'll gladly prove how the hooks I added are not affecting general performance or memory need in any significant way, as they are mostly only going into action when the user is actually using the splitters which trigger the behaviour here, so after that move, we're back to zero costs.

I also don't buy the "maintenance burden" argument as a) this could be said for *any* new code, and b) there's really nothing to maintain here, the code will just live happily in our code base ever after and no one will ever touch it, because it's so specific and clearly limited to our purposes of finetuning the UI/UX here, and also because no one else is likely to do any UI/UX changes because - well - we're in maintenance mode... My code is well-documented, and it's not conglomerate code going deep into dozens of files and layers, it's actually pretty straight-forward and all conveniently in the same file. Also, as I said before, I'm doing a lot of small UX improvements here, so just ripping out one or two hooks is unlikely to reduce the patch code much.

If we're allowing the user to minimize the bucket (which looks like a sensible thing to do, much better than allowing to collapse full bucket, which is error-prone, or force a random min-width, which is amateurish), most of the hooks must definitely stay because the minimized bucket isn't functional, so any way of touching the bucket must restore it to a reasonable size first.

I'm honestly quite failing to see how something which works perfectly and has been green-lighted by ui-review should be rejected by a pretty subjective notion of "I don't like this because it needs a bit of extra code". If anyone can show that exactly what I'm doing can be done with less code, I'll gladly accept that. Otherwise this is an implementation-level argument which shouldn't be used to reduce polished UX back to primitive UX. That's the end of all polishing, a blanket argument to shoot down any fine-tuned UI/UX improvements.

Comment 122

[:aceman]

Comment on attachment 8959999 [details]
Screenshot 1: without scrollbar trickery - a) scrollbar hiding icons, b) cut-off pane content and header

Yes, I understand this would happen if you didn't do the scrollbar calculations. That is why I proposed to set the min-width to some 5em and then you do not need to care if there is the vertical scrollbar or not. I'd think it does not matter if you see first 3... (with scrollbar) or 5... characters (without scrollbar) of the attachment name. If we agreed on this, probably much of the patch could be reduced.

Comment 123

[:aceman]

Attachment: screenshot 2: minimized bucket

But your patch does not do what you say, at least on Linux. See the screenshot. Not only the icons are shown, but also part of the highlight of the attachment name. Also there seems to be a dummy scrollbar (while unneded) without the moving part. Yes, that part seems to be a XUL bug, which could maybe be solved by collapsing the size column or something.
Anyway, as you can see some attachments do not even have icons (not caused by this patch). I simply do not see why this very minimized state would make any sense to the user. The icons alone do not even convey the info how many attachments there are (the count above does). Why are the beginning of the names not needed? This looks unbaked to me.
Even not operating the bucket and attaching new files via toolbar makes the bucket expand. So what information does the minimized bucket with just icon convey? That there are ANY attachments?

Comment 124

[:aceman]

(In reply to :aceman from comment #122)
> Yes, I understand this would happen if you didn't do the scrollbar
> calculations. That is why I proposed to set the min-width to some 5em and
> then you do not need to care if there is the vertical scrollbar or not. I'd
> think it does not matter if you see first 3... (with scrollbar) or 5...
> characters (without scrollbar) of the attachment name. If we agreed on this,
> probably much of the patch could be reduced.

OK, I see that when dragging the bucket to minimize it, the names reduce to '...' faster than the sizes. So we do not get the first 3 or 5 characters of the name. Now hiding the sizes when the bucket is under a certain size could be a useful feature.

Comment 125

[Richard Marti (:Paenglab)]

Comment on attachment 8960002 [details]
screenshot 2: minimized bucket

Yes, either show the icons only (with the scrollbar calculation) or a min-width with some text would be better. Linux and Windows Classic are a bit special with not highlighting the whole item.

The not showing a icon is like you wrote not the issue of this bug. I could fix this easily through CSS.

Comment 126

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #119)
> (In reply to Thomas D. from comment #106)
> > (In reply to :aceman from comment #105)
> > And honestly, as we have very little to offer in terms of ux improvements, I
> > think even a little bit of eye candy would be more than welcome.
> 
> Sure, but not with 10 hooks everywhere throughout the code to get some tiny
> effect.

Let's please keep it professional and not use suggestive common places without looking at the detail.
I haven't counted the hooks but suffice to say they are all doing different things, and if we want the feature of "minimized bucket state", you might be able to rip out one or two (at a high cost of looking unprofessional, see e.g. screenshot attachment #959999), but most hooks are needed for this feature to work conveniently and efficiently without forcing the user into repeated manual adjustments.

We want minimized bucket state for several reasons:
- ux-error-prevention - don't allow collapsing the full bucket
- widget space competition - we don't know how much space the user really has (and even wide-screen space is limited when there's more than one app on screen), so it's helpful to allow the user to show/hide/minimize UI elements as needed, here: to maximize the space for the address widget. Random min-widths for the attachment bucket are NOT helpful.
- ux-minimalism - when I'm done adding attachments, they are just visual clutter potentially distracting from the message. Why not tuck them away nicely for a smoother overall UI?

> > Please let's not get into the habit of dismissing any improvement just
> > because it needs a bit of code. If we want to strive for awesome UX, we
> > can't keep counting lines. But as I said, it's actually few lines for all
> > the improvements which I'm shipping.
> 
> A bit of code is fine. But it is always a tradeoff that we need to judge.
> You know we are mostly in maintenance mode and the amount of code we need to
> manage matters.

This isn't much code for the multitude of things which it does.
There's nothing to manage about this code once this has landed.

> > I think this has all been explained, discussed, tried and tested already
> > many times in this bug by me and others so I won't repeat all that and then
> > get blamed for lengthy comments... ;-)
> 
> Somehow I haven't found the explanation.

As to philosophy of minimized bucket, see above in this comment (reasons).
There are several ways to minimize/restore full bucket:
- click on bucket header
- Alt+M repeatedly
- drag bucket splitter
- touching minimized bucket in whichever way restores the bucket to make it actionable (focus, select, keypress etc.). I've tried to reduce the handling into something more generic, but due to the way events are fired, I couldn't find anything better. I've commented on that before with more details.

> > > Currenly with one recipient per line, there is a ton of free space for the
> > > bucket :)

Maybe not when recipient's name is "Richard von Weizäcker <richard.von.weizä[e-mail]>", and when you're sharing the screen between two applications side by side.

> > Pls consult with Jörg who requested this feature in bug 1427092 comment 18,
> > and Richard who also proposed it in this bug 1428631 comment 19... You've
> > been on board all along, isn't it a bit late now to start discarding the
> > idea?
> 
> Good, then I can let Jorg review this :)

Jörg is an excellent reviewer, why? ;)

> > Let's be aware that not everybody is running on big screens. Even with big
> > screen, I can shrink TB to share the screen with some other app and
> > immediately space becomes an issue. 
> 
> OK. But if you minimized the bucket and then click it again (or add an
> attachment), it expands back. So what is the space saving?

Minimized bucket isn't functional, so we must expand when touched.
The space saving allows you to maximize the address widget for those times where you're NOT working on the attachments, i.e. for when you're currently working on the address widget or prefer to see more details of that. Widgets should be conveniently sized when they are being worked on, isn't it? It's common practice to reduce the size of non-active widgets to allow working on active widgets. I believe there are entire operating systems like Android built around that idea. You can't edit attachments AND addresses at the same time, so we allow user to focus on the current widget between the two.

> > > ::: mail/components/compose/content/MsgComposeCommands.js
> > > @@ +4871,5 @@
> > > > +  document.addEventListener("mousemove", attachmentPaneSizerOnMouseMove, true);
> > > > +  document.addEventListener("mouseup", attachmentPaneSizerOnMouseUp, {capture: false, once: true});
> > > 
> > > Can't you set up the listeners once at startup?
> > 
> > Why waste resources to listen all the time when listening while splitter is
> > dragged is just enough?
> 
> But it would simplify the code :)

I doubt that, as this hook is actually pretty simple and straightforward already. I'm not an expert on hooks, but I didn't have any problem to understand the existing hooks for address widget.

> Also setting up and removing listeners
> takes some resources at a place where you do not want to cause more visual
> non-smoothness of dragging due to running JS.

I don't see any visual delays, because I've trimmed all of my hooks to do as little as possible under the circumstances. Btw, the existing address widgts hooks do a lot more and more expensive stuff than mine, and no one has ever complained about visual delays or anything. Maybe reviewer wants to make the hooks even more efficient, that's fine with me as long as it preserves the intended functionality.

> > > @@ +4878,5 @@
> > > > +function attachmentPaneSizerOnMouseMove()
> > > > +{
> > > > +  // Using attachmentbucket-sizer splitter can minimize the attachment bucket,
> > > > +  // and then we want to update the header of the bucket accordingly.
> > > > +  updateAttachmentPane();
> > > 
> > > Do you really need to do this for every move when user didn't release the
> > > mouse yet? Responsive design, I know ;)
> > 
> > Yes. So you know the answer.
> 
> Does not mean I agree with it. Calling it in attachmentPaneSizerOnMouseUp()
> would be fine with me.
> The only difference is that the 3 dots from 'attachment' vanish while the
> user has hit the minimum width, but still holding the mouse. In my version
> they would vanish after he releases the mouse.

I've tried and considered that and found it odd.
And it's more than just the 3 dots when the vertical scrollbar is there.
It's also a good visual hint (especially for empty bucket), that you've succeeded to minimize the bucket. Without that hint, user might collapse the empty bucket which is certainly not his intention (because just clicking the [x] would do that trick much more efficiently).

I'm totally failing to see how removing this hook would help anything. It's only going into action when user drags the splitter, otherwise it's not active so not eating any memory or processing ressources at all.
The very fact that this requires a few lines of code can't be an argument against responsive design. That's what makes users have a good feeling of a professional application.

> If there were some 100px region where the labels would change already, but
> the user could drag further, there would be a sense. But when the change
> happens only when the bucket is at the minimum size, calling the function
> seems unneeded.

Ah well, dito, we're not loosing anything but it just feels more responsive - removing strings after I release the mouse is kinda odd and surprising, almost like a bug. Dragging triggers the header change, and we should show that immediately.

> > > @@ +4927,5 @@
> > > > +  // words). Empty bucket can't have scrollbars so exact scrollbar width doesn't
> > > > +  // matter, but we need to give the splitter-dragging user a landing range for
> > > > +  // getting the mini header as empty bucket collapses when dragging beyond
> > > > +  // gBucketMinWidthDefault.
> > > > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> > > 
> > > This would be more readable with some brackets added. count && x || !count?
> > > I could guess '||' has lower priority than '&&', but still...
> > 
> > Well, I think Jörg was telling me to skip any superfluous brackets the other
> > day...
> > It's not about guessing, precedence rules are public and clear and certainly
> > won't ever change.
> 
> Yeah, but when we need to think in several different languages...

I'd love to hear from Jörg what he thinks about adding unneeded brackets.

> > But yeah, we could add some brackets here...
> 
> Please do.

I already did in the latest patch which was available at the time of your review.

> > I also doubt anyone is going to read this code again once it's there up and
> > running...
> 
> This isn't the best attitude ;)

Yeah, but my code isn't bad, it's only you don't want any extra code for fine-tuning things.
Otherwise I think it's a fact that no one is likely to ever look into these particular spots of code again until we re-write TB from scratch.

> > > @@ +4928,5 @@
> > > > +  // matter, but we need to give the splitter-dragging user a landing range for
> > > > +  // getting the mini header as empty bucket collapses when dragging beyond
> > > > +  // gBucketMinWidthDefault.
> > > > +  if (count && bucketWidth > parseInt(bucket.minWidth) ||
> > > > +      !count && bucketWidth > gBucketMinWidthDefault + 17 ||
> > > 
> > > What is this magic value of 17 and how did you arrive at it? I surely do not
> > > like it.
> > 
> > 17 happens to be the default width of a vertical toolbar,
> 
> I think Paenglab would know better whether this is safe to assume. There can
> be different themes or spaced up UI (hi DPI) where the size would be
> different.
> You must get the value from the widget, not hard-code it here. 

No, you still don't understand what I'm saying.
I'm saying that the actual scrollbar size is totally irrelevant because this is purely for creating a landing zone for dragging the splitter where responsive design tells the user that "now your empty bucket is minimized, stop dragging here or else it will disappear". And that's what it does if you drag a bit further to the right. 

> > > @@ +5354,5 @@
> > > > +  // (next step: hide), but "not yet fully visible" for headerClick and shortcut
> > > > +  // key operation (next step: show).
> > > > +
> > > > +  switch (true) {
> > > > +    case !!aAction:
> > > 
> > > More syntax experiments? :) You would get a fat warning/error from a proper
> > > C++ compiler that the case values are not in the range of the switch
> > > variable, or that you do not cover all the possible values with cases (e.g.
> > > in case of an enum).
> > > It seems you are just misusing switch instead of 'if () elseif' cascades :)
> > 
> > This is correct javascript, working well, reliable, concise and easy to
> > read. I don't and I can't care about C++ compilers when I'm writing
> > javascript.
> 
> The Javascript compiler is getting stricter to allow it to get faster.

Even the strictest JavaScript compiler cannot refuse "switch(true)" with "case (boolean expression):"

> > No misuse here, just intelligent and more readable notation.
> > Try to do without - given the lack of labelled goto statements in
> > javascript, this would require really odd workarounds to skip the entire
> > switch. And nested if/else are harder to write, harder to read, and not
> > helping anyone.
> 
> I see no advantage of using this hack instead of proper else if branches. No
> gotos or workarounds needed.

You are right that no gotos or workarounds are needed to remove the "case (!!aAction):" which for some reason you don't like. The big advantage of using switch() here is that makes MUCH more readable code, as it avoids all the bracketed clutter of if structures, and - more importantly - allows easy visual distinction of the main cases VS. sub-conditionals inside the main cases. Iow, we're avoiding multi-level nested if-structures, hard to write, hard to read, easy to get wrong if you just forget a single "}" somewhere, and replace it with a minimalist list of cases which focus on the actual conditions rather than the bells and whistles of if-syntax. I totally love switch() because it streamlines unreadable nested if's into a clear main structure, then each case may have it's sub-conditionals as needed.
This is NOT a hack, it's a perfectly legitimate smart use of what the language offers, evaluating the boolean in "switch(true)" against boolean expressions in "case (boolean expression)".

> > So I don't see the point of criticizing this.
> > That said, would you prefer the following?
> > 
> > if (!aAction) {
> >   switch (true) {
> >   ...
> >   ...
> >   ...
> >   }
> > }
> 
> NO, I want to kill the switch.

Do whatever you please (according to your personal taste), but I myself am not going to re-bloat my streamlined, very readable switch() code with tons of nested ifs, that's absurd.

> And if you really want to remove the 'if (!aAction) { }' indentation level,
> you can do:
> if (aAction) {
>  // do nothing

Yeah, that removes the indentation level but it's also ugly and less readable.
I actually like the outer "if (!aAction)" and the extra indentation level inspired by your criticism of "case (!!aAction):", as it describes clearly when we get into the switch or not.

> } else if (bucketWidth == parseFloat(bucket.minWidth)) {
> ...
> } else if { ... }
> 
> > > Or is this particularly tuned to only ever shown the icon, so you need to
> > > tune the size to scrollerSize (if any) + 16px for icon? But I do not see any
> > > such constant here (of course you would be getting icon size from the css).
> > 
> > Yes, it's finetuned to only ever show the icons, not more, not less.
> > I'm using 2 em which is icon size with needed margins, but if there's a
> > better way to set the minWidth, pls advise.
> > Then I'm calculating the scrollbar width and auto-balancing that out when it
> > appears or disappears.
> > I've tried very hard to find an easier way of getting the scrollbar width
> > (see outcommented code), but failed radically. Those anonymous elements are
> > very hard to access, even using their special methods.
> 
> Why would it be useful to only show the icon?

Maximize space for editing address widget, and more reasons, see above.

> Notice some attachments even have no icon.

On windows, every file has some icon, at least a generic one. Not on other OS?

> Even then, your 2em do not work for what you want. It shows the icon and
> also some pixels of the highlight of the selected item.

Not on my Windows 10. Could you please attach a screenshot?

> > > @@ +5809,5 @@
> > > > +  let defaultWidth = parseFloat(defaultWidthCSS);
> > > > +  let defaultFontSize;
> > > > +  if (defaultWidthCSS.toLowerCase().includes("em")) {
> > > > +    defaultFontSize = parseFloat(getComputedStyle(bucket).fontSize);
> > > > +    defaultWidth = defaultFontSize * defaultWidth;
> > > 
> > > What? Doesn't the style engine give you the px value directly with some call?
> > 
> > I couldn't find a better way - can you?
> 
> Why not GetMsgAttachmentElement().width or similar?

bucket.width doesn't work because that's the actual width of the bucket, which iir is affected by the persisted width of the attachments-box, which can be changed by user. Or else bucket.width was not available or something like that. What we want here is the *default* width of the bucket, as set in the CSS value. We need this now because we allow dragging the bucket widths which are smaller than the reasonable/actionable default, e.g. minimized to icon-size.

> > > @@ +5881,5 @@
> > > >    let bucketSizer = document.getElementById("attachmentbucket-sizer");
> > > >    if (attachmentBucket.itemCount > 0) {
> > > >      attachmentBucket.removeAttribute("empty");
> > > > +    // Restore default minWidth which we adjusted below if still applied.
> > > > +    if (attachmentBucket.minWidth == gBucketMinWidthDefault - 4) {
> > > 
> > > Magic values again?
> > 
> > Same purpose as above, an arbitrary fuzzy landing zone for dragging the
> > splitter so that the empty bucket does not disappear suddenly before it's
> > correctly minimized. Otherwise it's hard for humans to drag this
> > pixel-perfect.
> 
> I do not understand. When would the bucket disappear? The patch enforces the
> 2em minimum size. Am I missing something?

Yes. We only enforce 2em min-width for the full bucket. I made it so that user can make the empty bucket disappear by dragging it to zero width. Empty bucket still has 2em min-width, but I change the splitter to collapse="after", i.e. dragging beyond min-width will make empty bucket disappear. Hence the need for a landing zone for the empty-minimized state so that dragging to minimized becomes possible, because humans will find it hard to drag pixel-perfect.
I don't see any alternative to that because the close-button is already disappeared when empty bucket is minimized, so enforcing 2em for empty bucket would mean there's no visual way of closing it when minimized.

> > > ::: mail/themes/windows/mail/compose/messengercompose.css
> > > @@ +12,5 @@
> > > >  @namespace html url("http://www.w3.org/1999/xhtml");
> > > >  
> > > > +#attachments-header-box {
> > > > +  overflow:hidden;
> > > > +  min-height: 27px;
> > > 
> > > Why 27px?
> > 
> > That's what makes it look in line with the visible line below the sender,
> > and same height as contacts side bar header.
> 
> OK, then add a comment referencing the 27px value at the other element and
> saying they must be the same. Otherwise theme authors may mess it up.

OK, will try that.

Comment 127

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #122)
> Comment on attachment 8959999 [details]
> Screenshot 1: without scrollbar trickery - a) scrollbar hiding icons, b)
> cut-off pane content and header
> 
> Yes, I understand this would happen if you didn't do the scrollbar
> calculations. That is why I proposed to set the min-width to some 5em and
> then you do not need to care if there is the vertical scrollbar or not. I'd
> think it does not matter if you see first 3... (with scrollbar) or 5...
> characters (without scrollbar) of the attachment name. If we agreed on this,
> probably much of the patch could be reduced.

The charming part about minimized bucket is NOT to have any cut-off attachment names at all, because that just looks ugly. With any min-width other than icon-width you're basically changing nothing about the current UX, except that you'll allow the user to get into more ugly and useless UI states.
Reducing patch size isn't an end in itself, I believe we should aim for a smooth, useful, and professional UX.

Comment 128

[Jorg K (CEST = GMT+2)]

I've come to try this patch, and I must say, it has its charm. At least on Windows, only the icons show, with or without scrollbar. Perhaps we can fix the Linux issues that Aceman raised.

There's one thing I didn't like: When the bucket is showing but is not focused, the first Alt+M focusses it. I think it should minimise it straight away.

I've been discussing this with Aceman on IRC since we really want to settle this. How about a simple minimum width and below that, either by shifting the slider or using Alt+M, we remove the bucked but leave the header. The number on its own with a row of icons is OK, but equally, "5 attachments   572 KB" with nothing below would be enough of an indication.

Comment 129

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #123)
> Created attachment 8960002 [details]
> screenshot 2: minimized bucket
> 
> But your patch does not do what you say, at least on Linux. See the
> screenshot. Not only the icons are shown, but also part of the highlight of
> the attachment name. Also there seems to be a dummy scrollbar (while
> unneded) without the moving part. Yes, that part seems to be a XUL bug,
> which could maybe be solved by collapsing the size column or something.

Yeah that's an oddness caused by XUL.
Reason being (I think) that the horizontal scrollbar doesn't have enough vertical space, so can't be shown, so verticall scrollbar shows because not all vertical content is shown, but then it should either be enabled, or not shown. Something like that...

> Anyway, as you can see some attachments do not even have icons (not caused
> by this patch). I simply do not see why this very minimized state would make
> any sense to the user. The icons alone do not even convey the info how many
> attachments there are (the count above does).

Huh? If you have few attachments or drag your horizontal splitter down far enough, all icons will be visible, so that does convey the number of attachments. As with any minimized widget, the point is to maximize the space for other widgets (address widget) and reduce visual clutter. Icon-width bucket with icons only is the perfect way of conveying that:
- there are attachments (ux-error-prevention - with collapsed full bucket user might think that there's nothing attached)
- the number and type of attachments
That's enough for ux-error-prevention, it may be enough for a rough check, and to see more details, just expand the bucket again which is one click on the header, or Alt+M, or dragging the splitter open.

> Why are the beginning of the
> names not needed? This looks unbaked to me.

They are not needed because the whole point of dragging the bucket smaller than default size is to get it out of the way and have more space for editing/viewing information of address widget. But in the current UI dispensation, we can't allow the bucket content to totally disappear, because of UX-error-prevention.
If we totally change the XUL box layout, we could have other ways of collapsing the bucket which leave a visual trace in the UI ("X attachments"), and I've commented on that, but with the current UI, that's not easily possible. Re-arranging the box model is at least as non-trivial as what we're doing here, and will require other tweaks again.

> Even not operating the bucket and attaching new files via toolbar makes the
> bucket expand. So what information does the minimized bucket with just icon
> convey? That there are ANY attachments?

YES, that's the most important point. But you can also see the type of attachments, and their number.

Comment 130

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #128)
> I've come to try this patch, and I must say, it has its charm.

Thanks. :)

> At least on
> Windows, only the icons show, with or without scrollbar. Perhaps we can fix
> the Linux issues that Aceman raised.

> There's one thing I didn't like: When the bucket is showing but is not
> focused, the first Alt+M focusses it. I think it should minimise it straight
> away.

It really sucks that I'm just talking to myself and no-one seems to ever read what I'm saying, even at that point where it's still short and concise, fresh, non-repeated information.

I already pointed out that we don't have a choice for that behaviour because Alt+M is advertised as an access key for attachment pane (underlined character 'm' in '5 Attach_m_ents'), which by definition must focus the pane first. With focus in body, we have no way of guessing whether user wants to focus the bucket to work on it (add/remove/rename/verify attachments) or to minimize it; but given the presence of the access key, user cannot expect immediate minimizing.

Furthermore, also from a workflow perspective, immediate minimizing from body does not make much sense:
- If user really prefers minimized attachment bucket, the best time to minimize is certainly right after working on the bucket. Which works immediately with Alt+M as long as focus is still on bucket, and conveniently shifts focus to body after minimizing the bucket. So then, Alt+M with focus on body and minimized bucket will restore the bucket.
So it would also be unexpected/unpredictable/inconsistent when Alt+M, with focus on body, would sometimes restore and sometimes hide the bucket.
The current patch behaviour is predictable and reliable not only in honoring the access key (which we must), but also in that whenever focus is outside bucket, the first Alt+M will always focus and restore the bucket, and the next Alt+M will always minimize full bucket or hide empty bucket.
Also in terms of probability, I think it's much more likely that user wants to focus and work on the bucket than to minimize it whilst focus is somewhere outside bucket.

> I've been discussing this with Aceman on IRC since we really want to settle
> this. How about a simple minimum width and below that, either by shifting
> the slider or using Alt+M, we remove the bucked but leave the header. The
> number on its own with a row of icons is OK, but equally, "5 attachments  
> 572 KB" with nothing below would be enough of an indication.

Yes, I have already presented that option as probably the best ultimate UI for this in several comments on several bugs. Unfortunately, I don't see anyone who'll step up to code this atm. This involves breaking up the entire existing box structure of the address widget, and adding more trickery to get things right. Trust me that's entirely non-trivial and least as complex as what we're doing here. Personally I don't have the time nor the desire to tackle that task atm and start all over again. So the current patch looks like a great improvement of the status quo to me, and I've actually come to like the charm of minimized bucket with icons-only. It's a valid alternative UI to the completely collapsed bucket with just the caption as an UI trace; maybe icons are a better trace in our current UI where attachment bucket has vertical orientation.
Most importantly, we eliminate the error-potential of collapsing full bucket, which is how we got here.

Comment 131

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #130)
> (In reply to Jorg K (GMT+1) from comment #128)
> ...
> I already pointed out that we don't have a choice for that behaviour because
> Alt+M is advertised as an access key for attachment pane (underlined
> ...
> Also in terms of probability, I think it's much more likely that user wants
> to focus and work on the bucket than to minimize it whilst focus is
> somewhere outside bucket.

Even more so for the empty bucket: there's absolutely no reason to keep an empty (large/small/minimized) bucket unless if you're planning to add more attachments (otherwise why wouldn't you just close it?). So with focus in body, and empty, visible bucket, we have all reason to assume that user wants to focus and work on the bucket (and I've mentioned that before...).
Then again it would be confusing and inconsistent if we'd minimize the full bucket, but restore the empty bucket when focus is in body.
Current patch behaviour is consistent and predictable in that the first Alt+M with focus outside bucket will always focus and restore the bucket, no matter what state it is in, empty/full, large, small, minimized, or even hidden.

Comment 132

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #124)
> OK, I see that when dragging the bucket to minimize it, the names reduce to
> '...' faster than the sizes. So we do not get the first 3 or 5 characters of
> the name. Now hiding the sizes when the bucket is under a certain size could
> be a useful feature.

Yes, I had already discussed this with Richard but I'm not going to fix that here unless someone offers the exact code.

Comment 133

[Jorg K (CEST = GMT+2)]

Attachment: mock-up.png

Late night, 1:50 AM, Aceman and I discussing this :-)

How about a very simple collapsed bucket?
- Minimum size
- If you get to this size, show attach.svg as background, hide the icons
- change the header to just show the number
- no scrollbars

Doable in CSS.

Mock-up done in TB with Developer Tools.

Comment 134

[:aceman]

Comment on attachment 8961989 [details]
mock-up.png

Thanks for the mockup. The point would be that this display would turn on at some fixed minimum size (e.g 2ch or some px), doing away with the current calculation of icon_size+scrollbar_size which seems to be fragile (depends on theme and OS and XUL scrollbar bugs). This could simplify the patch.

Comment 135

[Richard Marti (:Paenglab)]

Comment on attachment 8961989 [details]
mock-up.png

Like it. With this we could get rid of the scrollbars with too many attachments.

Comment 136

[Jorg K (CEST = GMT+2)]

The motivation for this idea was this: It's been suggested to just remove the attachment bucket and leave the header, but that's hard to do since they both share the same vbox. Hiding the bucket just leaves empty space below the header. So I had the idea to draw something useful into that empty space that lets the user know that there are attachments.

Comment 137

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #134)
> Comment on attachment 8961989 [details]
> mock-up.png
> 
> Thanks for the mockup. The point would be that this display would turn on at
> some fixed minimum size (e.g 2ch or some px), doing away with the current
> calculation of icon_size+scrollbar_size which seems to be fragile (depends
> on theme and OS and XUL scrollbar bugs).

I don't see anything fragile about the calculation, and it does definitely NOT depend on theme and OS in the sense of making it fragile. The opposite is true. The calculation will get it right independent of theme and OS, which makes it stable.
Also the calculation does not "depend" on scrollbar bugs. Even when those bugs go away, it will still work correctly (but could be trimmed a bit).

> This could simplify the patch.

I doubt that. The handful of lines which you can remove will be added elsewhere.

Comment 138

[Thomas D. (:thomas8)]

Comment on attachment 8961989 [details]
mock-up.png

Interesting idea - charming eyecatcher!

But I'm quite puzzled that our UX design is now driven and mutilated by implementation-level considerations.

Apart from being a wonderful oversize eye catcher - which may very well become annoying and distracting over time - what are we winning?

What I see is a significant loss of information and versatility compared to my patch.
a) icons are a much more straightforward representation of attachments present
b) icons are visually countable
c) the paper clip is ambiguous as we're also using it for "Attach" button, so from the icon alone it's not clear at all if there are attachments or not, as it could also be an oversize button to add attachments from empty bucket. The only real indicator of attachments, the "4" number label above, is quite small and becomes even less salient compared to the oversize icon
d) icons carry helpful information on the type of attachment, which may very well be enough to know what's in the bucket (1 Excel file - oh, the quarterly performance table; 1 Word document - yes, the covering letter with more information...)
e) with icons, user can hover over minimized bucket and check the file name/path of every attachment. Now it's more complicated as he needs to expand the bucket first.
f) minimizing the bucket with splitter to iconsize seems like an organic and predictable UI behaviour.
g) the point of minimizing the bucket is to reduce its salience - are we still achieving that goal with an oversize paper clip? I can't tell - maybe yes (as it's more calm), or maybe no (as it's oversize and more screaming).

Having said that, I'll play with this a bit more to see if I'll find it "cool" or possibly "useless", "irritating" and "obfuscating"...

Comment 139

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere) from comment #130)
> (In reply to Jorg K (GMT+1) from comment #128)
> > How about a simple minimum width and below that, either by shifting
> > the slider or using Alt+M, we remove the bucked but leave the header.

> Yes, I have already presented that option as probably the best ultimate UI
> for this in several comments on several bugs. Unfortunately, I don't see
> anyone who'll step up to code this atm. This involves breaking up the entire
> existing box structure of the address widget, ...

Yes, I looked at it and you are correct. The solution of just showing the header, which you have presented elsewhere also has the downside of not having icons, so at least b) d) e) would be disadvantages of that solution as well.

Since the addressing widget is rather wide, so I don't see many cases were people would want to close a non-empty bucket. We're just here to stop them closing the bucket completely and sending out something without being fully aware. So one click more to get back to the details and a full width bucket without having to hover and read seems acceptable.

Comment 140

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #139)
> (In reply to Thomas D. (currently busy elsewhere) from comment #130)
> > (In reply to Jorg K (GMT+1) from comment #128)
> > > How about a simple minimum width and below that, either by shifting
> > > the slider or using Alt+M, we remove the bucked but leave the header.
> 
> > Yes, I have already presented that option as probably the best ultimate UI
> > for this in several comments on several bugs. Unfortunately, I don't see
> > anyone who'll step up to code this atm. This involves breaking up the entire
> > existing box structure of the address widget, ...
> 
> Yes, I looked at it and you are correct. The solution of just showing the
> header, which you have presented elsewhere also has the downside of not
> having icons, so at least b) d) e) would be disadvantages of that solution
> as well.

True.
 
> Since the addressing widget is rather wide, so I don't see many cases were
> people would want to close a non-empty bucket. We're just here to stop them
> closing the bucket completely and sending out something without being fully
> aware.

Agreed

> So one click more to get back to the details and a full width bucket
> without having to hover and read seems acceptable.

Yeah. Good luck to make that click happen, as I explained before, it's non-trivial because before you can catch the click, focus event will already have expanded the attachment pane, then the click event fails to see that the bucket was minimized, hence you'll get attach dialog instead of expanding. So you'll have to change the logics somehow which needs more thought and/or more trickery.

We could also try a variant of your minimized bucket where we hide the header bar and have the number of attachments under the paper click with a rounded light-grey background:

 

Comment 141

[Richard Marti (:Paenglab)]

Attachment: 1428631-placeholder.patch

This patch enables minimizing the bucket with attachments in it. It then shows a placeholder with a paper clip. Clicking on it (or ALT+m or through the menu) opens the bucket again.

Comment 142

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969909 [details] [diff] [review]
1428631-placeholder.patch

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

Excellent! Thanks for picking this up and bringing it to a close. I can fix the nits when landing, no new patch required.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +686,5 @@
>          } else {
>            cmdToggleAttachmentPane.setAttribute("checked", "true");
>          }
>  
>          // Enable this command when the compose window isn't locked;

Nit. Trailing semicolon.

@@ +5208,4 @@
>        aAction = "hide"
>      } else {
>        // Menu click with hidden bucket (empty or full)
>        // or cmd_toggleAttachmentPane via shortcut key;

and here.

Comment 143

[Richard Marti (:Paenglab)]

Thanks. Is the leave-open keyword still needed?

Comment 144

[:aceman]

Comment on attachment 8969909 [details] [diff] [review]
1428631-placeholder.patch

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

Thanks, nice short patch :) Does it replace all the features Thomas had in his large patch?

When minimizing the bucket with the splitter, there remains a vertical gray line delimiting it from the address rows.
When you toggle it via Alt+M, the line is not there. That would need some synchronizing.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +687,5 @@
>            cmdToggleAttachmentPane.setAttribute("checked", "true");
>          }
>  
>          // Enable this command when the compose window isn't locked;
> +        return (!gWindowLocked)

Brackets unneeded and a semicolon missing.

Comment 145

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #143)
> Thanks. Is the leave-open keyword still needed?

I would leave it for Thomas to make a patch on top of yours to add the possible code improvements he intended (now that the visual part was simplified).

Comment 146

[:aceman]

Comment on attachment 8959677 [details] [diff] [review]
Part 2 - Patch V.3.2: Implement view state machinery incl. minimize/restore (rebased, nitfixes, removed console.logs and test code)

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

Please see attachment 8969909 [details] [diff] [review] that reworked the visual effect. Please reduce this patch to your intended code improvements.

Comment 147

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #144)
> Comment on attachment 8969909 [details] [diff] [review]
> 1428631-placeholder.patch
> 
> Review of attachment 8969909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, nice short patch :) Does it replace all the features Thomas had in
> his large patch?
> 
> When minimizing the bucket with the splitter, there remains a vertical gray
> line delimiting it from the address rows.
> When you toggle it via Alt+M, the line is not there. That would need some
> synchronizing.

I've seen this too. With ALT+m it sets collapsed="true" to the splitter. When collapsing through the splitter state="collapsed" is used. Should this been synced to splitter default state="collapsed"?

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +687,5 @@
> >            cmdToggleAttachmentPane.setAttribute("checked", "true");
> >          }
> >  
> >          // Enable this command when the compose window isn't locked;
> > +        return (!gWindowLocked)
> 
> Brackets unneeded and a semicolon missing.

Jörg, can you do this when checking-in (when no new patch is needed for the splitter collapsed issue)?

Comment 148

[Jorg K (CEST = GMT+2)]

No "leave-open" needed any more, the next push will close the bug. I can also fix the return. Does this need another patch? I see difference in the two methods of collapsing on Windows.
  attachmentBucketSizer.collapsed = false;
is pre-existing to the patch, but we could try attachmentBucketSizer.state = "collapsed";
I can't try it since I don't see the difference.

Comment 149

[Richard Marti (:Paenglab)]

Attachment: 1428631-placeholder.patch with fixed comments and differen collapse mechanism

Fixed the review comments and changed the collapse mechanism.

I don't obsolete the old patch in case my new patch is rubbish (also when it works ;-) ).

Comment 150

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969919 [details] [diff] [review]
1428631-placeholder.patch with fixed comments and differen collapse mechanism

I've tried this as well. The proper state is "open", not "", see:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter

Using "open" or "" destroys the ability to move the splitter, right?

Perhaps Aceman can see whether he can get the line he complained about to disappear.

Comment 151

[Richard Marti (:Paenglab)]

Attachment: 1428631-placeholder.patch with fixed comments and different collapse mechanism [LANDED in comment 158]

(In reply to Jorg K (GMT+1) from comment #150)
> Comment on attachment 8969919 [details] [diff] [review]
> 1428631-placeholder.patch with fixed comments and differen collapse mechanism
> 
> I've tried this as well. The proper state is "open", not "", see:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter

When dragging the splitter the system sets "" not "open".

> Using "open" or "" destroys the ability to move the splitter, right?

My patch works for this.

> Perhaps Aceman can see whether he can get the line he complained about to
> disappear.

The line is the splitter and now constantly shown.

Comment 152

[:aceman]

Comment on attachment 8969919 [details] [diff] [review]
1428631-placeholder.patch with fixed comments and differen collapse mechanism

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

Yes, this fixes the line for me (it is there in both cases).

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5216,5 @@
>    switch (aAction) {
>      case "show":
>        attachmentsBox.collapsed = false;
>        attachmentBucketSizer.collapsed = false;
> +      attachmentBucketSizer.state = "";

Please use setAttribute here too, and use the value of "open".
The state is not a property: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter

Comment 153

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #151)
> > I've tried this as well. The proper state is "open", not "", see:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter
> 
> When dragging the splitter the system sets "" not "open".

When dragging the value should be "dragging". Are the docs not true again?

Comment 154

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #152)
> Comment on attachment 8969919 [details] [diff] [review]
> 1428631-placeholder.patch with fixed comments and differen collapse mechanism
> 
> Review of attachment 8969919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this fixes the line for me (it is there in both cases).
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +5216,5 @@
> >    switch (aAction) {
> >      case "show":
> >        attachmentsBox.collapsed = false;
> >        attachmentBucketSizer.collapsed = false;
> > +      attachmentBucketSizer.state = "";
> 
> Please use setAttribute here too, and use the value of "open".
> The state is not a property:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter

Fixed in my last patch.

(In reply to :aceman from comment #153)
> (In reply to Richard Marti (:Paenglab) from comment #151)
> > > I've tried this as well. The proper state is "open", not "", see:
> > > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter
> > 
> > When dragging the splitter the system sets "" not "open".
> 
> When dragging the value should be "dragging". Are the docs not true again?

Correct. I meant, after dragging completely to the right the state is "".

Comment 155

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969921 [details] [diff] [review]
1428631-placeholder.patch with fixed comments and different collapse mechanism [LANDED in comment 158]

Yes, this works for me. Can we settle for "" or "open"? How can I see the value, I tried in the inspector and didn't see it.

> I meant, after dragging completely to the right the state is "".
You mean "left", so it is completely open?

Comment 156

[Richard Marti (:Paenglab)]

Attachment: splitter-state.png

(In reply to Jorg K (GMT+1) from comment #155)
> Comment on attachment 8969921 [details] [diff] [review]
> 1428631-placeholder.patch with fixed comments and different collapse
> mechanism
> 
> Yes, this works for me. Can we settle for "" or "open"? How can I see the
> value, I tried in the inspector and didn't see it.

This is a screenshot of the devtools inspector with the state=""

> > I meant, after dragging completely to the right the state is "".
> You mean "left", so it is completely open?

Yes, left. It doesn't need to be completely open. Not collapsed is enough.

Comment 157

[Jorg K (CEST = GMT+2)]

Right, I watched in the inspector now, I saw "dragging", "collapsed" and "". Setting it to open works, but the next drag resets to "". Aceman found the code:
https://dxr.mozilla.org/comm-central/rev/5ded36cb383d3ccafd9b6c231c5120dcdae196a2/mozilla/layout/xul/nsSplitterFrame.cpp#409

Let's go with "".

Comment 158

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd2f985278a6
Show a placeholder when the attachment bucket with attachments is collapsed. r=jorgk,aceman

Comment 159

[Jorg K (CEST = GMT+2)]

Thomas, please file a new bug for any follow-up work. This bug has become far too confusing at comment 159 :-(

We need this function now to uplift since TB 60 ESR is coming in a few weeks.

Comment 160

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 5):
https://hg.mozilla.org/releases/comm-beta/rev/8e864de65e00f4ecf1d213b5cccc091aeff3b051

Comment 161

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #145)
> (In reply to Richard Marti (:Paenglab) from comment #143)
> > Thanks. Is the leave-open keyword still needed?
> 

Thanks Richard for picking this up and realizing a minimal variant of the new style which apparently was preferred.

> I would leave it for Thomas to make a patch on top of yours to add the
> possible code improvements he intended (now that the visual part was
> simplified).

I bet we've lost a number of my improvements and intentions with the new minimal patch but from the lack of specific responses I'm under the impression that maybe my improvements and intentions aren't very relevant to others.

Unfortunately I'm preoccupied with non-TB matters atm so I don't have time to look into TB bugs.

If someone could tell me where to find the latest TB daily downloads (given that https://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/ is stuck on 21-03-18, and daily doesn't update itself), that would be appreciated.

Comment 162

[Jorg K (CEST = GMT+2)]

I've already explained this in a PM and on tb-planning. I've just refreshed that post:
https://mail.mozilla.org/pipermail/tb-planning/2018-April/006010.html
In short, get it from https://treeherder.mozilla.org/#/jobs?repo=comm-central by clicking on an "N".

Comment 163

[Thomas D. (:thomas8)]

Comment on attachment 8959677 [details] [diff] [review]
Part 2 - Patch V.3.2: Implement view state machinery incl. minimize/restore (rebased, nitfixes, removed console.logs and test code)

(In reply to :aceman from comment #145)
> I would leave it for Thomas to make a patch on top of yours to add the
> possible code improvements he intended (now that the visual part was
> simplified).

Thank you. I have landed the code improvements of this patch and a number of other goodies in followup bug 1461170.

This patch realized an icons-only minimized bucket, unfortunately requiring a significant amount of tweaks. It was obsoleted by the new visual approach of attachment 8969921 [details] [diff] [review], namely showing a big paperclip icon as a placeholder for the minimized bucket, elegantly eliminating the need for most tweaks.