1427037 - Always enable 'Reorder Attachments' command

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

As long as there's more than 1 attachment, there's no reason to disable 'Reorder Attachments' command just because attachment panel doesn't have focus yet at that moment. We can ensure focus on bucket when showing the panel.

Requirements:
- for enabling the command: ensure number of attachments > 1
- for executing the command:
  - programmatically unhide attachment pane if hidden
  - programmatically focus attachment bucket

Comment 1

[Thomas D. (:thomas8)]

I have fixed this in the current patch for bug 1426344, review pending.

Comment 2

[Thomas D. (:thomas8)]

FTR: this prevents us from having alternative keyboard shortcut Alt+X on MAC for cmd_reorderAttachments, which creates special characters if used from message body, so it works only with focus in attachment bucket. But Ctrl+X on MAC works from body and bucket without side effects, and easy access (this bug) weighs more than cross-OS consistency in this case. So it's all good.

Comment 3

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Enable cmd_reorderAttachents regardless of focus and attachment pane visibility, polish UpdateAttachmentBucket()

So let's do it here.

Changes implemented by this patch:

1) Move cmd_reorderAttachments into defaultController
2) New function: toggleAttachmentPane(aShow)
3) toggleAttachmentPane(true) in showReorderAttachmentsPanel() so that we ensure visibility of attachment panel for reordering, i.e. cmd_reorderAttachments works even when user has collapsed the panel with existing attachments.

4) Overhaul UpdateAttachmentBucket(aShowPane)
- better description block
- var -> let
- update count and total size even when the pane is going to be hidden (avoid incorrect UI states, and allow unhiding the panel without updating).
- attachmentBucket is only the listBox, it's really about Attachment Pane
- if someone can check if this function is used by any addons, I'd like to rename this to 
> updateAttachmentPane(aShowPane)

Comment 4

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #3)
> 4) Overhaul UpdateAttachmentBucket(aShowPane)
> - if someone can check if this function is used by any addons, I'd like to
> rename this to 
> > updateAttachmentPane(aShowPane)

or even
> updateAndToggleAttachmentPane(aShowPane)

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939010 [details] [diff] [review]
Patch V.1: Enable cmd_reorderAttachents regardless of focus and attachment pane visibility, polish UpdateAttachmentBucket()

Thanks for moving this code out of bug 1426344.

Nice idea to have the (very useful) "Reorder Attachments" (Alt+X) always available. Sadly, this patch doesn't work 100%.

When I position into the e-mail body and use the menu or press Alt+X, the reorder menu appears and all items are greyed out. So I can't use the menu. However, one attachment appears to be selected and highlighted in blue if it was selected before I moved the cursor to the e-mail body. So the user needs to click the apparently selected item again which seems to be quite confusing, wouldn't you agree?

Comment 6

[Jorg K (CEST = GMT+2)]

BTW, no problem doing |hg qref| on this patch. Would you mind fixing the indentation in showReorderAttachmentsPanel().

Comment 7

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Enable cmd_reorderAttachents "always", polish UpdateAttachmentBucket(), implement showPopupById() (incl. nitfixes)

(In reply to Jorg K (GMT+1) from comment #5)
> Comment on attachment 8939010 [details] [diff] [review]
> Nice idea to have the (very useful) "Reorder Attachments" (Alt+X) always
> available.

Thanks :))

> When I position into the e-mail body and use the menu or press Alt+X, the
> reorder menu appears and all items are greyed out. So I can't use the menu.

That's sad! :/

> However, one attachment appears to be selected and highlighted in blue if it
> was selected before I moved the cursor to the e-mail body. So the user needs
> to click the apparently selected item again which seems to be quite
> confusing, wouldn't you agree?

I would agree that this is wrong and bad UX... good catch!
Fixed. Explanations added in code, because these focus issues are quite delicate, hard to understand, but easy to break. Sometimes I'm surprised that everything is working so well...

cmd_sortAttachmentsToggle is now always enabled (with or without prior selection), and focus in attachment bucket for changing selection and/or reordering via keyboard, that's sufficient.

While I was at it...
I'm on record for not liking bulky document.getElementById(someID) calls. So when I saw this, I couldn't help cleaning up:

>    document.getElementById("reorderAttachmentsPanel")
>            .openPopup(document.getElementById("attachmentBucket"),
>                       "after_start", 15, 0, true);
So I implemented a nice generic showPopupById function which simplifies this type of call in a number of spots:

> showPopupById("reorderAttachmentsPanel", "attachmentBucket", "after_start", 15);

Also here:

>      <menupopup onpopupshowing="event.preventDefault();
>                                 showPopupById('languageMenuList',
>                                               'spellingButton');"/>

