1921772 - Centralize enter/space keypress handling of toolbar buttons (gClickAndHoldListener, XULButtonElement, custom elements, toolbarKeyNav) into button.js or comparable toolkit base JS layer

Status: NEW

(Firefox :: Keyboard Navigation, task)

Description

[:Gijs (he/him)]

Over in bug 1920319 an existing test started failing on beta only, only on macOS. The test tested that hitting "Enter" while focusing the new tab toolbar button actually opened a new tab. It worked on nightly and non-macOS.

This was because it turned out the keypress handling for "Enter" was only present by virtue of the new tab button being in the gClickAndHoldListeners group (together with back/fwd buttons) which was calling button.click() in response to space or enter keypresses. But we only add the new tab button to that group when containers are enabled, which they are by default on Nightly but not beta/release.

In the tabstrip things work by virtue of https://searchfox.org/mozilla-central/rev/9fe73403523732f57cd82d30590ecc272fb0b165/browser/base/content/browser-toolbarKeyNav.js#408-415 .

Between that and the fact that https://searchfox.org/mozilla-central/rev/9fe73403523732f57cd82d30590ecc272fb0b165/toolkit/content/widgets/button.js#27-34 handles space but not enter for all buttons, and the fact that it kept working on windows/linux (haven't yet checked why), and that the toolbar code handles modifiers but gClickAndHoldListeners doesn't, this all feels very messy.

We should figure out a more rational way of dealing with this.

(Marking as task as this is basically refactoring, but arguably the modifier bits are actually buggy so "defect" wouldn't be inappropriate either)

Comment 1

[:Gijs (he/him)]

Oh, and one last piece of the puzzle I couldn't check earlier because I wasn't at my Windows machine: on Windows and Linux this didn't break because of this delightful bit of C++ code, which is ifdef'd out on macOS:

https://searchfox.org/mozilla-central/rev/9fe73403523732f57cd82d30590ecc272fb0b165/dom/xul/XULButtonElement.cpp#563-579

// On mac, Return fires the default button, not the focused one.
#ifndef XP_MACOSX
    case eKeyPress: {
      WidgetKeyboardEvent* keyEvent = event->AsKeyboardEvent();
      if (!keyEvent) {
        break;
      }
      if (NS_VK_RETURN == keyEvent->mKeyCode) {
        if (RefPtr<nsIDOMXULButtonElement> button = AsXULButton()) {
          if (OnPointerClicked(*keyEvent)) {
            aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
          }
        }
      }
      break;
    }
#endif

Comment 2

[:Gijs (he/him)]

Off hand my thinking here is that we should drop the XUL C++ bits in favour of JS handlers. So I'd be tempted to change the button.js keypress handler to also run a click event when:

1. "enter" is pressed on non-mac
2. "enter" is pressed on mac for toolbarbuttons (rather than other buttons)
3. ensure we pass modifier keys to the click event there.

AIUI that should allow removing a bunch of bespoke keypress handling (including what we just added in bug 1920319, but probably also the gClickAndHoldListeners keypress handling, plus perhaps some of the toolbar keynav bits. I don't know how much it'd break...

Emilio/Jamie, any reasons that's a bad idea that I'm missing?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

No, in general moving as much as possible the event handling to toolkit makes sense. But note that:

• That bit of code you pointed at fires also a command event etc.
• PostHandleEvent is a subtly different timing from what a regular event listener would do.

But those seem feasible to make work, specially if there are already other key handlers around.

Comment 4

[James Teh [:Jamie]]

Overall, this makes sense to me. One question, though:

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

2. "enter" is pressed on mac for toolbarbuttons (rather than other buttons)

Why do we want to do this? I believe (correct me if I'm wrong) that the OS convention on Mac is that enter shouldn't activate buttons. Why are toolbarbuttons special? Or does Mac native differentiate toolbar buttons too?

I know we probably have various bits of code scattered around for various toolbarbuttons which handle both enter and space regardless of OS. To be clear, I don't object to enter activating buttons - the Mac convention has always kinda confused me - but if we're codifying this in toolkit, I'd like to understand the rationale here.

Comment 5

[:Gijs (he/him)]

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

Overall, this makes sense to me. One question, though:

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

2. "enter" is pressed on mac for toolbarbuttons (rather than other buttons)

Why do we want to do this? I believe (correct me if I'm wrong) that the OS convention on Mac is that enter shouldn't activate buttons. Why are toolbarbuttons special? Or does Mac native differentiate toolbar buttons too?

I haven't been able to focus any mac native toolbar buttons to try (even after enabling full keyboard access). Do you know of an example Apple-produced mac app that supports this?

(if this is impossible I suppose there's an argument that there's just no macOS convention and thus we can make up our own, though that seems exceedingly feeble/specious as rationales go)

Mostly I am suggesting that we do this because it's what the current toolbar navigation code (that, I think, you wrote?) does :-)
Given this is effectively a technical refactor I'm inclined to avoid rocking the boat and thus keeping the status quo as much as is reasonable. And although this may be macOS convention, people coming over from Linux and Windows do expect it to work and are occasionally surprised when it doesn't (cf. bug 1505748).

I know we probably have various bits of code scattered around for various toolbarbuttons which handle both enter and space regardless of OS. To be clear, I don't object to enter activating buttons - the Mac convention has always kinda confused me - but if we're codifying this in toolkit, I'd like to understand the rationale here.

It sounds like we're probably mostly violently agreeing here, as it were - I'm only erring on the side of "oh well this is what we're already doing in the toolbar, and if we're trying to rely on the generic code then we must either make that code do the same or accept a change in functionality, and the former seems less work and unlikely to upset anyone".

Does that still sound reasonable? I could easily be convinced to do the opposite. :-)

Comment 6

[James Teh [:Jamie]]

Sounds reasonable to me. Thank you.