[Select] Focusing component with inputRef gives null value despite check

[fast-reflexes]

Problem:
I attempt to focus a MaterialUI Select component programmatically by passing a callback ref. In this ref, I verify that that the input is not null and then I try to focus the input by means of focus(). The original error involves shifting from native to non-native component based on screen width, and this bizarre error appeared only when enlarging the screen from narrow to wide (otherwise it worked as expected). However, in the example, it doesn't work at all and all in all, it's the same error probably.

Codebox: https://codesandbox.io/s/focused-germain-f6fvg?file=/src/App.js

Expected result:
Ideally a successful focus but at least not passing the null check and the value STILL being null despite this.

Actual result:
Program fails because input WAS after all null and Cannot read property focus of null.

Context:
MaterialUI: 4.10.2
React: 16.13.1
React-dom: 16.13.1
Rect-scripts: 3.4.1

Attempting to add the ref to ref in inputProps results in the same problem.

[tchmnn]

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 27446e29a..1ad7eaa28 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -77,7 +77,9 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     handleRef,
     () => ({
       focus: () => {
-        displayNode.focus();
+        if (displayNode) {
+          displayNode.focus();
+        }
       },
       node: inputRef.current,
       value,

What about this?

[eps1lon]

What about this?

This fixes the symptoms (the crash) but not the actual issue (focusing as soon as inputRef is instantiated).

[geoffreyfloyd]

Running into this issue as well, Since the use case is most commonly to focus as soon as the component is rendered, perhaps it could use a setTimeout if the internal ref isn't yet present. In our own codebase we created a util method that checks if the ref exists, and if so runs the callback function supplied as a second param right away, else it will use a setTimeout at 100ms and then callback,

In our experience if it's not yet available after a brief timeout (important that it's shorter than humans could generally notice), then it's likely an implementation issue causing the ref not to get set.

A second advantage of the setTimeout approach is that if it DOES error out it won't crash the React process, it'll just fail to focus.

If you find that there ARE valid cases where a single setTimeout of 100ms (or less) doesn't always work (and the implementation is fine), then you could have it try up to n times at very brief intervals rather than increasing a single timeout so that in most cases users will not notice the delay.

Here's our current internal solution that seems to work just fine. Again, retrying could be a better approach if further testing finds the ref still does not exist in some cases (and the implementation is not flawed).

export function waitOnRef (ref, callback) {
    if (ref) {
        callback();
    }
    else {
        // If ref is still not assigned after brief timeout,
        // then there is almost certainly a deeper issue to solve.
        setTimeout(function () {
            callback();
        }, 100);
    }
}

Example usage:

waitOnRef(displayNode, () => {
    displayNode.focus();
});;

Shameless shout out to my German brethren, i was born in Landstuhl, Germany and lived there til i was 11. Help me get out of this crazy country, lol. Any of you need a software architect?

[oliviertassinari]

@geoffreyfloyd The fix could be as simple as:

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index d8b0c630a2..2d2ce321f5 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -76,9 +76,9 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   React.useImperativeHandle(
     handleRef,
     () => ({
-      focus: () => {
+      focus: displayNode ? () => {
         displayNode.focus();
-      },
+      } : null,
       node: inputRef.current,
       value,
     }),

But that's probably not what we want. Instead, we can support the implicit timing:

--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -67,48 +67,53 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   });

   const inputRef = React.useRef(null);
+  const displayRef = React.useRef(null);
   const [displayNode, setDisplayNode] = React.useState(null);
   const { current: isOpenControlled } = React.useRef(openProp != null);
   const [menuMinWidthState, setMenuMinWidthState] = React.useState();
   const [openState, setOpenState] = React.useState(false);
   const handleRef = useForkRef(ref, inputRefProp);

