1426344 - MAC keyboard shortcuts for 'Reorder Attachments' actions revisited (followup of bug 663695)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #4)
> Oh, and the Alt key won't work on Mac (Alt-Up is Home and Alt-Down is End),
> so you'll need to come up with different keys there.  "Things" uses Cmd-Up
> and Cmd-Down.  "iTunes" doesn't offer the capability at all.

Looking at Apple's official keyboard shortcuts page [1], I think this comment isn't factually accurate:

> Fn–Left Arrow 	Home: Scroll to the beginning of a document.
> Fn–Right Arrow 	End: Scroll to the end of a document.

So the current implementation of MAC keyboard shortcuts for Reorder Attachments panel is based on the wrong premise of that comment, which makes it unnecessarily complicated and different from Windows.

So I think we can revisit/simplify our keyboard shortcuts on MAC and make the life of TB support and users easier by having mostly the same key combinations on Windows/Linux and MAC.

Richard, what do you think?

On MAC keyboards, Option key is equivalent of ALT key, so I'll just use ALT to show the congruence between OSs.

Function             Win/Linux       Mac (Proposal of this bug)

Show Reorder Panel   Alt+X           Ctrl+X             [#0]
                                     Alt+X              
Move to Top          Alt+Home        Alt+Command+Arrow-Up   [#1]
                                     Alt+Home           [#2]
Move Up              Alt+Arrow-Up    Alt+Arrow-Up
Move together (up)   Alt+Arrow-Left  Alt+Arrow-Left     [#3]
Move together (down) Alt+Arrow-Right Alt+Arrow-Right    [#3]
Move Down            Alt+Arrow-Down  Alt+Arrow-Down
Move to Bottom       Alt+End         Alt+Command+Arrow-Down [#4]
                                     Alt+End            [#2]
Sort                 Alt+V           Alt+V              [#5]

So that's maximally cross-OS consistent and intuitive, while not violating any traditional patterns afasics.

[#0] Richard said that on MAC, the access key for Attachments Pane is Ctrl+M, so Ctrl+X would be more consistent and convenient for showing the panel after that (rather than Alt+X). It's a bit of an odd-one-out, but opening the panel is arguably another level of action compared to the movement keys. But I think we should offer Alt+X as an alternative shortcut for consistency's sake. Just curiosity: What do you press on MAC to focus subject?

[#1] On Mac, according to [1], Command+Arrow-Up is "Move the insertion point to the beginning of the document." So then, as with all the other actions, we add ALT modifier for *moving the element* rather than just the focus. Which sounds plausible and consistent to me. Unless someone more familiar with Mac keyboard shortcuts tells me otherwise, I think that's a reasonable compromise, plausible, efficient, transparent and somewhat consistent.
- The condensed Apple keyboard doesn't have a home key, the official equivalent is Fn-Arrow-Left (which isn't transparent nor intuitive at all), but according to documentation [1], that means "Scroll to the beginning of the document", which is NOT what Windows Home key does and what we want here.
- So we're using keys which are directly/transparently available on any MAC keyboard, not cryptic combinations like Fn-Arrow-Left.
- Alt+Cmd are comfortable/efficient modifiers as they are next to each other, easy to press.
- Recommended by CNET:
https://www.cnet.com/how-to/two-mac-keyboard-shortcuts-for-missing-home-and-end-keys/

[#2] I think we have to offer Alt+Home / Alt+End as alternative shortcuts on MAC, because I understand that Home/End (= fn+left-arrow/fn+right-arrow) are the only official shortcuts to jump-select first/last item. And apart from being clumsy for many cases, I don't see anything wrong with that, it's useful e.g. for the extended MAC keyboard which DOES have dedicated Home/End keys. However, for the reasons given in [#1], we don't advertise this on the menu, only in documentation. We've done similar things e.g. with Ctrl+I vs. Alt+Enter for properties, allowing both shortcuts on both platforms, but advertising only the more platform-conforming one in menus.

[#3] Alt+Arrow-Left/Right for Move-together-upwards/downwards: Here we just keep things consistent within our own set of shortcuts, both on Windows/Linux and MAC. On MAC, Alt+Arrow-Left/Right originally means "Move the insertion point to the beginning of the previous/next word." Fair enough.

[#4] By analogy, same as [#1].

[#5] Alt+V for Sort (with and without panel): If this works on MAC, I think that's most consistent:
- Same "movement modifier" key for all movement actions: ALT+...
- Cross-OS consistency

[1] https://support.apple.com/en-us/HT201236

Comment 1

[Richard Marti (:Paenglab)]

(In reply to Thomas D. from comment #0)
> [#0] Richard said that on MAC, the access key for Attachments Pane is
> Ctrl+M, so Ctrl+X would be more consistent and convenient for showing the
> panel after that (rather than Alt+X). It's a bit of an odd-one-out, but
> opening the panel is arguably another level of action compared to the
> movement keys. But I think we should offer Alt+X as an alternative shortcut
> for consistency's sake. Just curiosity: What do you press on MAC to focus
> subject?

control+s

> [#5] Alt+V for Sort (with and without panel): If this works on MAC, I think
> that's most consistent:

Attention, ALT+V is on Linux/Windows the access key for the View menu also when the focus is in the attachments pane.

Comment 2

[Thomas D. (:thomas8)]

Richard, could you please check if any of the above keyboard shortcuts proposed for MAC currently do something else.
If they do nothing, we're fine to use them. For your testing, please adhere strictly to following procedure (to avoid false positives):

0) To be tested on TB release, not Daily (to avoid artefacts from shortcuts we already implemented for this feature)
1) Message with a list of 5 attachment items.
2) Select attachment #3 [details] [diff] [review] in the middle of the list (important! every time)
3) press MAC keyboard shortcut from comment 0 proposal:

Mac (Proposal of this bug)

Ctrl+X   (with focus on message body and one word selected)  --> what happens?          
Option+X (with focus on message body and one word selected)  --> ...             
Option+Command+Arrow-U (with attachment #3 [details] [diff] [review] selected) --> what happens?
Option+Home            (dito...) -->
Option+Arrow-Up        -->
Option+Arrow-Left      -->
Option+Arrow-Right     -->
Option+Arrow-Down      -->
Option+Command+Arrow-D -->
Option+End             -->
Option+V               -->
Option+Y               --> (instead of Option+V, if we can get it to work, Bug 1425891)

--> what happens for each of these shortcuts?

Expected Result for all of these: nothing :)

Comment 3

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #1)
> (In reply to Thomas D. from comment #0)
> > [#0] Richard said that on MAC, the access key for Attachments Pane is
> > Ctrl+M, so Ctrl+X would be more consistent and convenient for showing the
> > panel after that (rather than Alt+X). It's a bit of an odd-one-out, but
> > opening the panel is arguably another level of action compared to the
> > movement keys. But I think we should offer Alt+X as an alternative shortcut
> > for consistency's sake. Just curiosity: What do you press on MAC to focus
> > subject?
> 
> control+s
> 
> > [#5] Alt+V for Sort (with and without panel): If this works on MAC, I think
> > that's most consistent:
> 
> Attention, ALT+V is on Linux/Windows the access key for the View menu also
> when the focus is in the attachments pane.

Yes, I was aware of that, and I deliberately chose an existing main menu accesskey to work around Bug 1425891. But I only just realized that contrary to expectation, this hijacks the accesskey not only when focus is in attachment pane, but for good. I would have accepted focus-based hijacking, but total hijacking is a no-go.
Note that if you first press and release Alt, then press V, it will always work for the main menu.
We're stuck between a rock and a hard place here. If we make it work for the panel, we all but kill the main menu access key.
If we use something which doesn't affect the main menu, like Alt+Y, it doesn't work for the panel, which will close when it shouldn't. But something is definitely wrong in the way these keyboard events are handled.

I guess we have to go with Alt+Y for now and accept panel to close prematurely, which is arguably better than hijacking a main menu access key. Any other ideas?

Comment 4

[Richard Marti (:Paenglab)]

> Ctrl+X   (with focus on message body and one word selected)  --> what happens?  nothing        
> Option+X (with focus on message body and one word selected)  --> ...   I get a double tilde (this could be de-CH keyboard special)
> 
> Option+Command+Arrow-U (with attachment 3 [details] [diff] [review] selected) --> what happens? nothing
> Option+Home            (dito...) --> not available
> Option+Arrow-Up        -->  nothing
> Option+Arrow-Left      -->  nothing
> Option+Arrow-Right     -->  nothing
> Option+Arrow-Down      -->  nothing
> Option+Command+Arrow-D -->  move to bottom
> Option+End             -->  not available
> Option+V               -->  nothing
> Option+Y               -->  nothing
> 
> --> what happens for each of these shortcuts?
> 
> Expected Result for all of these: nothing :)

All tested on TB 52.

Comment 5

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #4)
> > Ctrl+X   (with focus on message body and one word selected)  --> what happens?  nothing        
> > Option+X (with focus on message body and one word selected)  --> ...   I get a double tilde (this could be de-CH keyboard special)

Oh, I see. Apparently a lot of Alt+Character combinations create special characters on MAC (although strangely ≈ is not among them, maybe a charset issue):

https://forlang.wsu.edu/help-pages/help-pages-keyboards-os-x/

> > Option+Command+Arrow-U (with attachment 3 [details] [diff] [review] selected) --> what happens? nothing
> > Option+Home            (dito...) --> not available

Home = Fn+Cursor-Left
Pls try Option+Fn+Cursor-Left (on 3rd attachment).

> > Option+Arrow-Up        -->  nothing
> > Option+Arrow-Left      -->  nothing
> > Option+Arrow-Right     -->  nothing
> > Option+Arrow-Down      -->  nothing
> > Option+Command+Arrow-D -->  *move to bottom*

That (move to bottom) looks like an error in test results...

> > Option+End             -->  not available

End = Fn+Cursor-Right
Pls try Option+Fn+Cursor-Right (on 3rd attachment).

> > Option+V               -->  nothing
> > Option+Y               -->  nothing
> > 
> > --> what happens for each of these shortcuts?
> > 
> > Expected Result for all of these: nothing :)
> 
> All tested on TB 52.

Thanks.

Comment 6

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Align MAC shortcuts with Windows/Linux

Richard, can you please test this again, now with this patch applied.

I'll mark this as "tentative" as there's two many bugs and odd/unexpected behaviour in this corner.

Especially, for MAC, we have to ensure that we don't hijack the functionality of:
- Alt+character key combinations to insert special characters in body, and
- Alt+movement key combinations for movement in body.

I'll add test matrix in the next comment.

Comment 7

[Thomas D. (:thomas8)]

Test Matrix (hope I got that right now, looks more complicated than it is...)

Mac (Proposal of this bug)

Round #1:
0) *Without* Patch
1) Use message body having this text:
> Line1 This is the beginning of the text,
> Line2 And here's our focus in second line.
> LIne3 This is line 3.
2) Select the word "focus" in line 2 every time before checking another shortcut (so that the effect of movement keys can be observed).
3) Apply shortcut (with focus in msg body on the word "focus")

(Expected: Nothing, some cursor movements wordwise etc., special chars in body)
Shortcut applied in    --> Actual Result
center of msg body         *without* patch

Ctrl+X                    --> 
Option+X                  -->              
Option+Command+Arrow-Up   -->
Option+Home (Opt+Fn+Left) -->
Option+Arrow-Up           -->
Option+Arrow-Left         -->
Option+Arrow-Right        -->
Option+Arrow-Down         -->
Option+Command+Arrow-Down -->
Option+End  (Opt+Fn+Right)-->
Option+V                  -->
Option+Y                  -->
Ctrl+V                    -->
Ctrl+Y                    -->


Round #2:
0) *With* Patch applied
1) Use message body having this text:
> Line1 This is the beginning of the text,
> Line2 And here's our focus in second line.
> LIne3 This is line 3.
2) Select the word "focus" in line 2 every time before checking another shortcut (so that the effect of movement keys can be observed).
3) Apply shortcut (with focus in msg body on the word "focus")

(Expected: Nothing, some cursor movements wordwise etc., special chars in body)
Shortcut applied in    --> Actual Result
center of msg body         *with* patch applied

Ctrl+X                    --> 
Option+X                  -->              
Option+Command+Arrow-Up   -->
Option+Home (Opt+Fn+Left) -->
Option+Arrow-Up           -->
Option+Arrow-Left         -->
Option+Arrow-Right        -->
Option+Arrow-Down         -->
Option+Command+Arrow-Down -->
Option+End  (Opt+Fn+Right)-->
Option+V                  -->
Option+Y                  -->
Ctrl+V                    -->
Ctrl+Y                    -->


Round #3:
0) With Patch
1) Msg with 5 attachments
2) Select attachment #3 [details] [diff] [review] before each test
3) Apply shortcut (with focus on attachment #3 [details] [diff] [review])

(Expected: Mostly Attachment movements)
Shortcut applied on       --> Actual Result
selected attachment #3 [details] [diff] [review]        *with* patch applied

Ctrl+X                    --> 
Option+X                  -->              
Option+Command+Arrow-Up   -->
Option+Home (Opt+Fn+Left) -->
Option+Arrow-Up           -->
Option+Arrow-Left         -->
Option+Arrow-Right        -->
Option+Arrow-Down         -->
Option+Command+Arrow-Down -->
Option+End  (Opt+Fn+Right)-->
Option+V                  -->
Option+Y                  -->
Ctrl+V                    -->
Ctrl+Y                    -->

Comment 8

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #7)
> Test Matrix (hope I got that right now, looks more complicated than it is...)
> 
> Mac (Proposal of this bug)
> 
> Round #1:
> 0) *Without* Patch
> 1) Use message body having this text:
> > Line1 This is the beginning of the text,
> > Line2 And here's our focus in second line.
> > LIne3 This is line 3.
> 2) Select the word "focus" in line 2 every time before checking another
> shortcut (so that the effect of movement keys can be observed).
> 3) Apply shortcut (with focus in msg body on the word "focus")
> 
> (Expected: Nothing, some cursor movements wordwise etc., special chars in
> body)
> Shortcut applied in    --> Actual Result
> center of msg body         *without* patch
> 
> Ctrl+X                    --> 
> Option+X                  -->              
> Option+Command+Arrow-Up   -->
> Option+Home (Opt+Fn+Left) -->
> Option+Arrow-Up           -->
> Option+Arrow-Left         -->
> Option+Arrow-Right        -->
> Option+Arrow-Down         -->
> Option+Command+Arrow-Down -->
> Option+End  (Opt+Fn+Right)-->
> Option+V                  -->
> Option+Y                  -->
> Ctrl+V                    -->
> Ctrl+Y                    -->
> 
> 
> Round #2:
> 0) *With* Patch applied

