1682621 - "Clicking" an add-on toolbarbutton with the space key executes the action twice

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Markus Stange [:mstange]]

Steps to reproduce:

1. Install the "Measure It" add-on.
2. Go to a regular web page, for example this bugzilla bug. (not addons.mozilla.org)
3. Focus the add-on's toolbarbutton with the keyboard.
4. Press the space key.

Expected results:
The page should dim and the mouse cursor should turn into the cross-hair cursor.

Actual results:
The page briefly flashes dimmed but returns back to the non-dimmed state immediately.

This is happening because add-on buttons use a "view" widget even if they don't have a popup, and the button's command is activated from onViewShowing. "View" buttons handle both keypress and command, so we "show the view" twice: When the space key is pressed, the we show it from the keypress listener, and when the space key is released, we "show the view" again from the command event listener (because the toolbarbutton fires command on space-key-keyup).
This double-activation does not happen when the Enter key is pressed, because Enter fires the command event on keypress rather than keyup, so the preventDefault() in the keypress listener prevents the double-firing.

I think we can remove the keypress listener for "view" widgets. The command event will still fire and open the view normally. On macOS, Enter does not fire command, so views will no longer open on Enter. This is consistent with other toolbarbuttons which handle command, e.g. the sidebar button or the refresh button - those buttons also don't activate on Enter on macOS. Interestingly, there are some buttons which do react to Enter on macOS, e.g. the back button or the home button: Those buttons handle click instead of command, and we have JS code which fires a synthetic click event when Enter is pressed.

Comment 1

[:Gijs (he/him)]

I think based on https://phabricator.services.mozilla.com/D11608#287705 that the addition of the keypress handler was deliberate, and that removing it would regress focus behaviour. Did you see this already?

Comment 2

[Markus Stange [:mstange]]

I had not seen that, thanks for pointing it out! I will double check if we in fact treat keyboard activation differently from mouse activation here.

Comment 3

[Markus Stange [:mstange]]

Indeed, the difference is handled here: https://searchfox.org/mozilla-central/rev/8883276967d39918e2ce64e873afdd432fb406ca/browser/components/customizableui/PanelMultiView.jsm#571-576

That makes fixing this less straightforward than I initially assumed, so I'm going to stop my investigation here.

Comment 4

[:Gijs (he/him)]

Jamie, when you're back, I wonder if you have an idea about what we could do here. It's unfortunate the keyboard support doesn't work well for extension buttons...

Comment 5

[James Teh [:Jamie]]

Maybe we could avoid registering a command listener and instead call this.handleWidgetCommand from handleWidgetClick for widgets with type "view"? I'm not sure what currently happens if a button/view widget defines both onCommand and onClick functions, though. Does onCommand somehow prevent onClick from being called or vice versa? or do they get called in a specific order? We'd need to emulate the existing behaviour in handleWidgetClick.

Alternatively, we could register a keyup listener for widgets with type "view" which prevents default for space. Yuck... but maybe less risky? Assuming preventDefault actually prevents the command event in this case? I can't remember.

Comment 6

[Markus Stange [:mstange]]

Another option might be to annotate command events with a flag that says whether the event was triggered by the keyboard. Then we would only need to handle command events and no key events, and we could still treat keyboard activations differently.

Comment 7

[James Teh [:Jamie]]

Access-s2 because this could prevent keyboard users from properly using certain add-ons.

Comment 8

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

The severity field for this bug is set to S4. However, the accessibility severity is higher, [access-s2].
:Gijs, could you consider increasing the severity?

For more information, please visit auto_nag documentation.

Comment 9

[:Gijs (he/him)]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #8)

The severity field for this bug is set to S4. However, the accessibility severity is higher, [access-s2].
:Gijs, could you consider increasing the severity?

For more information, please visit auto_nag documentation.

S2 is basically "we must fix this as soon as possible". Given this has now lingered for well over a year, I am not sure that's accurate. Jamie?

Comment 10

[James Teh [:Jamie]]