+  const handleDisplayRef = React.useCallback((node) => {
+    displayRef.current = node;
+
+    if (node) {
+      setDisplayNode(node);
+    }
+  }, []);
+
   React.useImperativeHandle(
     handleRef,
     () => ({
       focus: () => {
-        displayNode.focus();
+        displayRef.current.focus();
       },
       node: inputRef.current,
       value,
     }),
-    [displayNode, value],
+    [value],
   );

   React.useEffect(() => {
-    if (autoFocus && displayNode) {
-      displayNode.focus();
+    if (autoFocus) {
+      displayRef.current.focus();
     }
-  }, [autoFocus, displayNode]);
+  }, [autoFocus]);

   React.useEffect(() => {
-    if (displayNode) {
-      const label = ownerDocument(displayNode).getElementById(labelId);
-      if (label) {
-        const handler = () => {
-          if (getSelection().isCollapsed) {
-            displayNode.focus();
-          }
-        };
-        label.addEventListener('click', handler);
-        return () => {
-          label.removeEventListener('click', handler);
-        };
-      }
+    const label = ownerDocument(displayRef.current).getElementById(labelId);
+    if (label) {
+      const handler = () => {
+        if (getSelection().isCollapsed) {
+          displayRef.current.focus();
+        }
+      };
+      label.addEventListener('click', handler);
+      return () => {
+        label.removeEventListener('click', handler);
+      };
     }
-
-    return undefined;
-  }, [labelId, displayNode]);
+  }, [labelId]);

   const update = (open, event) => {
     if (open) {
@@ -132,7 +137,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     }
     // Hijack the default focus behavior.
     event.preventDefault();
-    displayNode.focus();
+    displayRef.current.focus();

     update(true, event);
   };
@@ -363,7 +368,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
           },
           className,
         )}
-        ref={setDisplayNode}
+        ref={handleDisplayRef}
         tabIndex={tabIndex}
         role="button"
         aria-disabled={disabled ? 'true' : undefined}

Do you want to work on a pull request? Maybe the latter approach would be better? :)

[eps1lon]

@oliviertassinari This fixes symptoms but not the underlying issue. #21443 fixes the underlying issue.

Though we need to test this with browsers. I remember .focus() having no effect during layout phase so it might be better to warn instead if .focus() is called too early.

[oliviertassinari]

I don't understand how is the second proposed diff not solving the underlying issue? The main difference with #21443 seems to be the Popover that can accept ref as anchor. In the past, didn't we say that doing such could lead to issues where the ref isn't resolved in time, which break lead to a layout shift once the ref resolve and the popup can position itself correctly?

So from what I can understand, the best course forward is to take the diff from the previous comment and the tests from #21443.

(I didn't realize an open pull request already existed for it).

[eps1lon]

I don't understand how is the second proposed diff not solving the underlying issue?

I don't understand how you can know that. Your diff does not include any tests. As I said earlier in the conversation and in the PR: this needs careful testing and thought and not just some guards.

[oliviertassinari]

My second proposed diff was tested with the provided codesandbox reproduction* of the issue. I haven't looked further. Note that my first provided diff need to update the reproduction to have it work.

In the past, I had issues with the auto focus logic of native inputs with React in a mobile app project. I recall needing to set them in a timeout to work.

reproduction*: https://codesandbox.io/s/focused-germain-f6fvg?file=/src/App.js

[oliviertassinari]

@eps1lon Regarding the underlying issue. Which one do you have in mind? From what I can understand we have discussed a couple of different layers of interpretation of what's going wrong. From the one at the surface to the deeper ones:

1. Problem: calling ref.current.focus() as soon as it resolves leads to throwing an error. Solution: we can swallow the issue. This is proposed in [Select] Focusing component with inputRef gives null value despite check #21441 (comment). This interpretation of things is rejected by maintainers. ❌
2. Problem: calling ref.current.focus() as soon as it resolves leads to throwing an error. Solution: we can delay the resolution of the ref. This is proposed in [Select] Focusing component with inputRef gives null value despite check #21441 (comment). However, this behavior diverges from a native <select> element, probably not a good idea. This interpretation of things is rejected by maintainers. ❌
3. Problem: displayNode ref resolves too late, after a set state. Solution: we use a ref internally to host the value, instead of a state. This is proposed in [Select] Focusing component with inputRef gives null value despite check #21441 (comment) (second implicit timing option). All good?
4. Problem: calling ref.current.focus() during some phases of the browser rendering cycle has no effect. Solution: warn if called too early. This is proposed in [Select] Focusing component with inputRef gives null value despite check #21441 (comment). However, this behavior diverges from a native <select> element. Shouldn't it be solved independently and more globally, like at the React level?

[reedanders]

I'll work on a pull request based on the latter approach in #21441 (comment).