Add <Focus> component to react-router-dom #6449

pshrmn · 2018-11-02T02:08:01Z

This PR should make it easier to make react-router-dom apps more accessible. This is pretty much a copy of the same component that I use for Curi and I think that this approach works well.

Focus uses a render-invoked prop to assign a ref to the component that should be focused when the location changes

import { Focus } from "react-router-dom";

<BrowserRouter>
  <Focus>
    {ref => (
      <main tabIndex={-1} ref={ref}>
        <Switch>...</Switch>
      </main>
    )}
  </Focus>
</BrowserRouter>

// or even pass the ref to route components (using React.forwardRef()/innerRef)

<BrowserRouter>
  <Focus>
    {ref => (
      <Switch>
        <Route
          exact
          path="/"
          render={match => <Home ref={ref} {...match} />}
        />
        <Route
          exact
          path="/about"
          render={match => <About ref={ref} {...match} />}
          />
      </Switch>
    )}
  </Focus>
</BrowserRouter>

The ref component is only re-focused when the location changes, so non-location change re-renders will not trigger this.

TrySound · 2018-11-02T14:06:55Z

@timdorr I don't think it's necessary. Files entries are deprecated. There is not reason to add the new one.

timdorr · 2018-11-02T14:15:10Z

Uh, I didn't hit submit on those review comments. refined-github goofed 😖

pshrmn · 2018-11-02T14:29:51Z

packages/react-router-dom/modules/Focus.js

