airbnb / rheostat

Rheostat is a www, mobile, and accessible slider component built with React
MIT License
1.69k stars 189 forks source link

Implements autoFocus #272

Open cassiozen opened 4 years ago

cassiozen commented 4 years ago

AutoFocus is an important accessibility issue. Currently, there is no way to implement in Rheostat - the handle ref is not forwarded and because custom handles are provided as render functions, overriding their ref is not easy.

This PR implements an autoFocus prop, consistent with the property name found in HTML input fields.

If multiple handles are used, focus is given to the first one.

cassiozen commented 4 years ago

can you elaborate on your use case for autofocusing?

Sure! It's important for people that use keyboard for navigation. In our case, the slider only appears after a click on the button:

ljharb commented 4 years ago

Is there a reason not to make part of the button's onClick logic be, "focus on the handle"?

cassiozen commented 4 years ago

Is there a reason not to make part of the button's onClick logic be, "focus on the handle"?

It could, but rheostat would then need to forward the refs to the handles. Since autoFocus works when the element is added to the DOM, even if something previously also had an autoFocus property, this looked simpler.

ljharb commented 4 years ago

That seems like you'll be relying on the implementation detail that that slider isn't in the DOM until it's visible/usable, which isn't guaranteed to stay that way.

cassiozen commented 4 years ago

Sorry, I should have been more clear - what I meant to say is that it works in ANY case - whether its dynamically added to the DOM or not. In both cases, the end result is that the element is focused (which is the desired effect).

cassiozen commented 4 years ago

Summarizing, autoFocus is an important feature for accessibility (and also for keyboard-heavy users) - so much so that it is a standard attribute in all form elements.

ljharb commented 4 years ago

Something being standard in no way demonstrates that it's important for accessibility, only that at one time some folks thought it was :-)

cc @backwardok to weigh in with a11y expertise

backwardok commented 4 years ago

I think there are legitimate cases where you would want something to focus on load, but it should be used with caution.

I find what may be the best thing to focus for some keyboard users may not be the best thing to focus for a screen reader user (or other kind of assistive technology) or even all keyboard users. For example, say you have a modal that has a title, some content that explains what this modal is for, and then some fields to enter. If the content is required to be read in order to know what the fields are for, that's context you'd want all users to have access to prior to accessing the fields. There could be cases where it seems like, for ease of quick response, that moving focus there automatically is preferred, since that would reduce the amount of keystrokes a keyboard user needs to do. But, if the person were using a screen reader or if they were using screen magnification, moving straight to the field may result in them missing that important context and forcing them to navigate backwards, and then forwards again, to continue forward. A similar case can happen in mobile browser contexts, where focusing an element results in the context getting scrolled past.

Bruce Lawson has a related writeup that would be good to read about this topic. The A11y Project also has a small snippet that goes into why autofocus isn't the best pattern. The jsx-a11y project also defaults to disabling that attribute by default after it was highlighted on Twitter as being problematic. It's worth pointing out, though, that in the current state of the WHATWG HTML spec that Steve Faulkner was referring to has removed that warning.

cassiozen commented 4 years ago

It seems to me then that we agree on the premise (programmatically setting focus IS important/desirable from the perspective of a11y) but we disagree on the API.

I’m interested in working on a different solution (maybe forwarding the handle ref), but only if you also find it useful, @ljharb. What are your thoughts?

ljharb commented 4 years ago

Forwarding the handle ref seems like a blunt instrument; i'd prefer to come up with some kind of api that's restricted to focusing on the handle on demand, but I don't know off the top of my head what that would look like.

cassiozen commented 4 years ago

AFAIK ref is the preferred way to deal with imperative APIs in React, but I agree that giving access to the handle's ref might be too much. Maybe exposing a focus method on the Rheostat's ref?

Would look something like this:

const MyComponent = () => {
  const sliderEl = useRef(null);
  useEffect(() => {
      sliderEl.current.focus();
  });
  return <Slider ref={sliderEl} />
}
ljharb commented 4 years ago

Also note it would need to be done without hooks and without React.createRef or React.forwardRef.