NoriginMedia / Norigin-Spatial-Navigation

React Hooks based Spatial Navigation (Key & Remote Control Navigation) / Web Browsers, Smart TVs and Connected TVs
MIT License
325 stars 96 forks source link

How to track the current item in focus after focus loss #25

Closed Anonymous-NewBee closed 1 year ago

Anonymous-NewBee commented 2 years ago

Is your feature request related to a problem? Please describe.

We are using Norigin Media for smart tv app development. One issue we keep of facing is loss of focus because of incorrect code and from the logs we are unable to track how to find out the current item in focus.

e.g.

setFocusnewFocusKey sn:focusable-item-1
index.js:1 smartNavigatedirection down
index.js:1 smartNavigatefromParentFocusKey null
index.js:1 smartNavigatethis.focusKey sn:focusable-item-1
index.js:1 smartNavigatecurrentComponent sn:focusable-item-1 null
index.js:1 smartNavigatecurrentCutoffCoordinate 0
index.js:1 smartNavigatesiblings 3 elements: sn:focusable-item-1, MAIN

So even though now that we have lost focus and current focussed item is sn:focusable-item-1, we are unable to locate which item in DOM corresponds to this element

Describe the solution you'd like

Is there a way we can track the currently focussed item in DOM, so that we can track why it is focussed now. E.g. if we can log the entire focussable tree in console or see the focused item (sn:focusable-item-1) in DOM so that developer can debug the issue better?

Please also suggest is there any other way to solve this problem which we may have missed out

predikament commented 2 years ago

Hello @Anonymous-NewBee!

Sorry to hear you're having some issues, but I will try to offer some suggestions to help you out:

These in themselves can be very helpful, but I will assume that you have already tried using these.

Since the issue is that you are unsure which DOM element corresponds to which focusable element, I would try to add fixed (statically named) focus-key values to your focusable components: https://github.com/NoriginMedia/Norigin-Spatial-Navigation/#focuskey-optional

Using this you could create focus-key values that are easy to trace, for example something like MY_SPECIFIC_ELEMENT_TYPE_${index}. If you do this then it should help you track down and understand exactly which element is gaining (or losing) focus at any given point in time.

I will close this on the assumption that this will help you resolve your issue. Feel free to reopen it if you need further input.

Best of luck to you and happy coding! ☺️

zemlanin commented 2 years ago

I'm also having problems with focus loss/stuck

Judging by debug logs, there are still references to nodes that have already left the DOM (during navigation between pages), and that are treated as siblings of nodes in the current DOM tree. Can't find reliable reproduction steps for the issue, but it happens quite often

(I've already tried calling root element's updateAllLayouts as an effect of location, but focus still gets stuck on the node that left the page)

predikament commented 2 years ago

Judging by debug logs, there are still references to nodes that have already left the DOM (during navigation between pages), and that are treated as siblings of nodes in the current DOM tree. Can't find reliable reproduction steps for the issue, but it happens quite often

@zemlanin: Hello again 👋

This sounds really strange to me. We are using this in several production apps and have not experienced any such issues.

I assume there is more to it - If you could please provide an example where this issue can be reproduced, then we will look into it.

Thanks.

zemlanin commented 2 years ago

Yeah, hi :)

I'll keep looking for an isolated reproduction case, but it seems to be related to preferredChildFocusKey: when I go back to a previous page after current page has already rendered but before SpatialNavigation calls getNextFocusKey for it, library sets focus on the node that just has left the tree

Maybe, some "funny" interaction with react-router@6… 🤔

zemlanin commented 2 years ago

Seems like it is a funny interaction with react-router. Wrapping navigate into setTimeout() consistently works around the issue

Anyways, here's a repro: https://github.com/zemlanin/lost-focus-bug

zemlanin commented 1 year ago

Hi. Any updates regarding reproduction case?

zemlanin commented 1 year ago

