From a6feb4d92801f93bb6bf52a3fa04a95fffda1b73 Mon Sep 17 00:00:00 2001 From: Dmitriy Bryokhin Date: Thu, 11 May 2023 11:15:06 +0200 Subject: [PATCH] Removed custom "useEffectOnce" hook, added more logs (#79) --- CHANGELOG.md | 11 ++++++++ package-lock.json | 4 +-- package.json | 2 +- src/SpatialNavigation.ts | 59 +++++++++++++++++++++++++++++++--------- src/useEffectOnce.ts | 44 ------------------------------ src/useFocusable.ts | 5 ++-- 6 files changed, 62 insertions(+), 63 deletions(-) delete mode 100644 src/useEffectOnce.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dc1dba..48132c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +# [1.3.1] +## Added +- Extra debug logs, printing focusable components data in addition to DOM nodes. +- Extra call to set `focused` state to `false` on unmount. This is to support "double-mount" in Strict mode in React 18. + +## Changed +- [Potentially Breaking] Auto restore focus when the item is removed is now happening with a slight debounced delay. + +## Removed +- Custom `useEffectOnce` hook that introduced issues with unmounted components being remained as focusable. + # [1.3.0] ## Added - new `init` config option `shouldFocusDOMNode` that focuses the underlying _accessible_ DOM node too. diff --git a/package-lock.json b/package-lock.json index 5fbb88c..4aaee60 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@noriginmedia/norigin-spatial-navigation", - "version": "1.3.0", + "version": "1.3.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@noriginmedia/norigin-spatial-navigation", - "version": "1.3.0", + "version": "1.3.1", "license": "MIT", "dependencies": { "lodash": "^4.17.21" diff --git a/package.json b/package.json index 798b420..f3969c8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@noriginmedia/norigin-spatial-navigation", - "version": "1.3.0", + "version": "1.3.1", "description": "React hooks based Spatial Navigation solution", "main": "dist/index.js", "types": "dist/index.d.ts", diff --git a/src/SpatialNavigation.ts b/src/SpatialNavigation.ts index ce527ae..8653326 100644 --- a/src/SpatialNavigation.ts +++ b/src/SpatialNavigation.ts @@ -6,6 +6,7 @@ import findKey from 'lodash/findKey'; import forEach from 'lodash/forEach'; import forOwn from 'lodash/forOwn'; import throttle from 'lodash/throttle'; +import debounce from 'lodash/debounce'; import difference from 'lodash/difference'; import measureLayout, { getBoundingClientRect } from './measureLayout'; import VisualDebugger from './VisualDebugger'; @@ -39,6 +40,8 @@ const DIAGONAL_SLICE_WEIGHT = 1; */ const MAIN_COORDINATE_WEIGHT = 5; +const AUTO_RESTORE_FOCUS_DELAY = 300; + const DEBUG_FN_COLORS = ['#0FF', '#FF0', '#F0F']; const THROTTLE_OPTIONS = { @@ -219,6 +222,8 @@ class SpatialNavigationService { private logIndex: number; + private setFocusDebounced: DebouncedFunc; + /** * Used to determine the coordinate that will be used to filter items that are over the "edge" */ @@ -554,6 +559,11 @@ class SpatialNavigationService { this.setKeyMap = this.setKeyMap.bind(this); this.getCurrentFocusKey = this.getCurrentFocusKey.bind(this); + this.setFocusDebounced = debounce(this.setFocus, AUTO_RESTORE_FOCUS_DELAY, { + leading: false, + trailing: true + }); + this.debug = false; this.visualDebugger = null; @@ -879,7 +889,8 @@ class SpatialNavigationService { 'smartNavigate', 'currentComponent', currentComponent ? currentComponent.focusKey : undefined, - currentComponent ? currentComponent.node : undefined + currentComponent ? currentComponent.node : undefined, + currentComponent ); if (currentComponent) { @@ -935,7 +946,8 @@ class SpatialNavigationService { 'siblings', `${siblings.length} elements:`, siblings.map((sibling) => sibling.focusKey).join(', '), - siblings.map((sibling) => sibling.node) + siblings.map((sibling) => sibling.node), + siblings.map((sibling) => sibling) ); } @@ -963,7 +975,8 @@ class SpatialNavigationService { 'smartNavigate', 'nextComponent', nextComponent ? nextComponent.focusKey : undefined, - nextComponent ? nextComponent.node : undefined + nextComponent ? nextComponent.node : undefined, + nextComponent ); if (nextComponent) { @@ -1159,6 +1172,12 @@ class SpatialNavigationService { this.updateLayout(focusKey); + this.log( + 'addFocusable', + 'Component added: ', + this.focusableComponents[focusKey] + ); + /** * If for some reason this component was already focused before it was added, call the update */ @@ -1171,7 +1190,11 @@ class SpatialNavigationService { const componentToRemove = this.focusableComponents[focusKey]; if (componentToRemove) { - const { parentFocusKey } = componentToRemove; + const { parentFocusKey, onUpdateFocus } = componentToRemove; + + onUpdateFocus(false); + + this.log('removeFocusable', 'Component removed: ', componentToRemove); delete this.focusableComponents[focusKey]; @@ -1189,18 +1212,21 @@ class SpatialNavigationService { return; } - forEach(this.focusableComponents, (component) => { - if (component.parentFocusKey === focusKey && component.focusable) { - // eslint-disable-next-line no-param-reassign - component.parentFocusKey = parentFocusKey; - } - }); - /** - * If the component was also focused at this time, focus another one + * If the component was also focused at this time, focus its parent -> it will focus another child */ if (isFocused && parentComponent && parentComponent.autoRestoreFocus) { - this.setFocus(parentFocusKey); + this.log( + 'removeFocusable', + 'Auto restoring focus to: ', + parentFocusKey + ); + + /** + * Focusing parent with a slight delay + * This is to avoid multiple focus restorations if multiple children getting unmounted in one render cycle + */ + this.setFocusDebounced(parentFocusKey); } } } @@ -1233,6 +1259,8 @@ class SpatialNavigationService { this.getNodeLayoutByFocusKey(this.focusKey), focusDetails ); + + this.log('setCurrentFocusedKey', 'onBlur', oldComponent); } this.focusKey = newFocusKey; @@ -1249,6 +1277,8 @@ class SpatialNavigationService { this.getNodeLayoutByFocusKey(this.focusKey), focusDetails ); + + this.log('setCurrentFocusedKey', 'onFocus', newComponent); } } @@ -1385,6 +1415,9 @@ class SpatialNavigationService { } setFocus(focusKey: string, focusDetails: FocusDetails = {}) { + // Cancel any pending auto-restore focus calls if we are setting focus manually + this.setFocusDebounced.cancel(); + if (!this.enabled) { return; } diff --git a/src/useEffectOnce.ts b/src/useEffectOnce.ts deleted file mode 100644 index c0ca30e..0000000 --- a/src/useEffectOnce.ts +++ /dev/null @@ -1,44 +0,0 @@ -/** - * Used to avoid multiple effect calls due to new React v18 feature in the Strict mode that mounts all components - * twice to simulate state persistence between mount + unmount + 2nd mount cycle. - * https://blog.ag-grid.com/avoiding-react-18-double-mount/ - */ -import { useEffect, useRef, useState } from 'react'; - -const useEffectOnce = (effect: () => void | (() => void)) => { - const effectFn = useRef<() => void | (() => void)>(effect); - const destroyFn = useRef void)>(); - const effectCalled = useRef(false); - const rendered = useRef(false); - const [, setVal] = useState(0); - - if (effectCalled.current) { - rendered.current = true; - } - - useEffect(() => { - // only execute the effect first time around - if (!effectCalled.current) { - destroyFn.current = effectFn.current(); - effectCalled.current = true; - } - - // this forces one render after the effect is run - setVal((val) => val + 1); - - return () => { - // if the comp didn't render since the useEffect was called, - // we know it's the dummy React cycle - if (!rendered.current) { - return; - } - - // otherwise this is not a dummy destroy, so call the destroy func - if (destroyFn.current) { - destroyFn.current(); - } - }; - }, []); -}; - -export default useEffectOnce; diff --git a/src/useFocusable.ts b/src/useFocusable.ts index 0429cce..7e5c04b 100644 --- a/src/useFocusable.ts +++ b/src/useFocusable.ts @@ -15,7 +15,6 @@ import { KeyPressDetails } from './SpatialNavigation'; import { useFocusContext } from './useFocusedContext'; -import useEffectOnce from './useEffectOnce'; export type EnterPressHandler

= ( props: P, @@ -140,7 +139,7 @@ const useFocusableHook =

({ [focusKey] ); - useEffectOnce(() => { + useEffect(() => { const node = ref.current; SpatialNavigation.addFocusable({ @@ -168,7 +167,7 @@ const useFocusableHook =

({ focusKey }); }; - }); + }, []); // eslint-disable-line react-hooks/exhaustive-deps useEffect(() => { const node = ref.current;