Conveniently, I also need the same function for my next patch where I make the attachment pane header interactive :)

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939040 [details] [diff] [review]
Patch V.1: Enable cmd_reorderAttachents "always", polish UpdateAttachmentBucket(), implement showPopupById() (incl. nitfixes)

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

This looks good and works well. r+ with a few nits/questions fixed/considered/answered.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3880,5 @@
> + * @param aPopupID   the ID of the popup element to be shown
> + * @param aAnchorID  the ID of an element to which the popup should be anchored
> + * @param aPosition  a single-word alignment value for the position parameter
> + *                   of openPopup() method; defaults to "after_start" if omitted.
> + * @param x          x offset over default position

The preposition should be "from", no? Offset from something?

@@ +3890,2 @@
>   */
> +function showPopupById(aPopupID, aAnchorID, aPosition="after_start",

Why lose the spaces around the = ?

@@ -4719,4 @@
>   */
> -function UpdateAttachmentBucket(aShowBucket)
> -{
> -  if (aShowBucket) {

Why lose the |if (aShowBucket) {| ? Do we want to do all the updates if the bucket is hidden in the end?

@@ +5088,4 @@
>  function showReorderAttachmentsPanel() {
> +  // Ensure attachment pane visibility as it might be collapsed.
> +  toggleAttachmentPane(true);
> +  showPopupById("reorderAttachmentsPanel", "attachmentBucket", "after_start", 15);

Please consider 15, 0. A single coordinate sure looks strange even  if the y-value is defaulted.

Comment 9

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: Enable cmd_reorderAttachents "always", polish UpdateAttachmentBucket(), implement showPopupById() [incl. nitfixes]

(In reply to Jorg K (GMT+1) from comment #8)
> Comment on attachment 8939040 [details] [diff] [review]

Thanks! Nitfixes done.

> > -function UpdateAttachmentBucket(aShowBucket)
> > -{
> > -  if (aShowBucket) {
> 
> Why lose the |if (aShowBucket) {| ? Do we want to do all the updates if the
> bucket is hidden in the end?

Yes, as explained in my comment 3 attached to the initial patch for reviewer's interest:
> - update count and total size even when the pane is going to be hidden
> (avoid incorrect UI states, and allow unhiding the panel without updating).

Hidden or not, imo incorrect UI states should be avoided at any time. Some add-on or some programmer (like myself, in bug 1427092) might unhide our hidden UI without updating, even to read values from the UI in the worst case, then it comes out wrong. We don't gain anything because not updating when hiding requires updating whenever we show, which however is not fully under our control if consumers don't use our methods. And "all the updates" is just reading 2 strings, which happens exactly once even after attaching/removing multiple attachments, so the performance impact is negligible.

> > +  showPopupById("reorderAttachmentsPanel", "attachmentBucket", "after_start", 15);
> 
> Please consider 15, 0. A single coordinate sure looks strange even  if the
> y-value is defaulted.

You are right.

Comment 10

[Thomas D. (:thomas8)]

Comment on attachment 8939061 [details] [diff] [review]
Patch V.1.1: Enable cmd_reorderAttachents "always", polish UpdateAttachmentBucket(), implement showPopupById() [incl. nitfixes]

Was ui-reviewed by Richard in bug 1426344.

Comment 11

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #9)
> Created attachment 8939061 [details] [diff] [review]
>
> > Why lose the |if (aShowBucket) {| ? Do we want to do all the updates if the
> > bucket is hidden in the end?
> 
> Yes, as explained in my comment 3 attached to the initial patch for
> reviewer's interest:
> > - update count and total size even when the pane is going to be hidden
> > (avoid incorrect UI states, and allow unhiding the panel without updating).

Furthermore, my bug 1427092 will remove the last instance where this method is actually called with aShowBucket=false, because imo hiding the attachment pane programmatically without explicit user action is bad UX. (Even if you remove all of your attachments, we can't predict that you're not going to add another set, so the right UX is to offer a close button when there's no attachments and let you decide). Meaning that removing the de facto unused condition is actually a micro performance gain every time we call this function.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939061 [details] [diff] [review]
Patch V.1.1: Enable cmd_reorderAttachents "always", polish UpdateAttachmentBucket(), implement showPopupById() [incl. nitfixes]

Thanks for addressing the nits/questions. I'll get this landed.

Comment 13

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #12)
> Thanks for addressing the nits/questions. I'll get this landed.

I know Jörg won't forget, but for my own peace of mind, setting the flag...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c55b6973a109
Enable 'Reorder Attachments' (cmd_reorderAttachments) regardless of focus; implement showPopupById() function. r=jorgk, ui-r=Paenglab