The severity/priority distinction is always messy, but at the end of the day, this prevents keyboard users from accessing certain add-ons. For the sake of argument, let's pretend that this affected all mouse users; i.e. that clicking with the mouse didn't work here in the same way that keyboard activation fails. I imagine that'd be an s2. On the other hand, if we have sufficient reason to think that this only affects a very small number of add-ons that are never/rarely used, maybe that would be grounds to drop it to s3. If you would triage this as s3 even if it affected mouse users, then I guess I'm happy with dropping the access rating also.

Comment 11

[James Teh [:Jamie]]

As an addendum, "we haven't gotten to this yet due to lack of resourcing" isn't necessarily the same as "this doesn't completely break important use cases for some users".

Comment 12

[:Gijs (he/him)]

(In reply to James Teh [:Jamie] from comment #10)

The severity/priority distinction is always messy, but at the end of the day, this prevents keyboard users from accessing certain add-ons. For the sake of argument, let's pretend that this affected all mouse users; i.e. that clicking with the mouse didn't work here in the same way that keyboard activation fails. I imagine that'd be an s2. On the other hand, if we have sufficient reason to think that this only affects a very small number of add-ons that are never/rarely used, maybe that would be grounds to drop it to s3. If you would triage this as s3 even if it affected mouse users, then I guess I'm happy with dropping the access rating also.

I think this is a complex question. The hypothetical unfortunately immediately falls down because the add-on in comment 0 is only usable by mouse users (ie even if we fixed this bug and you activated the toolbar button with the keyboard, you can then click 2 points on the page to measure the distance between them - but there is no way to do it with the keyboard, AFAICT). This bug only affects add-ons that:

0. are used with space for activation, rather than enter (though it's worth noting enter doesn't work for activation on macOS, it works on Windows and I expect on Linux, too)
1. don't open a popup/view
2. also have state / do one-off things where doing them twice causes problems. For instance, the only add-on I have that adds a toolbar button without a view/popup, taburdle, copies the state of your tabs to the clipboard and shows a desktop notification. It appears to work fine when activated by the keyboard (and I also only see 1 desktop notification on macOS, though I dunno if that's because the add-on somehow doesn't hit this bug, or if there's throttling in the add-on / Firefox / macOS for the desktop notification).
3. the add-on doesn't provide a keyboard shortcut or other more accessible way of doing what it's doing - keeping in mind that the toolbar a11y is relatively new, we've therefore always recommended add-ons provide shortcuts for keyboard a11y to their add-on UI, and those shortcuts are user-customizable, and more user-friendly than tabbing to the right toolbar button. Older and more popular add-ons would hopefully have keyboard shortcuts.

It isn't easy to determine how many add-ons fit this bill. But as a proxy, it took nearly 2 years from when the behaviour was implemented for this bug to even be discovered. As a result I am skeptical that it is frequently encountered and warrants being S2. Does that make sense?

Part of me thinks that although investigating the fix might be a bit of work, the patch could be relatively simple, so maybe we're better off just fixing it rather than going back and forth over the severity. But I'm slowly learning that even relatively "simple" patches aren't always as simple as they seem in terms of the repercussions they end up having. So I'm not sure what to do, really! If we really think this is S2 then we will add it to our quality foundation backlog and it will get fixed sooner. But I'm not convinced that's warranted.

Comment 13

[James Teh [:Jamie]]

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

3. the add-on doesn't provide a keyboard shortcut or other more accessible way of doing what it's doing - keeping in mind that the toolbar a11y is relatively new, we've therefore always recommended add-ons provide shortcuts for keyboard a11y to their add-on UI, and those shortcuts are user-customizable, and more user-friendly than tabbing to the right toolbar button.

A dedicated keyboard shortcut would certainly constitute a workaround and thus reduce the severity if we could be certain that all affected add-ons provide one. That said, keyboard shortcuts being more user friendly depends on what dimension of friendliness you're talking about. While keyboard shortcuts are more efficient, they are also a lot less discoverable, and potentially less memorable if a user has an add-on they don't use often enough. This is a major reason the a11y team prioritised the implementation of toolbar keyboard navigation.

But as a proxy, it took nearly 2 years from when the behaviour was implemented for this bug to even be discovered. As a result I am skeptical that it is frequently encountered and warrants being S2. Does that make sense?

It does. On the other hand, it's hard to know whether this is a satisfactory proxy. I don't have data, but I would guess only a small proportion of our users actually report bugs here. Keyboard only users are already a subset of our users, so that's a smaller proportion still. Toolbar keyboard navigation is a really good example of this. We had enough of a sense that users were unhappy about the lack of toolbar keyboard navigation that we prioritised it above other work (despite being primarily a platform team), but I don't think much (if any) of that sentiment was learned from Bugzilla.

I'm slowly learning that even relatively "simple" patches aren't always as simple as they seem in terms of the repercussions they end up having.

I sooo hear you. :(

So I'm not sure what to do, really! If we really think this is S2 then we will add it to our quality foundation backlog and it will get fixed sooner. But I'm not convinced that's warranted.

For me, this is right on the line. I still think it's an access-s2 because there might be no workaround in some cases and because keyboard shortcuts are not an ideal workaround in others. That said, I can't give you specific examples of add-ons where this actually causes a real problem, so perhaps that makes it an s3.

Comment 14

[:Gijs (he/him)]

(In reply to James Teh [:Jamie] from comment #13)

For me, this is right on the line. I still think it's an access-s2 because there might be no workaround in some cases and because keyboard shortcuts are not an ideal workaround in others. That said, I can't give you specific examples of add-ons where this actually causes a real problem, so perhaps that makes it an s3.

Well, let's err on the side of gut instinct and make this an S2, then.

Comment 15

[Dianna Smith [:diannaS]]

:gijs is there an update on getting this assigned given the severity?

Comment 16

[:Gijs (he/him)]

(In reply to Dianna Smith [:diannaS] from comment #15)

:gijs is there an update on getting this assigned given the severity?

No, this is in the JIRA queue ( https://mozilla-hub.atlassian.net/browse/FIDEFE-2160 ) and that is being dealt with by the engineering team as and when they have capacity, dependent on other priorities.

(Edit: my response was unnecessarily defensive, for which I apologize. I've removed everything but a factual reply to the question.)

Comment 17

[Neil Deakin]

Attachment: Bug 1682621, set input source to MOZ_SOURCE_KEYBOARD for command events on buttons, r=tnikkel

Comment 18

[Neil Deakin]

Attachment: Bug 1682621, remove keypress listener for main toolbar buttons, and use the inputSource to distinguish keyboard events, r=gijs

The virtual click event is also removed. It seems this event is only used to check whether this was a key or mouse event in PanelMultiView.jsm

This also removes support for pressing the enter key to trigger these toolbarbuttons on Mac which shouldn't be happening anyway.

Depends on D149284

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/782fd4871c5f
set input source to MOZ_SOURCE_KEYBOARD for command events on buttons, r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/31a6b027b4e7
remove keypress listener for main toolbar buttons, and use the inputSource to distinguish keyboard events, r=Gijs

Comment 20

[Marian-Vasile Laza]

Backed out 2 changesets (Bug 1682621) for causing bc failures on browser_ext_browserAction_click_types.js.
Backout link
Push with failures
Failure Log

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e32910a4948b
set input source to MOZ_SOURCE_KEYBOARD for command events on buttons, r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/88be5107a4a1
remove keypress listener for main toolbar buttons, and use the inputSource to distinguish keyboard events, r=Gijs

Comment 22

[Norisz Fay [:noriszfay]]

Backed out for causing bc failures on browser_menu_touch.js

Backout link

Push with failures

Failure log

Comment 23

[Neil Deakin]

Attachment: Bug 1682621, the command event can supply an input source so provide it in PendingPopup::MouseInputSource so that it is available during the popupshowing event, r=tnikkel

This is to fix the test failure in browser_menu_touch.js.

Comment 24

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8a9d64524b78
https://hg.mozilla.org/mozilla-central/rev/41c2f75017bf
https://hg.mozilla.org/mozilla-central/rev/5f662264782d