1833494 - Keyboard zoom shortcut switches tabs when zoom level maxed out with certain keyboard layouts

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Austin Donisan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/113.0

Steps to reproduce:

Install the AZERTY keyboard layout
Open 6 tabs
Go to tab 1
Repeatedly hit "Ctrl -" to decrease the zoom level

Actual results:

After the zoom level hits 30% you will jump to tab 6

Expected results:

You should remain on tab 1 with the zoom floored at 30%

Comment 1

[Austin Donisan]

Note that this same bug happens with the Programmer Dvorak keyboard layout, but with "Ctrl +" shortcut and jumping you to tab 4 after the zoom level hits 500%.

I feel like this is a recent regression since I haven't noticed this before.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Tabbed Browser' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Austin Donisan]

I guess "recently" was a stretch; this regressed between builds 20220610213450 and 20220611095155. I think the fix for bug 698873 is the cause.

Comment 4

[Austin Donisan]

In that diff cmd_fullZoomEnlarge and cmd_fullZoomReduce are disabled when the zoom is maxed out. In browser-sets.inc the keyboard shortcuts mapped to those commands are defined above the those for tab switching. When those commands are disabled the keyboard shortcut matching falls through to the tab switching ones.

Creating new commands that are always enabled and mapping those to the keyboard shortcuts fixes the problem. I'll try to figure out how to submit a patch with this fix.

Comment 5

[Rares Doghi, Desktop QA]

I was also able to reproduce this issue using the Azerty keyboard layout (took me a while until i realised the "-" button is the Nr. 8 on my keyboard :)) ). I was also able to get a regression range for this issue and as stated in Comment 3 the Fix for Bug 698873 is whats causing it.

Here is the pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=69a884d79997eb3e84e12678b85b12cd026587b8&tochange=ced61ad7cf8f44a792577f96f3513f0c4007d9b9

@Mike can you please take a look at this issue ?

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 698873

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Right, so the issue here is that there's a collision between the "-" character and the "6" character on the keyboard.

See the AZERTY keyboard layout here: https://en.wikipedia.org/wiki/AZERTY#/media/File:KB_France.svg

So effectively, what we have here is a keyboard shortcut collision due to keyboard layout.

Hey masayuki, I'm not sure you're the right person to ask this... do you have any suggestions on how we could try to avoid this shortcut collision? For example, is it possible for us to detect the keyboard layout at runtime to special case the AZERTY layout?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If the zoom level reaches the boundaries, the <command> elements are disabled. If the command element itself is disabled, the default of the key is not prevented.

If the <command> is conditionally disabled, I think the shortcut keys should keep consuming the events. However, I have no idea how to check that and it's not realistic that check all existing shortcut keys.

I think that GlobalKeyListener should stop matching shortcut keys with ignoring a modifier state if there is a command which matches exactly.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, but if we fix bug 1530458 as same as Chrome, Ctrl - Key6 will work as Ctrl - 6. Anyway, my current patch for this bug is not conflict with the bug.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1833494 - Make `GlobalKeyListener` not try to match handler with keyboard event with ignoring `Shift` state after one handler matches with the key combination r=smaug!,#dom-core

GlobalKeyListener try to match shortcut keys with exactly checking Shift
state, but for the cases which do not match localized language and active
keyboard layout location, it scans another shortcut keys with ignoring
Shift state if no handler is available.

<command> elements and <key> elements may be disabled conditionally.
E.g., Zoom-in/Zoom-out are disabled when current zoom level is max/min value.
In this case, it's odd that another shortcut key which does not exactly match
the modifiers works.

Therefore, this patch makes GlobalKeyListener does not try to scan handlers
with ignoring Shift state if it has already found a disabled handler which
exactly matches with the modifiers.

Comment 11

[Dianna Smith [:diannaS]]

:masayuki any update on landing this patch? Will it ride the fx118 train?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, but the patch still needs some updates.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29f07ef75ebf
Make `GlobalKeyListener` not try to match handler with keyboard event with ignoring `Shift` state after one handler matches with the key combination r=smaug

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/29f07ef75ebf

Comment 15

[Rares Doghi, Desktop QA]

This issue is Verified as fixed in our latest Nightly 118.0a1 (2023-08-09).