Refactor focus on mount (2)

WordPress · Dec 14, 2020 · 5fb74bb · 5fb74bb
1 parent ecfa1ab
commit 5fb74bb
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 45 deletions.
diff --git a/packages/compose/README.md b/packages/compose/README.md
@@ -212,7 +212,7 @@ _Parameters_
 
 _Returns_
 
--   `(Function|Object)`: Element Ref.
+-   `(Function|undefined)`: Ref callback.
 
 <a name="useFocusReturn" href="#useFocusReturn">#</a> **useFocusReturn**
 

diff --git a/packages/compose/src/hooks/use-focus-on-mount/index.js b/packages/compose/src/hooks/use-focus-on-mount/index.js
@@ -1,14 +1,14 @@
 /**
  * WordPress dependencies
  */
+import { useCallback, useRef } from '@wordpress/element';
 import { focus } from '@wordpress/dom';
-import { useCallback, useEffect, useRef, useState } from '@wordpress/element';
 
 /**
  * Hook used to focus the first tabbable element on mount.
  *
  * @param {boolean|string} focusOnMount Focus on mount mode.
- * @return {Function|Object} Element Ref.
+ * @return {Function|undefined} Ref callback.
  *
  * @example
  * ```js
@@ -25,58 +25,37 @@ import { useCallback, useEffect, useRef, useState } from '@wordpress/element';
  * }
  * ```
  */
-function useFocusOnMount( focusOnMount = 'firstElement' ) {
-	const focusOnMountRef = useRef( focusOnMount );
-	const [ node, setNode ] = useState();
-	useEffect( () => {
-		focusOnMountRef.current = focusOnMount;
-	}, [ focusOnMount ] );
+export default function useFocusOnMount( focusOnMount = true ) {
+	const didMount = useRef( false );
+	const ref = useCallback( ( node ) => {
+		if ( ! node || didMount.current === true ) {
+			return;
+		}
 
-	// Ideally we should be running the focus behavior in the useCallback directly
-	// Right now we have some issues where the link popover remounts
-	// prevents us from doing that.
-	const ref = useCallback( setNode, [] );
+		didMount.current = true;
 
-	// Focus handling
-	useEffect( () => {
-		if ( ! node ) {
+		if ( node.contains( node.ownerDocument.activeElement ) ) {
 			return;
 		}
-		/*
-		 * Without the setTimeout, the dom node is not being focused. Related:
-		 * https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example
-		 *
-		 * TODO: Treat the cause, not the symptom.
-		 */
-		const focusTimeout = setTimeout( () => {
-			if ( focusOnMountRef.current === false || ! node ) {
-				return;
-			}
 
-			if ( focusOnMountRef.current === 'firstElement' ) {
-				const firstTabbable = focus.tabbable.find( node )[ 0 ];
+		let target = node;
 
-				if ( firstTabbable ) {
-					firstTabbable.focus();
-				} else {
-					node.focus();
-				}
+		if ( focusOnMount === 'firstElement' ) {
+			const firstTabbable = focus.tabbable.find( node )[ 0 ];
 
-				return;
+			if ( firstTabbable ) {
+				target = firstTabbable;
 			}
+		}
 
-			if (
-				focusOnMountRef.current === 'container' ||
-				focusOnMountRef.current === true
-			) {
-				node.focus();
-			}
-		}, 0 );
+		target.focus();
+	}, [] );
 
-		return () => clearTimeout( focusTimeout );
-	}, [ node ] );
+	// If focus doesn't need to be set on mount, no need to return a ref
+	// callback.
+	if ( ! focusOnMount ) {
+		return;
+	}
 
 	return ref;
 }
-
-export default useFocusOnMount;