Norigin-Spatial-Navigation
Norigin-Spatial-Navigation copied to clipboard
accept `focusDetails` argument in `focusSelf`
An application scrolls to an element in onFocus
handler. onFocus
can be called either automatically (for example, on page load) or in response of user action (keyboard event or, in case of webOS, mouse event). For the sake of convenience, cursor hover on webOS is handled by calling focusSelf
Because of UX, scroll on automatic focus has behavior: 'instant'
, on user action — behavior: 'smooth'
. But because there's no way to pass MouseEvent
to focusSelf
, there's no way to distinguish "mouse hover focus" from "page load focus". Accepting focusDetails
argument in focusSelf
would allow application to pass MouseEvent
when it's necessary to do that and then do something like this:
const onFocus = (layout, extraProps, focusDetails) => {
scrollIntoViewIfNeeded(layout.node, {
behavior:
focusDetails?.event instanceof KeyboardEvent ||
focusDetails?.event instanceof MouseEvent
? 'smooth'
: 'instant',
});
};
This change isn't critical, since application can call setFocus(focusKey, { event })
instead of focusSelf()
/focusSelf({ event })
, but a shorter option is convenient to have
While setFocus
accepts focusDetails
argument, it is missing in type definition. Is it on purpose?
Hello @zemlanin,
Thanks a lot for the contribution, it's appreciated.
We'll have a look, discuss and get back to you.
Cheers
Hello again @zemlanin,
We will be reviewing this PR next week.
From a quick glance it looks like this will need another small change related to focusDetails.event
, since currently it is always of type KeyboardEvent
.
We might just do this change ourselves, depending on what comes out of the review, but we'll get back to you.
Thanks regardless for the contribution! 🙇
I didn't want to potentially require library users to deal with MouseEvent
type if they don't need it (and since the library itself doesn't fire them) and extended the focusDetails
type in the app code
I created a new PR based on your suggestion @zemlanin . And this change was published in the last release.
Now focusDetails
can contain a KeyboardEvent or MouseEvent, or any value you pass into it. In case you receive a SyntheticEvent you might need to use the nativeEvent
key (instead of event
). So your previous code should be slightly changed to:
const onFocus = (layout, extraProps, focusDetails) => {
scrollIntoViewIfNeeded(layout.node, {
behavior:
focusDetails?.event instanceof KeyboardEvent ||
focusDetails?.nativeEvent instanceof MouseEvent
? 'smooth'
: 'instant',
});
};
Alternatively you can pass any object when you load your content/page, for instance:
useEffect(() => {
focusSelf({scroll: 'instant'});
}, [focusSelf]);
And then previous example code can be replaced to:
const onFocus = (layout, extraProps, focusDetails) => {
scrollIntoViewIfNeeded(layout.node, {
behavior:
focusDetails?.scroll === 'instant'
? 'instant'
: 'smooth',
});
};
Thanks for your contribution.
Closing as fixed.