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

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 1 commit
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
22 changes: 11 additions & 11 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": 10390,
"minified": 6185,
"gzipped": 2000,
"treeshaked": {
"rollup": {
"code": 1250,
"code": 1262,
"import_statements": 417
},
"webpack": {
"code": 3322
"code": 3336
}
}
},
"umd/react-router-dom.js": {
"bundled": 159709,
"minified": 57597,
"gzipped": 16540
"bundled": 162104,
"minified": 58742,
"gzipped": 16871
},
"umd/react-router-dom.min.js": {
"bundled": 97476,
"minified": 34651,
"gzipped": 10216
"bundled": 99476,
"minified": 35454,
"gzipped": 10393
}
}
59 changes: 59 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,59 @@
import React from "react";
import { __RouterContext as RouterContext } from "react-router";
import warning from "tiny-warning";

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

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

componentDidMount() {
this.focus();
}

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

@pshrmn pshrmn Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@pshrmn pshrmn Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.focus();
}
}

focus() {
const { preserve, preventScroll = false } = this.props;
timdorr marked this conversation as resolved.
Show resolved Hide resolved
// 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?"
);
}
}
}

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.

export default Focus;
Loading