1425891 - Symptom fix: Remove all access keys from 'Reorder Attachments' panel as some unexpectedly cause the panel to close; add Alt+Y for sorting to restore Alt+V for view menu

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

I'm observing some weird, unexplainable, and sometimes unpredictable behaviour wrt using access keys to trigger reorder actions from the "Reorder Attachments" panel recently implemented by bug 663695.

Tested on a flat install of Jörg's debug trybuild (but I started seeing this before on my flat install of daily which I call daily.workbench):

https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-11c3e0b3a24bc1ed2280f373d893b905896ee92a/try-comm-central-win32-debug/

That's 32bit TB on Windows 10 64bit.

STR

1) Compose msg with multiple attachments (say at least 5), Contacts Side bar NOT shown, keep your compose window simple without more buttons with access keys! (no attachment reminder etc.)
2) Select attachment #3 [details] [diff] [review] (so as to be flexible to move it around in any direction)
3) Alt+X (on Win) or Ctrl+X (on Mac) to show Reorder Attachments Panel - it's important to use keyboard in this step
4) We're now testing access keys (not shortcut keys) of the various reorder actions; on Windows, that requires Alt+Accesskey (I think Alt is required because the panel doesn't have focus, and isn't meant to, and it's not a menu, technically we have buttons - maybe that's the mistake? But we tried menus and it didn't work well):
4a) Alt+T = Move to top
4b) Alt+B = Move to bottom
...and to be tested for all other accesskeys on the panel in the same way!

Actual Result

4a) Alt+T = Move to top
--> Command executes, and Panel stays open (as it should, always)
4b) Alt+B = Move to bottom
--> Command executes, then Panel closes (but shouldn't)

Expected Result

As in 4a), panel should never close from pressing panel actions' access keys.

Here's the full list:

fail = command executes, but panel closes (this bug)
ok = command executes, and panel remains open (expected behaviour)

Alt+T = Move to Top --> ok
Alt+U = Move up --> fail
Alt+V = Move Together --> ok
Alt+D = Move Down --> fail (I reproduced several times that it was working 1st time, but pressing Alt+D again would close the panel - strangely, now I can't reproduce it again).
Alt+B = Move to Bottom --> fail
Alt+O = Sort (Selection) --> fail

Four interesting observations:

A) Alt+T and Alt+V, which always work correctly, are menu access keys of main menu's View and Tools Menu. Selecting a contact from contacts side bar which has Alt+B for Add-to-Bcc will also improve success vor Alt+B for Move-To-Bottom, but not reliably (in my tests, mostly succeeds first time, but not second time)

B) Using mouse (instead of Alt+x) to open the panel, will cause most failing access keys to succeed exactly one time, but second time fails.

C) Keyboard selection with Ctrl+Space now fails to select, but definitely succeeded before, shortly before or after we implemented this.

D) I'm pretty sure that all access keys were working as expected in my tests during implementation (i.e. the panel never closed), but I can't tell exactly when they started to fail. Apart from unexplainable weird behaviour, suspects here might be:
- hacks implemented by myself to make panel stay open/close correctly
- changes in the widget/keyboard focus/event behaviour
Specifically, I remember testing alt+o and definitely it never closed the panel.

Comment 1

[Thomas D. (:thomas8)]

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

> C) Keyboard selection with Ctrl+Space now fails to select, but definitely
> succeeded before, shortly before or after we implemented this.

That's a pretty bad failure seen on the debug trybuild of daily, so if anybody else sees this too, we should file that as a bug.
Select attachment #1 [details] [diff] [review],
Hold Ctrl+key while cursor down to #3,
then press Space while still holding Ctrl+key.
This should select #1 and #3, but now only selects #3. Correct behaviour seen on Release version.

Comment 2

[Jorg K (CEST = GMT+2)]

Since bug 663695 landed on TB 58 on 2017-11-11 two days before the branch to TB 59, I'm trying this ins TB 58 beta.

Alt+Home and Alt+End as advertised on the panel work and don't close the panel, Alt+T doesn't close the panel, but Alt+B does.

