Always enable 'Reorder Attachments' command

RESOLVED FIXED in Thunderbird 59.0

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

Tracking

({ux-efficiency})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

2 years ago
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
Assignee

Updated

2 years ago
Blocks: 663695
Assignee

Comment 1

2 years ago
I have fixed this in the current patch for bug 1426344, review pending.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Depends on: 1426344
OS: Unspecified → All
Hardware: Unspecified → All
Assignee

Comment 2

2 years ago
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.
Assignee

Comment 3

2 years ago
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)
Attachment #8939010 - Flags: review?(jorgk)
Assignee

Comment 4

2 years ago
(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

2 years ago
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?
Attachment #8939010 - Flags: review?(jorgk)

Comment 6

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

Comment 7

2 years ago
(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 :)
Attachment #8939010 - Attachment is obsolete: true
Attachment #8939040 - Flags: ui-review+
Attachment #8939040 - Flags: review?(jorgk)

Comment 8

2 years ago
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.
Attachment #8939040 - Flags: review?(jorgk) → review+
Assignee

Comment 9

2 years ago
(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.
Attachment #8939040 - Attachment is obsolete: true
Attachment #8939061 - Flags: review?(jorgk)
Assignee

Comment 10

2 years ago
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.
Attachment #8939061 - Flags: ui-review+
Assignee

Comment 11

2 years ago
(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

2 years ago
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.
Attachment #8939061 - Flags: review?(jorgk) → review+
Assignee

Comment 13

2 years ago
(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...
Keywords: checkin-needed

Comment 14

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c55b6973a109
Enable 'Reorder Attachments' (cmd_reorderAttachments) regardless of focus; implement showPopupById() function. r=jorgk, ui-r=Paenglab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

2 years ago
Target Milestone: --- → Thunderbird 59.0
You need to log in before you can comment on or make changes to this bug.