The reproduction case works fine in desktop Chrome but fails in Safari — cleanup function of useEffectOnce just isn't called, so the element from another page is kept in service's this.focusableComponents

Unsure if that's what also happening for me on TVs, will keep digging…

zemlanin commented 1 year ago

As a workaround, the reseting spatial navigation service (by calling destroy, after applying changes from https://github.com/NoriginMedia/Norigin-Spatial-Navigation/pull/47) on navigation between screens seems to work

predikament commented 1 year ago

Hello @zemlanin.

Sorry for not getting back to you on this as quickly as I had wanted, but I have personally been quite busy these last few weeks.

Also again, sorry to hear you're having these issues with the hook - To me it kind of looks like this is happening when used in combination with react-router v6, as you mentioned.

Generally I would like to try to avoid needing to make changes in relation to exposing lots of the internal functions of the service, as it should work without needing to do things like manually calling navigate / destroy / using timeouts, etc.

We need to fix this in the library (in a good way), if we can. Exposing internal or additional functionalities should be more of an "extra" to account for some edge-case or add some more functionality, more so than "we literally cannot use this library without calling these functions ourselves".

As such, I will ping @asgvard on this one, in case he has time to look into this, as I will be even busier for the next few days.

zemlanin commented 1 year ago

This might be related?.. https://github.com/NoriginMedia/Norigin-Spatial-Navigation/blob/dbf9a315f2dce2b42ed8c882c95419d9942a703c/src/useEffectOnce.ts

@asgvard Was this hook introduced only to fix focusKey generation in StrictMode, which has lead to focusing on everything (https://github.com/NoriginMedia/Norigin-Spatial-Navigation/issues/8)?

guilleccc commented 1 year ago

Seems like it is a funny interaction with react-router. Wrapping navigate into setTimeout() consistently works around the issue

Anyways, here's a repro: https://github.com/zemlanin/lost-focus-bug

First of all, thanks @zemlanin for sharing this example code. That was very useful to reproduce the issue you were mentioning. So as you mentioned, that issue was reproducible on Safari.

However deep in the code you shared, I found that there was repeated focusKey along different instances of View. After removing line96 (focusKey: "view",), the example worked fine in Safari. In case you need to explicitly set a focusKey, be aware that they might need to have different value. So following your example you can would need to use <View focusKey="RootView"> and <View focusKey={"BackView"}>, and pass that prop to useFocusable.

It is a fair point that there is no mention at the docs page that the focusKey might be unique. We should add that later.

Let me know if that works for you as well

zemlanin commented 1 year ago

Ahhh… Thanks 🙏 I'll try it next week

For what it worth, it seems, at least initially, very strange to assign different focusKeys for singleton components (like "content" and "sidebar") on different screens. Yeah, global focus effect/remote shortcut might rely on react context with generated focusKey, but that's less obvious than setFocus(CONTENT_FOCUS_KEY)

zemlanin commented 1 year ago

Still trying, but I might have found another "lost focus" case: https://github.com/zemlanin/lost-focus-bug/commit/8f055fe3f2004353250cba0dcd2c63767515da69

  1. Focus and press "add nested button"
  2. Focus and press "remove button"
  3. Focus seems to be lost

When container with a focused child is removed from DOM, the library should focus on a container's parent, shouldn't it?

(~as a workaround, used const parentFocusKey = useContext(FocusContext); in the child component~ hmm, that doesn't always work)

zemlanin commented 1 year ago

And another: https://github.com/zemlanin/lost-focus-bug/commit/0e3bcd20c5395c311c5769bd47bd6b20572a94c0

  1. Open main view (http://localhost:8000/)
  2. Focus and press "/similar"
  3. Focus is set to view-/similar, but the library thinks that view-/ is rendered (judging by visualDebug)

Removing set focusKey from View helps, but then it gets harder to make some element focusable from its sibling without using a constant focusKey, i.e. in case like this:

function Root() {
  const location = useLocation()
  const focusKey = someFn(location)

  return <main>
    <Sidebar contentFocusKey={focusKey} />
    <Content focusKey={focusKey} />
  </main>
}

function Sidebar({ contentFocusKey }) {
  const { ref, setFocus } = useFocusable({
    focusKey: contentFocusKey + '-sidebar'
  })

  function someCallback() {
    setFocus(contentFocusKey)
  }

  // ...
}

function Content({ focusKey }) {
  const { ref } = useFocusable({
    focusKey: focusKey
  })

  // ...
}

(as a workaround, reorganized Root and Content components in the app)

predikament commented 1 year ago

Thanks for the input on these issues, @zemlanin, we really appreciate it. 🙇

I'll make sure we get these looked into and provide some feedback and / or fixes.

zemlanin commented 1 year ago

When container with a focused child is removed from DOM, the library should focus on a container's parent, shouldn't it?

Seems to be fixable with these lines in SpatialNavigation.removeFocusable, to propagate removed node's parent to its (potentially focused) children:

const children = filter(
  this.focusableComponents,
  (component) =>
    component.parentFocusKey === focusKey && component.focusable
);

if (children.length) {
  forEach(children, (child) => {
    // eslint-disable-next-line no-param-reassign
    child.parentFocusKey = parentFocusKey;
  });
}
zemlanin commented 1 year ago

And another: https://github.com/zemlanin/lost-focus-bug/commit/0e3bcd20c5395c311c5769bd47bd6b20572a94c0

And this one is fixable in app code via setting different keys for reused react-router element:

<Routes>
  <Route path="/" element={<RootView key="root" />} />
  <Route path="/similar" element={<RootView key="similar" />} />
</Routes>

But the fix for parameterized paths, like /videos/:id, is more involved

guilleccc commented 1 year ago

Thanks @zemlanin for your contribution.

When container with a focused child is removed from DOM, the library should focus on a container's parent, shouldn't it?

Seems to be fixable with these lines in SpatialNavigation.removeFocusable, to propagate removed node's parent to its (potentially focused) children:

This was updated in the last version based on your suggestion.

And this one is fixable in app code via setting different keys for reused react-router element:

Based on what I see here, there is no creation of new focusables when entering the "similar" page. As far as I can see, the RootView is not unmounted (root) -> mounted (similar) when the location is changed. That means that entering a new page, keeps the old focusableComponents if they use the same route element (from react-router-dom). Alternatively you can pass key or define your customKey for every "duplicated route". As this seems to be an edge case when using react-router-dom, we won't fix it for now. Anyway any input from @asgvard would be appreciated.

zemlanin commented 1 year ago

This was updated in the last version based on your suggestion.

Thank you

As far as I can see, the RootView is not unmounted (root) -> mounted (similar) when the location is changed

If I read useFocusableHook correctly, if hook's focusKey option has changed, SpatialNavigation.removeFocusable+SpatialNavigation.addFocusable won't be called (so there might be a stale reference in memory), and SpatialNavigation.updateFocusable will update (and/or create?) a new focusable object with only partial contents (no saveLastFocusedChild, trackChildren, or autoRestoreFocus)

lmalom commented 1 year ago

Did you find a good way to go around the problem of routing and this library?

I am having a similar focus problem when using react-router-dom, especially when the application is run on older TV's (Tizen 5.0, 5.5, 6.0).

I have reproduced this bug by adding routing to the library example: Routing looses focus. I have also added more assets to up the number of rendered elements in the DOM.

It seems (just like zemlanin) that the focused element is an element that is no longer in the DOM.

predikament commented 1 year ago

I am going to close this issue as it's not been updated for a longer period.

When it comes to loss of focus there's been a release to add some improvements as to how to detect that: https://github.com/NoriginMedia/Norigin-Spatial-Navigation/releases/tag/v2.0.0

Hopefully this will help resolve the issues you guys are seeing; If not then please open a new issue with the specific problem you are having and we can discuss it accordingly.