Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Button] No standard keyboard focus API for buttons #3008

Closed
puzzfuzz opened this issue Jan 21, 2016 · 3 comments
Closed

[Button] No standard keyboard focus API for buttons #3008

puzzfuzz opened this issue Jan 21, 2016 · 3 comments
Assignees
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request
Milestone

Comments

@puzzfuzz
Copy link

I am attempting to build a webapp that relies 100% on synthetic or keyboard navigation. The UX is kind of like that of a video game where a user can pop in and out of menus and use the controller sticks to navigate focus around a bunch of interactive button, menus, etc. I've built my own focus management stack (so not asking for general keyboard / tab-order accessibility here), but I'm finding that the support for either stateful or programmatic focus management of MUI button components is inconsistent at best, and often non-existent.

The EnhancedButton component has a pair of set/removeKeyboardFocus functions that work well for my purposes, however the various higher-order button components that wrap EnhancedButton either hide or obscure this functionality or occasionally partially implement it.

  • EnhancedButton supports setting keyboard focus via props, however it will not unset it and most higher-order buttons rely on simply proxying props down to the EnhancedButton so this doesn't work.
  • IconButton has a setKeyboardFocus method (which delegates to the EnhancedButton it wraps via this.refs) however it does not have a removeKeyboardFunction analog
    • It does expose its wrapped EnhancedButton via refs as "button", so I can get there by doing this.refs['myIconnBtn42'].refs.button.setKeyboardFocus() but this feels pretty wrong
  • RaisedButton has no programmatic support
    • It does expose its wrapped EnhancedButton via refs as "container", so I can do as above, but it's a different ref name, so I have to interrogate the ref chain to see where to make this call
  • FlatButton does not have a programmatic API for this, nor does it expose its wrapped EnhancedButton whatsoever.

It would be great if all the buttons either proxied the set/removeKeyboardFocus functions consistently, or exposed their wrapped EnhancedButton as a ref via a consistent name.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 5, 2017

@puzzfuzz Thanks for opening this issue, with a summarized situation.
@mzamoras Is proposing the following solutions in #6268 (comment)

So as I see there are 4 possible ways:

  • Make a callback ref property for each button, for consistency (breaking current implementations).
  • Give the exact same kind of access to FlatButton ( ref="container").
  • Give the access to FlatButton but a little less odd (this PR).
  • Do nothing but FlatButton can't programmatically be focused as RaisedButton is.

Here is my perspective

  1. A good solution could be to advise users to use the DOM API: findDOMNode. Here is an example:
import React from 'react';
import ReactDOM from 'react-dom';
import FlatButton from 'material-ui/FlatButton';

class MyComponent extends React.Component {
  componentDidMount() {
    ReactDOM.findDOMNode(this.button).focus(); // Set the correct `document.activeElement`
  }

  render() {
    return <FlatButton ref={(node) => this.button = node } label="label" />;
  }
}


export default MyComponent;
  1. But we have to go down the DOM implementation and components might not have a focusable element as their root. For instance, the RaisedButton is wrapped into a div. So, exposing a focus method on the instances could be another good solution as providing a good abstraction.

I think that we should be supporting 2. for the next branch.

Regarding setKeyboardFocus, it's definitely an implementation detail as we are speaking. I don't think that we should be exposing it. It's only setting the focus UI state, not the actual focus.

@mzamoras I'm closing #6268 until we agree on the right path going forward.

@mzamoras
Copy link

mzamoras commented Mar 6, 2017

@oliviertassinari I understand your point but:

  1. Shouldn't you remove the current access provided to Raised Button? If enhanced button is not meant to be exposed, why keep it there?
  2. Focusing the element doesn't provide any visual feedback (ripple feedback), it's only visible after calling setKeyboardFocus().

I know you are focusing your efforts on the next development, but right now it'll be helpful to grant the same level of access for buttons either remove it or enable it for both.

Or if you could suggest me a way to programmatically achieve the same result as pressing tab key to reach a button.

I just want to load a view and automatically focus a FlatButton, including the ripple effect and everything ... that would be amazingly helpful, which by the way is possible only with RaisedButton.

@oliviertassinari
Copy link
Member

If enhanced button is not meant to be exposed, why keep it there?

I guess it's used internally by the component.

Focusing the element doesn't provide any visual feedback

That's true, the visual feedback is only added when the focus is acquired through a keyboard interaction. That's by design.
We have some other options:

  1. We could remove that focus distinction between a keyboard/no keyboard focus. But I don't think that it would work as we acquire the focus when clicking on the button.
  2. Exposing a method that programmatically behaves as the keyboardFocused property sounds like a much better option.

I just want to load a view and automatically focus a FlatButton, including the ripple effect and everything ... that would be amazingly helpful, which by the way is possible only with RaisedButton.

You have the keyboardFocused property for that use case.

@oliviertassinari oliviertassinari added new feature New feature or request and removed accessibility a11y labels Jul 28, 2017
@oliviertassinari oliviertassinari changed the title No standard keyboard focus API for buttons. No standard keyboard focus API for buttons Jul 28, 2017
@oliviertassinari oliviertassinari added the component: button This is the name of the generic UI component, not the React module! label Jul 28, 2017
@oliviertassinari oliviertassinari changed the title No standard keyboard focus API for buttons [Button] No standard keyboard focus API for buttons Jul 28, 2017
@oliviertassinari oliviertassinari added this to the post v1 milestone Feb 11, 2018
@oliviertassinari oliviertassinari self-assigned this May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants