1815210 - Keyboard shortcuts should not propagate to the normal UI while customizing Unified Toolbar

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect, P1)

Description

[Martin Giger [:freaktechnik]]

STR:

• Open unified toolbar customization (right click on the toolbar and select customize)
• Execute a global command shortcut like Ctrl + K

Expected result:
Nothing actually happened

Actual result:
The keyboard handler for the global Ctrl + K shortcut is executed and opens a gloda facet.

Importantly we shouldn't just check if Ctrl + K doesn't propagate, this should apply to all keyboard interactions.

Comment 1

[Thomas D. (:thomas8)]

Imho this is extremely dangerous. Users might think they are already in the customization search box and start typing. Any A in their typing will covertly archive messages away in the background. If for any reason they press Del after entering customization, messages get deleted behind user's back. It's just unpredictable what will happen when keypresses end up on the main UI layer while that's hidden.

Comment 2

[Martin Giger [:freaktechnik]]

A couple of observations:

• We should move focus into the customization when it opens
• Apparently preventingDefault on all (document level) keyboard events makes problems go away (at least partially, I did not check every keyboard shortcut type we have was fixed by it), however, it also makes it so typing doesn't type, so that's not great.

Comment 3

[Alessandro Castellani [:aleca]]

I added this to the unified-toolbar-customization.mjs as a quick test and it seems to work:

this.addEventListener("keydown", event => {
  event.stopPropagation();
});

Comment 4

[Thomas D. (:thomas8)]

(In reply to Martin Giger [:freaktechnik] from comment #2)

A couple of observations:

• We should move focus into the customization when it opens

If the customization search field could have initial focus, that would be great, and also help to reduce the problem a little bit (not entirely at all), as "a" and Del key get absorbed by the text input (but other shortcuts like Ctrl+K are not absorbed because the text input doesn't use them). Of course, that's still not enough because whenever something outside the search box gets focus, we're back to the problem. Alex' proposal of comment 3 looks promising!

Comment 5

[Martin Giger [:freaktechnik]]

(In reply to Alessandro Castellani [:aleca] from comment #3)

I added this to the unified-toolbar-customization.mjs as a quick test and it seems to work:

this.addEventListener("keydown", event => {
  event.stopPropagation();
});

That's not enough in my tests. Make sure you have a selected message before opening the customization.

Comment 6

[Alessandro Castellani [:aleca]]

Yeah, what I suggested seem to work only if the focus is on the search input.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Martin Giger [:freaktechnik] from comment #5)

(In reply to Alessandro Castellani [:aleca] from comment #3)

I added this to the unified-toolbar-customization.mjs as a quick test and it seems to work:

this.addEventListener("keydown", event => {
  event.stopPropagation();
});

That's not enough in my tests. Make sure you have a selected message before opening the customization.

In my tests, this keydown listener didn't even trigger on the <unified-toolbar-customization> when you press a key like "Del" after opening customization. Is that correct?
Because I used a click listener for comparison which triggered, but the keydown listener never triggered here.

Comment 8

[Martin Giger [:freaktechnik]]

Attachment: Bug 1815210 - Prevent commands from being executed in tabs while unified toolbar customization is shown. r=#thunderbird-front-end-reviewers

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9319485 [details]
Bug 1815210 - Prevent commands from being executed in tabs while unified toolbar customization is shown. r=#thunderbird-front-end-reviewers

[Triage Comment]
Optimistically approving for beta.
If testing comes back negative we will back out.

Comment 10

[Daniel Darnell [:dandarnell]]

Thunderbird 111.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/701c87942186

Comment 11

[Wayne Mery (:wsmwk)]

Tests successfull using Windows build from treeherder, per Martin's steps at https://phabricator.services.mozilla.com/D170766#5618238

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a00d2f33bfd6
Prevent commands from being executed in tabs while unified toolbar customization is shown. r=aleca

Comment 13

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #11)

Tests successfull using Windows build from treeherder, per Martin's steps at https://phabricator.services.mozilla.com/D170766#5618238

I have also just tested Martin's steps and they work as expected. Also Del/Shift+Del or A does no longer leak, that's good!
111.0b1 (64-bit) cand build3, en-US, Win10

Comment 14

[Martin Giger [:freaktechnik]]

Attachment: Bug 1815210 - Prevent various global commands from executing while customization is open. r=#thunderbird-front-end-reviewers

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1955f6c5a729
Prevent various global commands from executing while customization is open. r=aleca