[Flare] First pass at an implementation of experimental FocusManager API #15849
Conversation
Details of bundled changes.Comparing: 824e9be...ef25df7 react-dom
react-events
Generated by 🚫 dangerJS |
|
I'm not exactly sure why the |
|
I think might be running tests on the bundles and since the feature flags aren't bundled it is failing. |
|
Ah ok, thanks @tjallingt! |
|
That seems to have fixed everything except fire. Maybe there is another flag for that somewhere... |
|
React Fire is a reimplementation of EDIT: the changes you made to |
|
Awesome, thanks for the help @tjallingt! 🎉 |
|
This looks awesome and I'll take a proper deep dive tomorrow. One thing I was thinking though - why is import { FocusScope, FocusManager } from 'react-events/focus-scope';or import FocusScope from 'react-events/focus-scope';
const FocusManager = FocusScope.Manager;You wouldn't really ever use one without the other too, but also it addresses some of the code-smells you had in regards to ReactDOM having to know implementation details relating to specific event responder modules (thus thus Furthermore, this isn't only relevant for ReactDOM. We might want the same high-level APIs to be exposed to other surfaces, such as React Native and React VR. In which case, they'd both be consuming the same APIs - so it doesn't make as much sense to be importing the manager from If they are tightly coupled, then import {FocusScope, FocusManager} from 'react-events/focus-scope'
function LeftPanel() {
function focusLeft() {
if (isRTL) {
FocusManager.focusScopeByKey('centre-panel');
} else {
FocusManager.focusScopeByKey('left-content');
}
}
function focusRight() {
if (isRTL) {
FocusManager.focusScopeByKey('right-panel');
} else {
FocusManager.focusScopeByKey('centre-content');
}
}
return (
<FocusScope key="left-panel" onLeftArrow={focusLeft} onRightArrow={focusRight}>
....
</FocusScope>
)
}Using |
|
@trueadm yeah I totally agree - it should ideally be in react-events. I just couldn't find a good way to expose the necessary internals from react-dom for react-events to access, e.g. If you have any guidance around how to do that, I'd be happy to update it. |
|
@devongovett One way would be to make it explicit: let focusManager = null;
<FocusScope getFocusManager={fm => focusManager = fm}>
...
</FocusScope>I also updated my post. Not sure if you saw the updated version. :) |
|
Definitely a possibility - I thought of maybe using a ref for it (make a ref to the FocusScope). It makes the API a little harder to use though IMO. Currently, the scope is found automatically based on the node you pass in, or the active element if no option is given. This means you don't need to know what scope an element is in, just that you need to move the focus. Also, sometimes a scope is defined further up the tree, but the events to move focus (e.g. arrow key handlers) are further down. Not the worst thing in the world, but it would require either manual propagation of those key events up to the parent with the FocusScope, or passing a FocusManager instance down the tree to the component that needs it. I guess this is an API ergonomics question more than anything. |
|
@devongovett I guess I was just hinting at a ref there. Automatic scope is nice, but really, if the manager is tied to the current FocusScope, that magic might actually be a problem and causes bugs further on down the line. If you want to do |
|
Just saw your updated post. I think both usecases would be valuable to support. One usecase is to move focus within a scope: for example a keyboard navigable list component with up and down arrow support. In many cases, you don't know how many items will appear in that list, and shouldn't need to manually pass the keys of each item to Another usecase is to move focus between focus scopes like in your example of panels. I think the idea of using keys for that seems good. In addition, focusing a particular element by key might also be useful in case you know exactly which element to focus and want to avoid refs. Definitely open to expanding on this API, I was mostly focused on the first usecase for this initial implementation. |
|
Perhaps a hook like API to get the parent focus manager? let focusManager = useFocusManager();
let onKeyDown = e => {
if (e.key === 'ArrowRight') {
focusManager.focusNext();
}
}; |
@devongovett That's a neat way. We can totally do that using hooks :)
That's fine. I just feel we should stay away from using DOM references. It makes this API hard to bring over to RN and other surfaces as soon as we make it about the DOM (which is definitely something we want to look into doing next half). |
|
Alright. I'll try experimenting with a hooks based API to get the focus manager. Would you recommend implementing that with context? Can event components expose a value to context somehow? My knowledge of react internals is very limited so far. 😉 |
|
@devongovett The idea was to always have a way of using event components via hooks. We never focused on that this half, but the function useFocusScope() {
const focusManager = useRef(null);
const FocusScope = useEventComponent(FocusScope, { ref: focusManager });
return [FocusScope, focusManager ];
}
const [FocusScope, focusMananger] = useFocusScope();
<FocusScope>
...
</FocusScope>Does that make sense? I can add a Alternatively, there can be another hooks API that would look up event components in the tree (kind of like you mentioned above), but that might be more difficult to design without it being too fragile right now. |
|
Oh, so rather than importing I guess I was more thinking you'd use FocusScope the same way as is currently implemented, and import a import {FocusScope, useFocusManager} from 'react-events/focus-scope';
function Parent() {
return (
<FocusScope>
<Child />
</FocusScope>
);
}
function Child() {
let focusManager = useFocusManager();
let onKeyDown = e => {
if (e.key === 'ArrowRight') {
focusManager.focusNext();
}
};
return <div onKeyDown={onKeyDown}>Child</div>;
}If you needed the focus manager in a component that rendered the scope (e.g. |
|
@devongovett Yeah, that was what I meant at the end of my comment. It's definitely possible to do, just will require some thought on how to best do it. :) Give me tonight/tomorrow to think about such an API. |
|
Thinking about this more, I feel that you'd probably use React context to capture the focus manager instead of introducing new primitive hooks into React. Introducing new primitive hooks that are so coupled to a specific event component implementation is risky and somewhat brittle to change – so I think think we should avoid that. |
|
Yeah I agree. What if react implemented it internally using context though: const FocusScopeInternal = React.unstable_createEventComponent(
FocusScopeResponder,
'FocusScope',
);
const FocusContext = React.createContext();
export function FocusScope(props) {
let focusManagerContext = {/* ... */};
return (
<FocusContext.Provider value={focusManagerContext}>
<FocusScopeInternal {...props} />
</FocusContext.Provider>
}
export function useFocusManager() {
return useContext(FocusContext);
}I guess the only small downside is that you'd see two components in dev tools instead of just one, but maybe there is a way to hide that. |
|
@devongovett I don't think we want to start embedding context into export function FocusScopeWithManager(props) {
return React.createElement(
FocusContext.Provider, {
value: focusManagerContext,
children: React.createElement(FocusScope, props);
}
);
}The reason is so that people can still access the original event component to be used in other APIs (like hooks). |
|
Aside from basic focus containment, I think usage with a focus manager will be the most common use for a FocusScope. So not sure why we'd want two different components. I think that might get confusing. |
|
@devongovett They are different components though: one is a functional component and the other is an event component. Event components can be consumed in different ways, so they both need to be exported from the event module. Unless we add a way of somehow mutating the |
|
Isn't the focus manager meant to be the imperative controller for the focus scope? And aren't refs basically meant to expose imperative controllers for components? For DOM components, that's the underlying DOM node sure, but you can also call imperative methods on it. Either way it won't get rid of the function wrapper component anyway because I still need to expose the controller to context. |
|
@devongovett Let me explain what I'm saying with some code: let ListContainer = (props) => {
return (
<FocusScope>
<List {...props} />
</FocusScope>
);
};
let List = (props) => {
const mananger = useFocusManager();
let onKeyDown = (e) => {
switch (e.key) {
case 'ArrowUp':
e.preventDefault();
mananger.focusPrevious();
break;
case 'ArrowDown':
e.preventDefault();
mananger.focusNext();
break;
}
};
return (
<ul onKeyDown={onKeyDown}>
<li tabIndex="0">Item 1</li>
<li tabIndex="0">Item 2</li>
<li tabIndex="0">Item 3</li>
</ul>
);
};The implementation of these looks like this: const FocusScopeImpl = React.unstable_createEventComponent(FocusScopeResponder, 'FocusScope');
const FocusManagerContext = React.createContext(null);
function createFocusManager() {
return {
// ...
};
}
export function FocusScope({ autoFocus, contain, children }) {
const focusManager = useRef(() => createFocusManager()));
return (
<FocusManagerContext.Provider value={focusManager}>
<FocusScopeImpl
autoFocus={autoFocus}
contain={contain}
// let the FocusScope responder know about
// its associated mananger
focusManager={focusManager}
children={children}
/>
</FocusManagerContext.Provider>
)
}
export function useFocusManager() {
return useContext(FocusManagerContext);
}I'm essentially saying that you shouldn't be able to access the |
|
Hmm ok... that's not what we discussed earlier in this thread. Do you have an example for why you think accessing via
Isn't the hook for accessing focus managers in child sub-trees? |
I think we were talking about different things. I was trying to come up with an approach that didn't use a
No, they access the immediate parent instance in the current branch of the tree, rather than the child of a tree. It's only context after-all.
They're simply not needed here. Having one way of accessing the focus manager, via the hook is much better than having many different ways (hooks + ref) IMO. Plus, I'd really like a future world where React did not have to use refs, and instead could use hooks, effects and declarative UI via props to describe everything. Given event components are new, I think we should try harder before just use the escape hatch approach of |
|
I'm all for getting rid of refs, but it might be confusing for people that they need to wrap their component in another component in order to use a focus manager. Calling I'll go ahead and change it. Just thought I'd throw that out there. |
|
@trueadm Ref support removed. |
|
There are some merge conflicts. |
|
@trueadm Fixed the conflicts. |
|
... and now there are more. Let me know when you're about ready to merge it and I'll resolve again. 😉 |
There was a problem hiding this comment.
So is the plan to address focus management across FocusScope instances in a follow up? I think that part is really important and will likely have an impact on the API design.
One sticking point I have with useFocusManager is that you have to call it in a child of FocusScope. I could see scenarios where the component rendering FocusScope would define methods that might want to manage the focus of that scope. I know we want to restrict access to the focus manager, but it feels awkward having to add a container component just for FocusScope.
This approach also means that every FocusScope instance pays the cost of rendering a context provider, and it's not clear to me if that's the right default. I have a feeling most people will just rely on the default semantics of FocusScope
| const FocusScopeContext: React.Context<FocusManager> = React.createContext(); | ||
|
|
||
| export function FocusScope(props: FocusScopeProps) { | ||
| let internalFocusManager: ?FocusManager; |
There was a problem hiding this comment.
Is it intentional that this gets reset every time FocusScope renders? Maybe this should be stored in a ref?
There was a problem hiding this comment.
That's a good suggestion
| ); | ||
| return internalFocusManager.focusPrevious(options); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
focusManager should be memoized if it's being passed down through context.
| focusManager.focusPrevious({from: divRef.current}); | ||
| expect(document.activeElement).toBe(inputRef.current); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@devongovett can you add a test for nested FocusScopes?
|
@aweary well... I already suggested two possible APIs to do this in this PR which were both rejected (singleton, and refs), so I think a follow up would be needed. I'd love to hear other suggestions! |
|
Fixed the conflicts again and updated to incorporate @aweary's comments. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing since this won’t be merged. Will follow up with @trueadm separately on the current state of focus management in React. |
This is a first pass at implementing the
FocusManagerAPI specified in reactjs/rfcs#109. It goes along with the already implementedFocusScopein react-events.I adjusted the API slightly, and will be updating the RFC to match. In particular, the
focusNextandfocusNextTabStopmethods have been merged, along with the corresponding previous methods. There is now an options object that can be passed to specify whether the element must be tabbable or only focusable. In addition, there are options to enable wrapping behavior at the ends of a scope, along with starting the search from a particular element rather than onlydocument.activeElement. The methods also return the element they focused if any, which allows the calling code to do something with that information.The API is now:
Perhaps we will also want
focusFirstandfocusLastmethods at some point as well. Happy to hear whatever feedback you have on the API.Most of the implementation is moved from the existing code for
FocusScope. There were a few things I wasn't sure about though:ReactDOM.unstable_FocusManagerif the event API feature flag is enabled. Perhaps it should be exposed as part ofreact-eventsinstead ofreact-dom, but I didn't see a good way of exposing the necessary internals.FocusScopeevent components in the tree. For now, I added anisFocusScopeoption to the event responder, but this is kind of a hack. I didn't want react-dom to have a dependency on react-events, so I wasn't sure how else to do it.focusNext({from: node}), where node contains focusable children? Should it focus the first item within that node, or go to the next focusable element within a sibling?Thanks in advance for your feedback! 🙏