- Message must have attachments,
- Select one attachment before selecting word in msg body
(So that we activate the attachment bucket controller)

> 1) Use message body having this text:
> > Line1 This is the beginning of the text,
> > Line2 And here's our focus in second line.
> > LIne3 This is line 3.
> 2) Select the word "focus" in line 2 every time before checking another
> shortcut (so that the effect of movement keys can be observed).
> 3) Apply shortcut (with focus in msg body on the word "focus")
> 
> (Expected: Nothing, some cursor movements wordwise etc., special chars in
> body)
> Shortcut applied in    --> Actual Result
> center of msg body         *with* patch applied
> 
> Ctrl+X                    --> 
> Option+X                  -->              
> Option+Command+Arrow-Up   -->
> Option+Home (Opt+Fn+Left) -->
> Option+Arrow-Up           -->
> Option+Arrow-Left         -->
> Option+Arrow-Right        -->
> Option+Arrow-Down         -->
> Option+Command+Arrow-Down -->
> Option+End  (Opt+Fn+Right)-->
> Option+V                  -->
> Option+Y                  -->
> Ctrl+V                    -->
> Ctrl+Y                    -->
> 
> 
> Round #3:
> 0) With Patch
> 1) Msg with 5 attachments
> 2) Select attachment #3 [details] [diff] [review] [diff] [review] before each test
> 3) Apply shortcut (with focus on attachment #3 [details] [diff] [review] [diff] [review])
> 
> (Expected: Mostly Attachment movements)
> Shortcut applied on       --> Actual Result
> selected attachment #3 [details] [diff] [review] [diff] [review]        *with* patch applied
> 
> Ctrl+X                    --> 
> Option+X                  -->              
> Option+Command+Arrow-Up   -->
> Option+Home (Opt+Fn+Left) -->
> Option+Arrow-Up           -->
> Option+Arrow-Left         -->
> Option+Arrow-Right        -->
> Option+Arrow-Down         -->
> Option+Command+Arrow-Down -->
> Option+End  (Opt+Fn+Right)-->
> Option+V                  -->
> Option+Y                  -->
> Ctrl+V                    -->
> Ctrl+Y                    -->

Comment 9

[Thomas D. (:thomas8)]

Note to self: If current patch hijacks Alt+... movement/special char keys from msg body, we could try using keypress events on attachment bucket instead of <key>. That way, we might succeed to have non-interfering, but cross-os keyboard shortcuts which only apply to the attachment bucket.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8938633 [details] [diff] [review]
Patch V.1: Align MAC shortcuts with Windows/Linux

(In reply to Thomas D. from comment #8)
> (In reply to Thomas D. from comment #7)
> > Test Matrix (hope I got that right now, looks more complicated than it is...)
> > 
> > Mac (Proposal of this bug)
> > 
> > Round #1:
> > 0) *Without* Patch
> > 1) Use message body having this text:
> > > Line1 This is the beginning of the text,
> > > Line2 And here's our focus in second line.
> > > LIne3 This is line 3.
> > 2) Select the word "focus" in line 2 every time before checking another
> > shortcut (so that the effect of movement keys can be observed).
> > 3) Apply shortcut (with focus in msg body on the word "focus")
> > 
> > (Expected: Nothing, some cursor movements wordwise etc., special chars in
> > body)
> > Shortcut applied in    --> Actual Result
> > center of msg body         *without* patch
> > 
> > Ctrl+X                    --> nothing
> > Option+X                  --> double tilde
> > Option+Command+Arrow-Up   --> nothing
> > Option+Home (Opt+Fn+Left) --> nothing
> > Option+Arrow-Up           --> start of line
> > Option+Arrow-Left         --> start of "focus"
> > Option+Arrow-Right        --> end of "focus"
> > Option+Arrow-Down         --> end of line
> > Option+Command+Arrow-Down --> nothing
> > Option+End  (Opt+Fn+Right)--> nothing
> > Option+V                  --> check sign exchanges "focus"
> > Option+Y                  --> Yen sign exchanges "focus"
> > Ctrl+V                    --> nothing
> > Ctrl+Y                    --> nothing
> > 
> > 
> > Round #2:
> > 0) *With* Patch applied
> 
> - Message must have attachments,
> - Select one attachment before selecting word in msg body
> (So that we activate the attachment bucket controller)
> 
> > 1) Use message body having this text:
> > > Line1 This is the beginning of the text,
> > > Line2 And here's our focus in second line.
> > > LIne3 This is line 3.
> > 
> > Shortcut applied in    --> Actual Result
> > center of msg body         *with* patch applied
> > 
> > Ctrl+X                    --> nothing but menuitem edit flashes
> > Option+X                  --> double tilde
> > Option+Command+Arrow-Up   --> nothing
> > Option+Home (Opt+Fn+Left) --> nothing
> > Option+Arrow-Up           --> start of line
> > Option+Arrow-Left         --> start of "focus"
> > Option+Arrow-Right        --> end of "focus"
> > Option+Arrow-Down         --> end of line
> > Option+Command+Arrow-Down --> nothing
> > Option+End  (Opt+Fn+Right)--> nothing
> > Option+V                  --> check sign exchanges "focus"
> > Option+Y                  --> Yen sign exchanges "focus"
> > Ctrl+V                    --> nothing
> > Ctrl+Y                    --> nothing
> > 
> > 
> > Round #3:
> > 0) With Patch
> > 1) Msg with 5 attachments
> > 2) Select attachment #3 [details] [diff] [review] [diff] [review] [diff] [review] before each test
> > 3) Apply shortcut (with focus on attachment #3 [details] [diff] [review] [diff] [review] [diff] [review])
> > 
> > (Expected: Mostly Attachment movements)
> > Shortcut applied on       --> Actual Result
> > selected attachment 3 [details] [diff] [review] *with* patch applied
> > 
> > Ctrl+X                    --> attachment panel appears
> > Option+X                  --> attachment panel appears
> > Option+Command+Arrow-Up   --> attachment on top (tried also with small listbox and first attachment not in view)
> > Option+Home (Opt+Fn+Left) --> attachment on top
> > Option+Arrow-Up           --> attachment 1 [details] [diff] [review] step up
> > Option+Arrow-Left         --> nothing
> > Option+Arrow-Right        --> nothing
> > Option+Arrow-Down         --> attachment 1 [details] [diff] [review] step down
> > Option+Command+Arrow-Down --> attachment on bottom (tried also with small listbox and last attachment not in view)
> > Option+End  (Opt+Fn+Right)--> attachment on bottom
> > Option+V                  --> nothing
> > Option+Y                  --> sort
> > Ctrl+V                    --> nothing
> > Ctrl+Y                    --> nothing

Comment 11

[Jorg K (CEST = GMT+2)]

Why the quote in comment #10?

Comment 12

[Richard Marti (:Paenglab)]

The answers are in the quoted text.

Comment 13

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #10)
> Comment on attachment 8938633 [details] [diff] [review]

Thanks for testing, and the results look good! :)
Meaning we are good to have essentially the same shortcuts for all OSs.

