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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b03561
Add <Focus> component to react-router-dom
pshrmn Nov 2, 2018
d097b16
Use suggestions
pshrmn Nov 3, 2018
fe5dd4d
Documentation
pshrmn Nov 3, 2018
102d84c
outline: none note
pshrmn Nov 4, 2018
126dc03
Don't use outline: none on interactive elements
pshrmn Nov 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions packages/react-router-dom/.size-snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
{
"esm/react-router-dom.js": {
"bundled": 7978,
"minified": 4880,
"gzipped": 1618,
"bundled": 10547,
"minified": 6376,
"gzipped": 2040,
"treeshaked": {
"rollup": {
"code": 1250,
"import_statements": 417
"code": 1285,
"import_statements": 440
},
"webpack": {
"code": 3322
"code": 3336
}
}
},
"umd/react-router-dom.js": {
"bundled": 159709,
"minified": 57597,
"gzipped": 16540
"bundled": 162216,
"minified": 58840,
"gzipped": 16893
},
"umd/react-router-dom.min.js": {
"bundled": 97476,
"minified": 34651,
"gzipped": 10216
"bundled": 99449,
"minified": 35481,
"gzipped": 10400
}
}
69 changes: 69 additions & 0 deletions packages/react-router-dom/docs/api/Focus.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# &lt;Focus>

Provides a way for an application to add [focus management](https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex#managing_focus_at_the_page_level) after navigation for better accessibility.

```jsx
import { Focus } from 'react-router-dom'

<Focus>
{ref => (
<main tabIndex={-1} ref={ref}>
{/* ... */}
</main>
)}
</Focus>
```

`Focus` uses a render prop to provide a `ref`. The `ref` should be passed to the element that will be focused.
mjackson marked this conversation as resolved.
Show resolved Hide resolved

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Focusing a DOM element will give it an outline; you can style it with `outline: none;` to hide this outline.
pshrmn marked this conversation as resolved.
Show resolved Hide resolved

**Note:** Only the element that is passed the `ref` should have the `outline: none` style. A global `outline: none` rule should **not** be used because it will make your application inaccessible to users who navigate the page using their keyboard. You also should not use `outline: none` style if you are attaching the `ref` to a natively focusable element, like an `input`, `a`, or `button`.

```jsx
<Focus>
{ref => (
<main tabIndex={-1} ref={ref} style={{ outline: "none" }}>
{/* ... */}
</main>
)}
</Focus>
```

## children: function

The `children` function will be called with a [`ref`](https://reactjs.org/docs/refs-and-the-dom.html) and should return valid React elements.

```jsx
<Focus>
{ref => (
<main tabIndex={-1} ref={ref}>
{/* ... */}
</main>
)}
</Focus>
```

## preserve: bool

When `true`, if one of the focused element's children is already focused (e.g. uses `autofocus`), then the element will not steal the focus from the child. Defaults to `false`.

```jsx
<Focus preserve={true}>
{ref => ...}
</Focus>
```

## preventScroll: bool

When `true`, the application will not scroll to the element when it is focused. Defaults to `false`.

**Note:** This is experimental functionality that does not work in all browsers.

```jsx
<Focus preventScroll={true}>
{ref => ...}
</Focus>
```
73 changes: 73 additions & 0 deletions packages/react-router-dom/modules/Focus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import React from "react";
import { __RouterContext as RouterContext } from "react-router";
import warning from "tiny-warning";
import PropTypes from "prop-types";
import { locationsAreEqual } from "history";

class FocusWithLocation extends React.Component {
setRef = element => {
this.eleToFocus = element;
};

componentDidMount() {
this.focus();
}

componentDidUpdate(prevProps) {
// only re-focus when the location changes
if (!locationsAreEqual(this.props.location, prevProps.location)) {
this.focus();
}
}

focus() {
const { preserve, preventScroll } = this.props;
// https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex#managing_focus_at_the_page_level
if (this.eleToFocus != null) {
warning(
this.eleToFocus.hasAttribute("tabIndex") ||
this.eleToFocus.tabIndex !== -1,
'The ref must be assigned an element with the "tabIndex" attribute or be focusable by default in order to be focused. ' +
"Otherwise, the document's <body> will be focused instead."
);

if (preserve && this.eleToFocus.contains(document.activeElement)) {
return;
}

setTimeout(() => {
this.eleToFocus.focus({ preventScroll });
});
} else {
warning(
false,
"There is no element to focus. Did you forget to add the ref to an element?"
);
}
}

render() {
const { children } = this.props;
return children(this.setRef);
}
}

const Focus = props => (
<RouterContext.Consumer>
{context => <FocusWithLocation location={context.location} {...props} />}
</RouterContext.Consumer>
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Focus.defaultProps = {
preventScroll: false
};

if (__DEV__) {
Focus.propTypes = {
preventScroll: PropTypes.bool,
preserve: PropTypes.bool,
children: PropTypes.func
};
}

export default Focus;
Loading