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

Implements autoFocus #272

Open
wants to merge 3 commits into
base: master
from
Open

Implements autoFocus #272

wants to merge 3 commits into from

Conversation

@cassiozen
Copy link

cassiozen commented Jan 2, 2020

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.

Copy link
Collaborator

ljharb left a comment

Please add some tests (not just a story).

This change looks good overall, but can you elaborate on your use case for autofocusing?

@cassiozen

This comment has been minimized.

Copy link
Author

cassiozen commented Jan 3, 2020

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:

  • The user already "tabbed" their way to the button element. When the button is activated and the slider appears, we want the focus to immediately move to the handle.
@cassiozen cassiozen requested a review from ljharb Jan 6, 2020
@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jan 7, 2020

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


it('should set focus on first handle when multiple handles are available', () => {
const wrapper = mount(<Slider values={[0, 50, 100]} autoFocus />);
const focusedElement = document.activeElement.outerHTML;

This comment has been minimized.

Copy link
@ljharb

ljharb Jan 7, 2020

Collaborator

rather than using outerHTML in these tests, could we put an attribute on the element, and compare that?

This comment has been minimized.

Copy link
@cassiozen

cassiozen Jan 7, 2020

Author

JSDom doesn't expose custom attributes (Last time I checked). It does forward the ID, but it wouldn't work on the multiple handles test...

This comment has been minimized.

Copy link
@ljharb

ljharb Jan 8, 2020

Collaborator

even data attributes?

This comment has been minimized.

Copy link
@cassiozen

cassiozen Jan 8, 2020

Author

Wrote a custom handle that will generate random IDs to use on the tests (instead of comparing the full generated HTMLs).

@cassiozen

This comment has been minimized.

Copy link
Author

cassiozen commented Jan 7, 2020

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jan 8, 2020

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

This comment has been minimized.

Copy link
Author

cassiozen commented Jan 8, 2020

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 cassiozen force-pushed the cassiozen:master branch from d830690 to 1e35d68 Jan 8, 2020
@cassiozen cassiozen requested a review from ljharb Jan 8, 2020
@cassiozen

This comment has been minimized.

Copy link
Author

cassiozen commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Member

backwardok commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Author

cassiozen commented Jan 17, 2020

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jan 17, 2020

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

This comment has been minimized.

Copy link
Author

cassiozen commented Jan 17, 2020

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jan 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.