> (In reply to Thomas D. from comment #8)
> > (In reply to Thomas D. from comment #7)
> > > Test Matrix (hope I got that right now, looks more complicated than it is...)

I think you tested Round #3 with 'Reorder Attachments' panel NOT shown, right?
Just to be on the safe side, could you do another round with Panel shown?
(Expected: Same result as below, and panel should never hide).
I suggest putting an asterix (*) in front of every result that has been confirmed.

> > > Round #3:
> > > 0) With Patch

0) b) Show 'Reorder attachments' panel

> > > 1) Msg with 5 attachments
> > > 2) Select attachment #3 [details] [diff] [review] [diff] [review] [diff] [review] [diff] [review] before each test
> > > 3) Apply shortcut (with focus on attachment #3 [details] [diff] [review] [diff] [review] [diff] [review] [diff] [review])
> > > 
> > > (Expected: Mostly Attachment movements)
> > > Shortcut applied on       --> Actual Result
> > > selected attachment 3 [details] [diff] [review] *with* patch applied
> > > 
> > > Ctrl+X                    --> attachment panel appears
> > > Option+X                  --> attachment panel appears
> > > Option+Command+Arrow-Up   --> attachment on top (tried also with small listbox and first attachment not in view)
> > > Option+Home (Opt+Fn+Left) --> attachment on top
> > > Option+Arrow-Up           --> attachment 1 [details] [diff] [review] step up

> > > Option+Arrow-Left         --> nothing
> > > Option+Arrow-Right        --> nothing

Oh, my mistake, pls retest these two after selecting attachment #3 [details] [diff] [review] and #5 (with and without panel)
Expected: Move together upwards/downwards.

(Proceed with panel open vv)

> > > Option+Arrow-Down         --> attachment 1 [details] [diff] [review] step down
> > > Option+Command+Arrow-Down --> attachment on bottom (tried also with small listbox and last attachment not in view)
> > > Option+End  (Opt+Fn+Right)--> attachment on bottom
> > > Option+V                  --> nothing
> > > Option+Y                  --> sort
> > > Ctrl+V                    --> nothing
> > > Ctrl+Y                    --> nothing

Comment 14

[Thomas D. (:thomas8)]

Comment on attachment 8938633 [details] [diff] [review]
Patch V.1: Align MAC shortcuts with Windows/Linux

So according to Richard's tests (assuming the last one in comment 13 also passes), this looks ready.

Please set checkin-needed if the patch is ready to land as-is.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8938633 [details] [diff] [review]
Patch V.1: Align MAC shortcuts with Windows/Linux

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

::: mail/components/compose/content/messengercompose.xul
@@ +256,5 @@
>    <key id="key_reorderAttachments"
>         key="&reorderAttachmentsCmd.key;" modifiers="control"
>         command="cmd_reorderAttachments"/>
> +  <key id="key_reorderAttachments2"
> +       key="&reorderAttachmentsCmd.key2;" modifiers="alt"

Is the reorderAttachmentsCmd.key2 needed? Couldn't you use the normal reorderAttachmentsCmd.key? If not, this would need a explanation for the localizers.

Comment 16

[Richard Marti (:Paenglab)]

No n-i needed with the pending r?

Comment 17

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #15)

> > +  <key id="key_reorderAttachments2"
> > +       key="&reorderAttachmentsCmd.key2;" modifiers="alt"
> 
> Is the reorderAttachmentsCmd.key2 needed? Couldn't you use the normal
> reorderAttachmentsCmd.key? If not, this would need a explanation for the
> localizers.

I think you're right, I can use the same .key for both shortcuts, given that we've tested that the alternative Alt+X shortcut only applies with focus on attachment bucket, so it doesn't prevent e.g. Alt+X in message body for entering characters.
If it's ok, I'll fix this in the final iteration when you've tested comment 13 and completed the review.

(In reply to Richard Marti (:Paenglab) from comment #16)
> No n-i needed with the pending r?

Yeah, the n-i was for the last pending test in comment 13 which then becomes part of the final review...

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 8938633 [details] [diff] [review]
Patch V.1: Align MAC shortcuts with Windows/Linux

Tested comment 13.

r+ with removing reorderAttachmentsCmd.key2 if possible.

Comment 19

[Thomas D. (:thomas8)]

For the avoidance of doubt, since I've asked Richard to test some extra keys, the matrix implemented in the latest patch is still the same as proposed and explained in comment 0, with one exception: Sort (originally proposed as Alt+V) becomes Alt+Y, implemented in Bug 1425891.

> Function             Win/Linux       Mac (Proposal of this bug)
>
> Show Reorder Panel   Alt+X           Ctrl+X             [#0]
>                                      Alt+X              
> Move to Top          Alt+Home        Alt+Command+Arrow-Up   [#1]
>                                      Alt+Home           [#2]
> Move Up              Alt+Arrow-Up    Alt+Arrow-Up
> Move together (up)   Alt+Arrow-Left  Alt+Arrow-Left     [#3]
> Move together (down) Alt+Arrow-Right Alt+Arrow-Right    [#3]
> Move Down            Alt+Arrow-Down  Alt+Arrow-Down
> Move to Bottom       Alt+End         Alt+Command+Arrow-Down [#4]
>                                      Alt+End            [#2]
  Sort                 Alt+Y           Alt+Y              [#5]

Comment 20

[Thomas D. (:thomas8)]

Attachment: Patch V.2: Align MAC shortcuts with Windows/Linux (except Ctrl+X to show the panel); fix bug 1427037

(In reply to Richard Marti (:Paenglab) from comment #18)
> Comment on attachment 8938633 [details] [diff] [review]
> Patch V.1: Align MAC shortcuts with Windows/Linux
> 
> Tested comment 13.
> 
> r+ with removing reorderAttachmentsCmd.key2 if possible.

Thanks. In fact, I was doing this in the wrong order.

So it turns out that we can't have Alt+X to show the Reorder Attachments Panel on MAC because bug 1427037 requires that cmd_reorderAttachments also works when focus is NOT in attachmentBucket, e.g. in body. That's where Alt+X would conflict with typing special characters in MAC. So we could only have Alt+X on MAC when focus is already on the bucket, but ah well, that's no longer much helpful. So I'm removing <key id="key_reorderAttachments2"...> (Alt+X) on MAC altogether.

I'm now also fixing bug 1427037 here by simply moving cmd_reorderAttachments from the attachmentBucketController into the defaultController, then we only use Ctr+X for that on MAC. I also added a 2-line tweak to ensure that cmd_reorderAttachments will first show the attachment pane in case it is hidden.

Comment 21

[Richard Marti (:Paenglab)]

Comment on attachment 8938896 [details] [diff] [review]
Patch V.2: Align MAC shortcuts with Windows/Linux (except Ctrl+X to show the panel); fix bug 1427037

LGTM, thanks.

Comment 22

[Thomas D. (:thomas8)]

Richard, thank you very much for all the patient testing on MAC and rapid review!
Time to get this out of the way, I'm not a big fan of digging into MAC keyboard shortcuts...

I'll be happy to see this land so that we polish and stabilize "Reorder Attachments" feature set of bug 663695 before the next release, and also to reduce my patch queue...

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8938896 [details] [diff] [review]
Patch V.2: Align MAC shortcuts with Windows/Linux (except Ctrl+X to show the panel); fix bug 1427037

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

Something fishy here. I applied the patch, then did a |hg qref|. The result was a huge different patch.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +673,5 @@
>          AttachPage();
>        }
>      },
>  
> +    cmd_reorderAttachments: {

Why are you moving this function? Can I remove this hunk?

@@ +5058,5 @@
>    // handlers, so we must do it now as the position of selected items has changed.
>    updateReorderAttachmentsItems();
>  }
>  
> +function showAttachmentPane(aShow = true) {

No need for the default value.

@@ +5059,5 @@
>    updateReorderAttachmentsItems();
>  }
>  
> +function showAttachmentPane(aShow = true) {
> +    document.getElementById("attachments-box").collapsed = !aShow;

Wrong indentation here.

@@ +5067,2 @@
>  function showReorderAttachmentsPanel() {
> +    showAttachmentPane();

Just add true here. Also wrong indentation here.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1426344_MAC-shortcuts-reorder-attachments-revisited.patch

I think this is what it should be.

Comment 25

[Thomas D. (:thomas8)]

Comment on attachment 8938896 [details] [diff] [review]
Patch V.2: Align MAC shortcuts with Windows/Linux (except Ctrl+X to show the panel); fix bug 1427037

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

Thank you for trying to land this. I don't know about the fishyness, but I'm working on a lot of patches in the same area so it's possible that there was something wrong, so thank you for fixing that. However, removing the hunk doesn't work and I'd prefer to keep omitting the default argument in the function call.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +673,5 @@
>          AttachPage();
>        }
>      },
>  
> +    cmd_reorderAttachments: {

No. Explained in patch description ("... fix bug 1427037") and comment 20 which was associated with the patch hence readily available for reviewers. You can't remove this hunk unless if you want to re-add it under my name in bug 1427037 and land that separately.

@@ +5058,5 @@
>    // handlers, so we must do it now as the position of selected items has changed.
>    updateReorderAttachmentsItems();
>  }
>  
> +function showAttachmentPane(aShow = true) {

I deliberately set a default value to simplify the default caller.

I guess it's a matter of taste, but I prefer callers to be as simple as possible. I see no value in being forced to use
> showAttachmentPane(true)
when calling the function without the argument works just as well while leaving no doubt about what it does:
> showAttachmentPane()

The default=true flavor of the function is more frequent than false, which will probably occur only once in the entire codebase, when I add an (x) button for user to close the attachment pane when there are no attachments. Otherwise afasics it's wrong UX to programmatically close the pane, ever. So 'false' is a rare special case of this function which generally just does what it says, and (true) argument doesn't seem to add any value while needlessly inflating code size by a few bytes and arguably making code less readable as it's not immediately clear what (true) does beyond just showing the pane.

@@ +5059,5 @@
>    updateReorderAttachmentsItems();
>  }
>  
> +function showAttachmentPane(aShow = true) {
> +    document.getElementById("attachments-box").collapsed = !aShow;

Thank you for nitfixing the wrong indentation.

@@ +5067,2 @@
>  function showReorderAttachmentsPanel() {
> +    showAttachmentPane();

Dito.

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 8938968 [details] [diff] [review]
1426344_MAC-shortcuts-reorder-attachments-revisited.patch

f- as explained in comment 25.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8938896 [details] [diff] [review]
Patch V.2: Align MAC shortcuts with Windows/Linux (except Ctrl+X to show the panel); fix bug 1427037

Sorry, yes I see now that cmd_reorderAttachments moved from attachmentBucketController to defaultController.

As for showAttachmentPane(), since this is a new function, there are only two callers:
  showAttachmentPane(aShowBucket)
  showAttachmentPane()
Readers of the code would typically refer to the function documentation, which sadly is non-existent in this case. Typically we add documentation blocks for new code. Maybe the function is badly named, since it can also hide the attachment pane.

Comment 28

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: Align MAC shortcuts with Windows/Linux (except Alt+X to show the panel)

Alright, to avoid further delays for this simple-in-code but hard-tested change, let's not do bug 1427037 here.

The remaining code changes are trivial and have been reviewed by Paenglab in comment 21.

Comment 29

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #28)
> Created attachment 8939009 [details] [diff] [review]
> Patch V.1.1: Align MAC shortcuts with Windows/Linux (except Alt+X to show
> the panel)
> 
> Alright, to avoid further delays for this simple-in-code but hard-tested
> change, let's not do bug 1427037 here.
> 
> The remaining code changes are trivial and have been reviewed by Paenglab in
> comment 21.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1138aa902823
Mac keyboard shortcuts for 'Reorder Attachments' actions revisited (follow-up of bug 663695). r=Paenglab