Norigin-Spatial-Navigation icon indicating copy to clipboard operation
Norigin-Spatial-Navigation copied to clipboard

accept `focusDetails` argument in `focusSelf`

Open zemlanin opened this issue 1 year ago • 4 comments

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

zemlanin avatar Sep 26 '22 09:09 zemlanin

While setFocus accepts focusDetails argument, it is missing in type definition. Is it on purpose?

zemlanin avatar Sep 26 '22 10:09 zemlanin

Hello @zemlanin,

Thanks a lot for the contribution, it's appreciated.

We'll have a look, discuss and get back to you.

Cheers

predikament avatar Sep 27 '22 13:09 predikament

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! 🙇

predikament avatar Oct 19 '22 15:10 predikament

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

zemlanin avatar Oct 19 '22 16:10 zemlanin