Trying the Tinderbox build of the build when the patch landed
https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1510406652/
Alt+B also closes the panel. So this is not a regression, it never worked.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #2)
> Since bug 663695 landed on TB 58 on 2017-11-11 two days before the branch to
> TB 59, I'm trying this ins TB 58 beta.

Thanks for testing!

> Alt+Home and Alt+End as advertised on the panel work and don't close the
> panel, Alt+T doesn't close the panel, but Alt+B does.
> 
> Trying the Tinderbox build of the build when the patch landed
> https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-
> win32/1510406652/
> Alt+B also closes the panel. So this is not a regression, it never worked.

What about Alt+o on that tinderbox build?
I'm quite sure I used that for testing the original sorting algorithm (sorting multiple selections only), and I don't recall any problems with that. Since sorting has an alternating icon which I was testing, it would have bored me if it was closing each time, but that never happened. So for that instance, I would not exclude the possibility of regression, at least from some stage in production to the final implementation.

What about Ctrl+Space selection? Can you reproduce that problem, on your daily, or on that tinderbox build? That was working 100% when I was producing the feature, now it doesn't work, so that looks like a clear regression to me (albeit probably unrelated).

Comment 4

[Thomas D. (:thomas8)]

In terms of easier solutions (assuming it won't be easy to track this down):

- Maybe most actions (except Sort) do NOT need a separate accesskey given that they have much more intuitive shortcut keys. Also, it's unusual and a bit confusing that Alt is required for accesskeys on what looks like a menu. So maybe we can get away with removing the access keys altogether. But sorting does NOT have a shortcut key yet.
- Maybe we could find another alt+... shortcut key for sorting, perhaps Alt+Y? If it works, that would also allow sorting without the panel.

- With more work, we could try to use menus on the panel instead of buttons. But I think out-of-the-box, there were two problems:
  - menu layout wasn't looking right
  - clicking menus also closed the panel :/

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #3)
> What about Alt+o on that tinderbox build?
Alt+o sometimes closed the panel and sometimes doesn't. Typically the first Alt+o doesn't close, the second one does.
Same in Daily as in the built of 11th Nov 2017.

> What about Ctrl+Space selection?
I'm not sure what that is meant to do: On the old built and on Daily, Crtl+Space deselects the current item if it was selected previously with the mouse. I goes from blue to white. Ctrl+Space makes it blue again. It doesn't close the menu.

I'm not the right guy for this, I never use any keyboard shortcuts apart from Crlt+N, Ctrl+E and for editing Ctrl+A.

It's possible that something regressed from the version you used as "flat install" during development until the day the patch got landed.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #5)
> (In reply to Thomas D. from comment #3)
> > What about Alt+o on that tinderbox build?
> Alt+o sometimes closed the panel and sometimes doesn't. Typically the first
> Alt+o doesn't close, the second one does.
> Same in Daily as in the built of 11th Nov 2017.

Thanks for checking. Any clue as to WHY that happens?
I'm especially puzzled that those access keys which duplicate main menu access keys are working *fine*, as I'd expect the opposite.

> > What about Ctrl+Space selection?
> I'm not sure what that is meant to do: On the old built and on Daily,
> Crtl+Space deselects the current item if it was selected previously with the
> mouse. I goes from blue to white. Ctrl+Space makes it blue again. It doesn't
> close the menu.

Yes, it's not about the menu. Yesterday, I was unable to select multiple disjunct items using that method, but today I can't reproduce that problem on the trybuild. 

> I'm not the right guy for this, I never use any keyboard shortcuts apart
> from Crlt+N, Ctrl+E and for editing Ctrl+A.

I see... 

> It's possible that something regressed from the version you used as "flat
> install" during development until the day the patch got landed.

Yes, possibly.

Comment 7

[Thomas D. (:thomas8)]

Attachment: Patch V.1 (workaround): Remove accesskeys, add shortcut key for Sort.

Richard, I suggest removing accesskeys as a workaround fix for this bug because it's unlikely that we'll get to the bottom of this anytime soon, but we shouldn't ship with broken accesskeys.

- introducing Alt+V for "Sort", which works with and without channel. V is iconic for the sorting triangle, and our choice is limited because surprisingly only access keys also found in main menu work correctly.
- So with this patch, we'll have real shortcut keys for all actions, so they are keyboard accessible even without access keys, and keyboard shortcuts are much more explicit as they are advertised on the menu.
- shortcut keys are highly mnemononic, imo better than access keys.
- acceskey usage here was non-standard as it required Alt+... on what looks like  menu.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #7)
> Created attachment 8937869 [details] [diff] [review]
> Patch V.1 (workaround): Remove accesskeys, add shortcut key for Sort.
> 
> Richard, I suggest removing accesskeys as a workaround fix for this bug
> because it's unlikely that we'll get to the bottom of this anytime soon, but
> we shouldn't ship with broken accesskeys.
> 
> - introducing Alt+V for "Sort", which works with and without channel.

reorder panel, not channel.

Comment 9

[Thomas D. (:thomas8)]

Comment on attachment 8937869 [details] [diff] [review]
Patch V.1 (workaround): Remove accesskeys, add shortcut key for Sort.

Ops, Alt+V didn't get copied into the main patch.
We also need a keyboard shortcut for sorting in MAC. Ideas?
Maybe CMD+Alt+V?

Comment 10

[Richard Marti (:Paenglab)]

CMD+OPT+v does nothing on Mac, so it should work.

Comment 11

[Thomas D. (:thomas8)]

Jörg, we're stuck here. Alt+V works perfectly with and without panel to sort attachments, but blocks main menu accesskey Alt+V for View menu even when attachment pane does not have focus.
Alt+Y does not steal the main menu access key, but surprisingly the panel closes after executing sort with Alt+Y (for which I see absolutely no reason; and why doesn't it close for Alt+V)?

What I don't understand:
There's a separate controller for attachment pane.
So I would expect that the keyboard shortcuts of any commands handled by that controller should be freed again when the attachment controller is not in control? Like: When focus in attachmen pane -> Alt+V goes to attachment pane controller, but when focus not in attachment pane -> let it go to main menu!


(In reply to Thomas D. from comment bug 1426344 #3)
> (In reply to Richard Marti (:Paenglab) from bug 1426344 comment #1)
> > > [#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 12

[Thomas D. (:thomas8)]

Bug 1292053 may or may not be related, but we might be able to get help from its assignee if we need it.

Comment 13

[Jorg K (CEST = GMT+2)]

Sorry, I know nothing about acces keys, controllers, etc. Maybe Aceman does, otherwise you need to get help from M-C people, this looks like belonging to the widget component (https://wiki.mozilla.org/Modules/Core#Widget). Also, Neil Deakin ([e-mail]) may be able to help here.

Comment 14

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1 (workaround): Remove accesskeys, add shortcut key Alt+Y for Sort.

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

> Richard, I suggest removing accesskeys as a workaround fix for this bug
> because it's unlikely that we'll get to the bottom of this anytime soon, but
> we shouldn't ship with broken accesskeys.

Removing accesskeys from the panel actions is actually beneficial in terms of access, ux-efficiency, ux-discovery:

- full keyboard access via keyboard shortcuts
- keyboard shortcuts with higher mnemonic value
- keyboard shortcuts are more generic/versatile as they work with and without panel (so it's counterproductive to get used to access keys)
- keyboard shortcuts are maximally discoverable: shown on panel actions
- access keys are less discoverable, because we require non-standard Alt+accesskey for what looks like a menu (but technically isn't, as we're using buttons). Standard for menus is just accesskey (without Alt).

After removing access keys, we need a new keyboard access for 'Sort':
- New: Alt+Y for 'Sort' (conveniently available with and without panel, like all other actions)
- Y is still somewhat iconic for the sorting triangle, and it's not used as a first-level accesskey for anything else in compose window

> - introducing Alt+V for "Sort", which works with and without channel. V is
> iconic for the sorting triangle, and our choice is limited

Turns out that Alt+V wasn't a good idea as it completely hijacks the View menu accesskey (but should not; this bug).

Comment 15

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #14)
> Created attachment 8938623 [details] [diff] [review]
> ...
> After removing access keys, we need a new keyboard access for 'Sort':
> - New: Alt+Y for 'Sort' (conveniently available with and without panel, like
> all other actions)
> - Y is still somewhat iconic for the sorting triangle, and it's not used as
> a first-level accesskey for anything else in compose window

Alt+Y still suffers from this bug (panel closes, but shouldn't), but imo that's acceptable:
- Sort action is unlikely to be used more than once in attachment list (so it doesn't hurt much to close the panel)
- Without panel, you can sort away as often as you like, just using Alt+Y with or without selected attachments.

Comment 16

[Thomas D. (:thomas8)]

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

> Removing accesskeys from the panel actions is actually beneficial in terms
> of access, ux-efficiency, ux-discovery:

...and ux-minimalism: reducing duplicity of accesskeys vs. keyboard shortcuts; less is more...

> - full keyboard access via keyboard shortcuts

Comment 17

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #15)
> (In reply to Thomas D. from comment #14)
> > Created attachment 8938623 [details] [diff] [review]
> > ...
> > After removing access keys, we need a new keyboard access for 'Sort':
> > - New: Alt+Y for 'Sort' (conveniently available with and without panel, like
> > all other actions)
> > - Y is still somewhat iconic for the sorting triangle, and it's not used as
> > a first-level accesskey for anything else in compose window
> 
> Alt+Y still suffers from this bug (panel closes, but shouldn't), but imo
> that's acceptable:
> - Sort action is unlikely to be used more than once in attachment list (so
> it doesn't hurt much to close the panel)
> - Without panel, you can sort away as often as you like, just using Alt+Y
> with or without selected attachments.

Richard, pls test the following:

Without this patch:
- Press Alt+Y in message body. Expected: ¥. Actual -->

With this patch:
- Press Alt+Y in message body. Expected: ¥. Actual -->
- Press Alt+Y with focus in attachment pane. Expected: Sort. Actual --> 

If we're unlucky, Alt+Y in attachment pane might hijack Alt+Y in body (which I'd consider another instance of this bug, because the command is not part of main controller, so keyboard shortcuts should be independent between main and attachment bucket controller.

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 8938623 [details] [diff] [review]
Patch V.1.1 (workaround): Remove accesskeys, add shortcut key Alt+Y for Sort.

(In reply to Thomas D. from comment #17)
> Richard, pls test the following:
> 
> Without this patch:
> - Press Alt+Y in message body. Expected: ¥. Actual --> ¥
> 
> With this patch:
> - Press Alt+Y in message body. Expected: ¥. Actual --> ¥
> - Press Alt+Y with focus in attachment pane. Expected: Sort. Actual --> Sort :)

Comment 19

[Thomas D. (:thomas8)]

Jörg, when checking in, I suggest to leave this bug open for now, because the bug is still there. We're just fixing symptoms.

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #19)
> Jörg, when checking in, I suggest to leave this bug open for now, because
> the bug is still there. We're just fixing symptoms.
Hmm, do you intend to fix the problem in this bug here soon? I don't have the impression, so we'd need a new bug for the follow-up. You might want to change the summary to reflect the "symptom fix".

Comment 21

[Jorg K (CEST = GMT+2)]

Like "disable non-working access keys in reorder attachments".

Comment 22

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1, mostly AFK 22-27 Dec., bustage-fix only) from comment #20)
> (In reply to Thomas D. from comment #19)
> > Jörg, when checking in, I suggest to leave this bug open for now, because
> > the bug is still there. We're just fixing symptoms.
> Hmm, do you intend to fix the problem in this bug here soon? I don't have
> the impression, so we'd need a new bug for the follow-up. You might want to
> change the summary to reflect the "symptom fix".

Most of the symptomatic description here is still perfectly correct, so in a new bug we'll just start out by describing mostly the same, and I wanted to avoid all that work for myself and just keep this open instead. I don't think it matters when it is going to be fixed as long as it describes a real bug (we could just update STR a bit). Symptom fix is orthogonal, doesn't change the underlying problem.
I'd rather have an imperfect bug on record than not have any bug on record, which will be the situation after closing this. I'm not saying it's a bad idea to file a new bug trying to abstract a bit more while preserving valuable symptomatic information, but who's going to do that?

Anyway, feel free to proceed either way.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b92a49ff3bd9
Remove malfunctioning accesskeys from Reorder Attachments panel, add shortcut for sorting attachments. r=Paenglab