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

focusSelf causes everything to be focused on Chrome

Open tieugioh opened this issue 2 years ago • 9 comments

Describe the bug Calling focusSelf() causes all focusable components to be in focusable state until arrow buttons are pressed

To Reproduce see this example: https://codesandbox.io/s/spatial-nav-example-66wiu9

Expected behavior First focusable child should be in focused state only.

tieugioh avatar Apr 25 '22 22:04 tieugioh

Hi!

This is very interesting... I have tried your example, and it works fine when I remove StrictMode in index.js. Apparently it has something to do with this. I will need to have a look. When looking at our example in App.tsx, we have very similar case: https://github.com/NoriginMedia/Norigin-Spatial-Navigation/blob/main/src/App.tsx#L167 But there is no such issue. It seems like all your Items are getting completely re-mounted, and their focusKeys are getting generated over and over, causing multiple items to have the same key. Focus key generation is happening here: https://github.com/NoriginMedia/Norigin-Spatial-Navigation/blob/main/src/useFocusable.ts#L115 And it is memorized, so as long as the item remains mounted, its focusKey is persistent, even if it was automatically generated.

But it's definitely interesting, worth investigating :)

asgvard avatar Apr 29 '22 18:04 asgvard

I have tried Strict mode in our App example and it works fine, however I've found that it doesn't work when I use:

import { createRoot } from 'react-dom/client';

When using good old ReactDOM.render(<App />, document.querySelector('#root')); - it works fine.

So the problem is somewhere in this createRoot. I'm wondering why it causes all Items to get fully remounted when the state of the App changes...I will try to look into it, since this createRoot is required to use React v18 features.

asgvard avatar Apr 29 '22 18:04 asgvard

Something is weird with this new createRoot + StrictMode. It is only reproducable with this combination. Works fine with the old ReactDOM.render, and works fine without StrictMode.

I've made even simpler example here: https://codesandbox.io/s/spatial-nav-example-bk72gs?file=/src/App.js As you can see, the App is mounted twice, and every state update increments the counter by 2. I would monitor React repo itself and see for similar issues, and if nothing, open a new issue there.

In relation to this library, I'm sure this is what causing focusKeys to be re-generated multiple times, and probably due to the focus keys collisions it marks multiple items as focused.

asgvard avatar Apr 29 '22 19:04 asgvard

Thanks for looking into this! i had a hunch it had something to do with react 18... I did a quick search in the react repo for this issue and didn't see anything related yet. In the meantime I'll probably stick with ReactDOM.render.

tieugioh avatar May 02 '22 20:05 tieugioh

https://reactjs.org/blog/2022/03/29/react-v18.html#new-strict-mode-behaviors this might have something to do with it.

tieugioh avatar May 02 '22 21:05 tieugioh

I'm having the exact same issue. Removing StrictMode for now, since I'm working on a POC. What would be the preferred work around though? Using ReactDOM.render vs removing React.StrictMode?

aupp avatar Jun 01 '22 08:06 aupp

I think the problem mostly lies in this new "createRoot", so unless you need some specific features from it, I would skip it for now. However if you don't need any specific features from StrictMode, you can of course omit it as well. We will look more into the root cause for this and how to make it work with both.

asgvard avatar Jun 01 '22 11:06 asgvard

Just recently came across this video from Reactathon.

David explains the common issues with useEffect and mentions some alternatives. One of the least awkward ways to get around the double firing of it is to use both useEffect + useRef. The value of useRef will not be lost between that simulated un-mount/re-mount.

If the useEffect with the empty dependency array is updated in useFocusable.ts to have this behaviour, the bug should disappear.

LynchyC avatar Jun 06 '22 13:06 LynchyC

Thanks! Nice find, I will watch it and see if it helps. Ohh, time to dive deeper in React v18 😃

asgvard avatar Jun 07 '22 13:06 asgvard

Hello! This issue should be now fixed in v1.1.0 🎉 We are using new useEffectOnce hook to avoid the "double mount" that is coming from the React v18 StrictMode.

asgvard avatar Sep 08 '22 10:09 asgvard