mui / base-ui

Base UI is an open-source library of accessible, unstyled UI components for React.
https://mui.com/base-ui/
MIT License
259 stars 44 forks source link

[button] Doesn't set active state to true when Enter is pressed #335

Closed brianle1301 closed 5 months ago

brianle1301 commented 6 months ago

Steps to reproduce

Link to live example: (required)

Steps:

  1. Go to the MUI Base doc
  2. Tab to a MUI Base button
  3. Press Enter

Current behavior

Only Space triggers a button, not Enter

Expected behavior

Space and Enter should both trigger a button as guided by WAI-ARIA here

Context

No response

Your environment

npx @mui/envinfo ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```

Search keywords: button, enter, trigger, activate

michaldudak commented 6 months ago

I couldn't reproduce the error. Pressing Enter when a button is focused does trigger it.

brianle1301 commented 6 months ago

@michaldudak It does? Doesn't seem to trigger, at least not the CSS animation for me. I will take another look. In case it helps, this is my browser:

Microsoft Edge
Version 124.0.2478.51 (Official build) (64-bit)
brianle1301 commented 6 months ago

Looking at useButton this caught my attention:

 const createHandleKeyDown =
    (otherHandlers: EventHandlers) => (event: React.KeyboardEvent & MuiCancellableEvent) => {
      otherHandlers.onKeyDown?.(event);

      if (event.defaultMuiPrevented) {
        return;
      }

      if (event.target === event.currentTarget && !isNativeButton() && event.key === ' ') {
        event.preventDefault();
      }

      if (event.target === event.currentTarget && event.key === ' ' && !disabled) {
        setActive(true);
      }

      // Keyboard accessibility for non interactive elements
      if (
        event.target === event.currentTarget &&
        !isNativeButton() &&
        event.key === 'Enter' &&
        !disabled
      ) {
        otherHandlers.onClick?.(event);
        event.preventDefault();
      }
    };

From the looks of it, pressing Space triggers the button's active state, but not onClick? Alternatively, pressing Enter triggers the onClick handler, but doesn't set the active state?

The buttons displayed on the doc site definitely doesn't show the active styling when Enter is pressed 🤔

michaldudak commented 6 months ago

Just to clarify - is the issue about not triggering the onClick action or not applying the :active state when Enter is pressed?

Native HTML buttons don't apply the :active state when Enter is pressed (see https://jsbin.com/huharifoji/edit?html,css,output).

From the looks of it, pressing Space triggers the button's active state, but not onClick

This is for non-interactive elements (for example using span to act as a button). Space triggers onClick when it's released, not when it's pressed.

brianle1301 commented 6 months ago

@michaldudak I'd like to refocus the issue on the :active state instead. I initially though that, because :active state did not apply when pressing Enter, the button is not activated.

Your explanation made sense and I do see how Mui Base aligns with native HTML standards. From the user's perspective, I would expect that the button appears "activated" when pressing either Enter or Space. I can, of course, apply the active state from my own code when Enter is triggered, but I feel like this can be done from Mui's end to improve DX.

On the other note, I would like to know why Space triggers onClick when it's key up and Enter triggers on keydown?

michaldudak commented 5 months ago

We decided not to patch such inconsistencies between browsers, but we didn't consider this exact case when discussing this. @colmtuite, do you have an opinion on this?

On the other note, I would like to know why Space triggers onClick when it's key up and Enter triggers on keydown?

Frankly, I have no idea 🤷

colmtuite commented 5 months ago

Yep this is just how the native button works, so not really a Base UI issue imo.

The spec is open to interpretation on this imo. It's not clear on how it should work. It says "The :active pseudo-class applies while an element is being activated by the user". You could argue that "being activated" means while its pressed but before it's actually activated.

That's fine when you activate the button using space or a pointer, because there is an intermediate state while the button is pressed but not activated. But because Enter triggers on keydown, there is not intermediate state. So it could make sense that the :active state is never applied.

In any case, as @michaldudak mentioned, we have decided (for now at least) to not patch these cross-browser/platform inconsistencies while we focus on much larger issues on our alpha redesign. So I'll close this and we might revisit it next year.

oliviertassinari commented 5 months ago

A benchmark:

https://github.com/mui/base-ui/assets/3165635/3e6477fe-361a-491d-bcd3-5fae12e5e9ae

https://material-web.dev/components/button/#types

But it's true, that today, it's not how Chromes behave, Enter doesn't trigger :active.

A related discussion: https://github.com/w3c/csswg-drafts/issues/4787.