1596215 - A tabbox (for instance, Certificate Manager) should stop "tab navigation" keyboard shortcuts from being consumed by the tabbrowser

Status: NEW

(Toolkit :: UI Widgets, defect, P3)

Description

[Alice0775 White]

Reproducible: always

Steps TO Reproduce:

1. Open several tabs
2. Open about:preferences#privacy
3. Click "View Certificate..." button
   --- observe selected tab of the "Certificate Manager" sub-dialog
4. Ctrl+PageUp to switch previous browser tab
5. Ctrl+PageDown to switch original browser tab
   --- observe selected tab of the "Certificate Manager" sub-dialog

Actual Results:
tab of the "Certificate Manager" sub-dialog and main browser's tab are switched at the same time.

Expected Results:
If browser chrome has focus, tab of the "Certificate Manager" sub-dialog should not be switched.
If "Certificate Manager" sub-dialog has focus, tab of the "Certificate Manager" sub-dialog should be switched. And main browser tab should not be switched.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82d0a583a9a39bf0b0000bccbf6d5c9ec2596bcc&tochange=27c6658b2a9004dd996ce79b35a40bb66ce98feb

Comment 2

[:Gijs (he/him)]

Brian, you moved this to Fx::Preferences, but although this is probably the only place it happens, I expect this is an issue with event listeners for either tabbrowser or for all tabs. It looks like the tabstrip code explicitly ignores whether the event has been consumed ( https://searchfox.org/mozilla-central/rev/01d3f084379bd52eec84f56a89dceb078b93ba41/toolkit/content/widgets/tabbox.xml#127-129 before, now https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/content/widgets/tabbox.js#135-136 ). This has been the case since at least bug 1008772.

I think this is a tabbox bug (so belongs in Toolkit :: XUL Widgets) and it should be fixed by making the handling code there call stopPropagation(). Does that make sense or am I missing something?

Comment 3

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

Any updates here, bgrins?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #2)

Brian, you moved this to Fx::Preferences, but although this is probably the only place it happens, I expect this is an issue with event listeners for either tabbrowser or for all tabs. It looks like the tabstrip code explicitly ignores whether the event has been consumed ( https://searchfox.org/mozilla-central/rev/01d3f084379bd52eec84f56a89dceb078b93ba41/toolkit/content/widgets/tabbox.xml#127-129 before, now https://searchfox.org/mozilla-central/rev/524bed6dfbc5ae21c62632d83b7573448b29e0ac/toolkit/content/widgets/tabbox.js#135-136 ). This has been the case since at least bug 1008772.

I think this is a tabbox bug (so belongs in Toolkit :: XUL Widgets) and it should be fixed by making the handling code there call stopPropagation(). Does that make sense or am I missing something?

To be clear - is the intended behavior here is that the browser changes tabs and the cert manager UI doesn't?

Because I was thinking that the cert manager UI in preferences would receive the event first so it would need to either ignore it if we want the browser to change tabs, or consume it and be in charge of stopping propagation / preventing default if we wanted only the cert manager to change tabs.

Comment 5

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #4)

To be clear - is the intended behavior here is that the browser changes tabs and the cert manager UI doesn't?

I think comment #0 is correct:

Expected Results:

1. If browser chrome has focus, tab of the "Certificate Manager" sub-dialog should not be switched.
2. If "Certificate Manager" sub-dialog has focus, tab of the "Certificate Manager" sub-dialog should be switched. And main browser tab should not be switched.

(1) is already happening. But in the steps in comment #0, we auto-focus into the dialog when it's opened, so the dialog has focus, and then I think it's up to the cert manager tab binding to consume the keypress and stop it from making its way to the browser's tabbox.

Comment 6

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

The priority flag is not set for this bug.
:bgrins, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Brian Grinstead [:bgrins] from comment #4)

To be clear - is the intended behavior here is that the browser changes tabs and the cert manager UI doesn't?

I think comment #0 is correct:

Expected Results:

1. If browser chrome has focus, tab of the "Certificate Manager" sub-dialog should not be switched.
2. If "Certificate Manager" sub-dialog has focus, tab of the "Certificate Manager" sub-dialog should be switched. And main browser tab should not be switched.

(1) is already happening. But in the steps in comment #0, we auto-focus into the dialog when it's opened, so the dialog has focus, and then I think it's up to the cert manager tab binding to consume the keypress and stop it from making its way to the browser's tabbox.

OK, thanks for the explanation. Makes sense - we should update the behavior. Probably somewhere around https://searchfox.org/mozilla-central/rev/053826b10f838f77c27507e5efecc96e34718541/toolkit/content/widgets/tabbox.js#140-159 doing stopPropagation as described in Comment 2.