+
+  componentDidUpdate(prevProps) {
+    // only re-focus when the location changes
+    if (this.props.location !== prevProps.location) {


I can't find any great documentation for how focus should be handled when clicking a hash link. This could potentially take a function prop that lets the user do a more refined comparison.

<Focus compare={(next, prev) => { // only re-focus when the pathname changes return next.pathname !== prev.pathname; }} >...</Focus>

The best behavior would probably be to always re-focus and attach the ref to the component with the matching id. That requires a much more complex setup (conditionally attaching the ref to components for location's with a hash with a fallback to the "main" content when there is no hash), so I'm not sure how many people would actually go that far.

This is the tricky part of this implementation IMO. I've never really considered object equality a good way to compare two location objects, so I always use locationsAreEqual. Having said that, this is a common thing that people do, so we should probably make the location object immutable between renders if it hasn't changed. Also, the locationsAreEqual function compares location.state, which I don't believe should trigger a refocus.

WRT the compare function, it feels like we should be able to pick the right thing to do based on what browsers currently do so we won't need to provide that hook. But I'm not 100% sure.

I think that a reference check is okay here because it is the object emitted by history, but I'd be fine using locationsAreEqual instead.

I did a quick check of a static page and if you focus an element, then click a link, the focus is moved back to document.body*. Maybe to start, <Focus> should re-focus for every location change. Down the road if people need more fine grained control, it would be easy enough to add the functionality.

* I just realized that I navigate with the mouse, which might have different behavior from a keyboard user. The site I was using doesn't show hash links to keyboard users (...), so I will need to find another site to verify the behavior.

mjackson

Thanks for the PR! This looks great, just had a few questions.

A couple other thoughts:

Let's also work up an example (doesn't need to be in this PR, could come later) that shows people how to do this in their app
Does it also make sense to provide a way to auto-focus the selected <Route> in a <Switch>? Something like a <Switch autoFocus> prop?

packages/react-router-dom/modules/__tests__/Focus-test.js

mjackson · 2018-11-02T21:36:05Z

packages/react-router-dom/modules/Focus.js

+    {context => <FocusWithLocation location={context.location} {...props} />}
+  </RouterContext.Consumer>
+);
+


Should we add some propTypes to the <Focus> component here, like we do elsewhere?

I'm kinda on the fence about propTypes personally these days, since so much of the community is moving away from them and they just increase our bundle size. But I think at least for 4.x we should keep using them on public API.

You can build your modules with a Babel plugin that strips prop types in production mode. That's what I do for RNWeb

I meant to do this. This code was adapted from TypeScript, so I stripped out all of type references and then completely forgot to add prop types.

I would not miss prop types.

Thanks for the tip, @necolas :) We used to use that plugin, but we use the __DEV__ flag now instead, just so it's really clear when looking at the code which stuff is included in dev and which stuff isn't.

When I mentioned bundle size, what I meant was that I'm not sure if our prop-types import is able to be dropped in our production builds, because it isn't conditional. We aren't actually using prop types in our production code, so in theory it should be tree-shaked out of our production bundles, but we have a brand new bundling process in 4.4 (thx to @TrySound) and I still need to verify.

packages/react-router-dom/modules/Focus.js

mjackson · 2018-11-02T21:43:58Z

packages/react-router-dom/modules/Focus.js

+
+  componentDidUpdate(prevProps) {
+    // only re-focus when the location changes
+    if (this.props.location !== prevProps.location) {


This is the tricky part of this implementation IMO. I've never really considered object equality a good way to compare two location objects, so I always use locationsAreEqual. Having said that, this is a common thing that people do, so we should probably make the location object immutable between renders if it hasn't changed. Also, the locationsAreEqual function compares location.state, which I don't believe should trigger a refocus.

WRT the compare function, it feels like we should be able to pick the right thing to do based on what browsers currently do so we won't need to provide that hook. But I'm not 100% sure.

Robdel12 · 2018-11-02T22:05:41Z

This is interesting! 😃 🎉 I don't quite have the time at the moment to do a full review but I wrote a big 'ol document about our experience making a React app with RR v4 accessible: https://github.com/folio-org/stripes-components/blob/master/docs/patterns/AccessibleRouting.md

We tried a couple of different methods, one of them looks similar to what the <Focus /> component here is doing. I hope to come back to this sometime over the weekend!

mjackson · 2018-11-02T22:34:48Z

A review of this PR from you would be invaluable, @Robdel12. If you can spare the time, I'd really appreciate it :)

mjackson · 2018-11-02T23:52:49Z

Just read through that doc, @Robdel12. I like the shouldFocus HOC you came up with.

In your example, the HOC gives you an extra degree of control through the shouldFocus prop. Do you think something like that would be important to include in the <Focus> API we are discussing here? Maybe something like a <Focus when> prop?

With that prop, you could write your HOC like this:

/**
 * Higher order component that, when given the prop
 * `shouldFocus={true}`, will focus the component's DOM node on mount
 * and on update when the prop is toggled from `false` to `true`.
 */
export default function shouldFocus(Focusable) {
  return class extends Component {
    static propTypes = {
      shouldFocus: PropTypes.bool
    };

    render() {
      return (
        <Focus when={this.props.shouldFocus}>
          {ref => (
            <Focusable {...this.props} ref={ref} />
          )}
        </Focus>
      );
    }
  };
}

and use it like this:

const CustomInput = shouldFocus(
  React.forwardRef((props, ref) => (
    <Wrapper {...props}>
      <input ref={ref} />
    </Wrapper>
  ))
);

<CustomInput shouldFocus={true} />

Would that work for you?

Robdel12 · 2018-11-03T00:15:30Z

I should have some free time later tonight / tomorrow morning! 😃

Robdel12 · 2018-11-03T00:16:18Z

@mjackson yes! So much yes :)

pshrmn · 2018-11-03T14:04:36Z

This should also be fairly straightforward to implement as a hook eventually.

function App() {
  const ref = useFocus({ preventScroll: true });
  return (
    <main tabIndex={-1} ref={ref}>
      {/* ... */}
    </main>
  );
}

packages/react-router-dom/docs/api/Focus.md

stowball · 2018-11-04T17:29:57Z

Commented on outline: none implications

pshrmn · 2018-11-04T17:42:07Z

@stowball commented where?

packages/react-router-dom/docs/api/Focus.md

stowball

Much clearer, but perhaps you also need to clarify that it's for non-interactive elements in case someone focuses an a, button or input?

pshrmn · 2018-11-04T21:16:22Z

@stowball you mean just for the outline: none style, correct? Not that the user necessarily should attach the ref to a interactive element, but you don't see an accessibility issue if someone were to do this:

<Focus>
  {ref => (
    <input ref={ref} />
  )}
</Focus>

stowball · 2018-11-04T21:25:06Z

@pshrmn yeah just for the outline note. You and I know it's not a good thing to focus an input directly, but Google do it when you open a new tab, so i imagine someone will build a UI like that

Robdel12

I like it 😈 I'm curious to see how folks use this out in the wild. There might be a little iteration but this is solid.

mjackson · 2018-11-05T19:03:19Z

packages/react-router-dom/docs/api/Focus.md

+
+`Focus` uses a render prop to provide a `ref`. The `ref` should be passed to the element that will be focused.
+
+In order for `Focus` to work, the component type for the focused element needs to either be natively focusable (like an `<input>` or a `<button>`) or be given a `tabIndex` of `-1`. If you do not do this, then the document's `<body>` will be focused instead.


If an element isn't natively focusable, could we perhaps do a cloneElement and add the tabIndex="-1" automatically behind the scenes?

I don't think that cloneElement would work with this API because we don't necessarily have access to the React element. We do have a ref to a DOM element, so the attribute could be to the element with ref.setAttribute("tabindex", "-1").

My personal preference would be to require the user to set the tab index. It's a little more work for the dev than RR injecting the attribute, but the ref warning will let them know if there is an issue with their code and it lacks any "magic" (being able to focus an element that shouldn't be focusable).

End of the day, (assuming setting the attribute works like I think it would) I can make the switch if you would prefer.

hellosmithy · 2019-03-01T16:12:47Z

it may be relevant to this discussion that there is an rfc currently being actively discussed for some focus management features to be provided by react-dom:

reactjs/rfcs#104
https://github.com/devongovett/rfcs-1/blob/patch-1/text/2019-focus-management.md

Add <Focus> component to react-router-dom

7b03561

timdorr approved these changes Nov 2, 2018 •

edited

Loading

View reviewed changes

pshrmn commented Nov 2, 2018

View reviewed changes

pshrmn mentioned this pull request Nov 2, 2018

SPA Accessibility - focus not reset when route changed #5210

Closed

mjackson reviewed Nov 2, 2018

View reviewed changes

mjackson added this to the 4.4 milestone Nov 2, 2018

Use suggestions

d097b16

Documentation

fe5dd4d

pshrmn commented Nov 3, 2018

View reviewed changes

packages/react-router-dom/docs/api/Focus.md Show resolved Hide resolved

pshrmn mentioned this pull request Nov 4, 2018

Add useFocus() hook to react-router-dom #6454

Closed

stowball reviewed Nov 4, 2018

View reviewed changes

packages/react-router-dom/docs/api/Focus.md Show resolved Hide resolved

outline: none note

102d84c

stowball reviewed Nov 4, 2018

View reviewed changes

Don't use outline: none on interactive elements

126dc03

Robdel12 approved these changes Nov 5, 2018

View reviewed changes

mjackson reviewed Nov 5, 2018

View reviewed changes

mjackson removed this from the 4.4 milestone Feb 22, 2019

mjackson added this to the 4.5 milestone Feb 22, 2019

mjackson removed this from the 5.1 milestone Mar 26, 2019

pshrmn closed this Jul 3, 2019

pshrmn deleted the focus branch July 3, 2019 14:27

mjackson mentioned this pull request Aug 26, 2019

Roadmap #6885

Closed

23 tasks

lock bot locked as resolved and limited conversation to collaborators Sep 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add <Focus> component to react-router-dom #6449

Add <Focus> component to react-router-dom #6449

pshrmn commented Nov 2, 2018

TrySound commented Nov 2, 2018

timdorr commented Nov 2, 2018

pshrmn Nov 2, 2018 •

edited

Loading

mjackson Nov 2, 2018

pshrmn Nov 2, 2018 •

edited

Loading

mjackson left a comment

mjackson Nov 2, 2018

necolas Nov 2, 2018

pshrmn Nov 2, 2018

mjackson Nov 2, 2018

mjackson Nov 2, 2018

Robdel12 commented Nov 2, 2018

mjackson commented Nov 2, 2018

mjackson commented Nov 2, 2018 •

edited

Loading

Robdel12 commented Nov 3, 2018

Robdel12 commented Nov 3, 2018

pshrmn commented Nov 3, 2018

stowball commented Nov 4, 2018

pshrmn commented Nov 4, 2018

stowball left a comment

pshrmn commented Nov 4, 2018

stowball commented Nov 4, 2018

Robdel12 left a comment

mjackson Nov 5, 2018

pshrmn Nov 5, 2018

hellosmithy commented Mar 1, 2019


		`Focus` uses a render prop to provide a `ref`. The `ref` should be passed to the element that will be focused.

		In order for `Focus` to work, the component type for the focused element needs to either be natively focusable (like an `<input>` or a `<button>`) or be given a `tabIndex` of `-1`. If you do not do this, then the document's `<body>` will be focused instead.

Add <Focus> component to react-router-dom #6449

Add <Focus> component to react-router-dom #6449

Conversation

pshrmn commented Nov 2, 2018

TrySound commented Nov 2, 2018

timdorr commented Nov 2, 2018

pshrmn Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshrmn Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

mjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robdel12 commented Nov 2, 2018

mjackson commented Nov 2, 2018

mjackson commented Nov 2, 2018 • edited Loading

Robdel12 commented Nov 3, 2018

Robdel12 commented Nov 3, 2018

pshrmn commented Nov 3, 2018

stowball commented Nov 4, 2018

pshrmn commented Nov 4, 2018

stowball left a comment

Choose a reason for hiding this comment

pshrmn commented Nov 4, 2018

stowball commented Nov 4, 2018

Robdel12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellosmithy commented Mar 1, 2019

pshrmn Nov 2, 2018 •

edited

Loading

pshrmn Nov 2, 2018 •

edited

Loading

mjackson commented Nov 2, 2018 •

edited

Loading