[fields] Fix input focus when input is not on main document

[louise-davies]

Fixes a bug where MUI datepickers would not accept keyboard interaction when loaded in a popup via React Portal. See bug reproduction code here: https://codesandbox.io/s/mui-datepickers-in-popup-window-via-react-portal-dx83lg?file=/src/App.js:0-3486

Instead of using the global document, instead use the inputRef.current's ownerDocument which accurately reflects the document the input is mounted on. This should have no effect to the "normal" usecase, as then inputRef.current.ownerDocument === document

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Deploy preview: https://deploy-preview-10847--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against d41af63

[LukasTy]

Thank you for your contribution! 🙏
The solution makes sense, but what do you think about also adding the following diff to fix picker focused button management as well as range picker behavior inside of a portal? 🤔

diff --git a/packages/x-date-pickers-pro/src/internals/hooks/useDesktopRangePicker/useDesktopRangePicker.tsx b/packages/x-date-pickers-pro/src/internals/hooks/useDesktopRangePicker/useDesktopRangePicker.tsx
index 030cbc422..6c97db447 100644
--- a/packages/x-date-pickers-pro/src/internals/hooks/useDesktopRangePicker/useDesktopRangePicker.tsx
+++ b/packages/x-date-pickers-pro/src/internals/hooks/useDesktopRangePicker/useDesktopRangePicker.tsx
@@ -90,9 +90,11 @@ export const useDesktopRangePicker = <
 
   const handleBlur = () => {
     executeInTheNextEventLoopTick(() => {
+      const fieldCurrent = fieldContainerRef.current;
+      const popperCurrent = popperRef.current;
       if (
-        fieldContainerRef.current?.contains(getActiveElement(document)) ||
-        popperRef.current?.contains(getActiveElement(document))
+        fieldCurrent?.contains(getActiveElement(fieldCurrent.ownerDocument)) ||
+        popperCurrent?.contains(getActiveElement(popperCurrent.ownerDocument))
       ) {
         return;
       }
diff --git a/packages/x-date-pickers/src/internals/components/PickersPopper.tsx b/packages/x-date-pickers/src/internals/components/PickersPopper.tsx
index 6152aa89c..6390657d0 100644
--- a/packages/x-date-pickers/src/internals/components/PickersPopper.tsx
+++ b/packages/x-date-pickers/src/internals/components/PickersPopper.tsx
@@ -359,7 +359,10 @@ export function PickersPopper(inProps: PickerPopperProps) {
     }
 
     if (open) {
-      lastFocusedElementRef.current = getActiveElement(document);
+      lastFocusedElementRef.current = getActiveElement(
+        lastFocusedElementRef.current?.ownerDocument ??
+          (anchorEl instanceof HTMLElement ? anchorEl?.ownerDocument : undefined),
+      );
     } else if (
       lastFocusedElementRef.current &&
       lastFocusedElementRef.current instanceof HTMLElement
@@ -372,7 +375,7 @@ export function PickersPopper(inProps: PickerPopperProps) {
         }
       });
     }
-  }, [open, role, shouldRestoreFocus]);
+  }, [anchorEl, open, role, shouldRestoreFocus]);
 
   const [clickAwayRef, onPaperClick, onPaperTouchStart] = useClickAwayListener(
     open,

Here is an adjusted codesandbox with showcased range picker.

If you modify the code in the pro codebase, technically, you should sign a CLA agreeing that the code you contributed will be used for commercial purposes, but if you agree that the change on pro codebase is trivial, this requirement can be omitted. 👌

In general, this change makes sense to me, but what do you think @flaviendelangle?

[louise-davies]

@LukasTy I can see that yes in your new sandbox there are also issues with the RangePicker and that your diff looks sensible to me and as you said it's a rather small change

[flaviendelangle]

The fix looks great
Would it make sense to change the signature of getActiveElement (or create another method) to take the inputRef?
With this kind of change, I'm slightly worried that we will forget to use this less-obvious approach.
We still have document.activeElement 😬

[LukasTy]

With this kind of change, I'm slightly worried that we will forget to use this less-obvious approach.

Yes, there is that problem. 🤔
But the change is about resolving the correct document to pass to the getActiveElement, I'm not sure inputRef has anything to do with it. 🤷

We still have document.activeElement 😬

In which case? Data Grid package?

[flaviendelangle]

I'm not sure inputRef has anything to do with it. 🤷

I just want to replace

    if (inputRef.current && inputRef.current === inputRef.current.ownerDocument.activeElement) {

With something higher level like:

  if (isActiveElement(inputRef) {}

To reduce the risk of mistake

In which case? Data Grid package?

https://github.com/mui/mui-x/blob/4295a7f78ef992293834c95f8ce80286b17e0558/packages/x-date-pickers/src/internals/hooks/useField/useField.ts#L462C58-L462C72

[LukasTy]

With something higher level like:

Makes sense. Will look into a possible alternative. 👌 Unless you have a diff to suggest. 🙈

https://github.com/mui/mui-x/blob/4295a7f78ef992293834c95f8ce80286b17e0558/packages/x-date-pickers/src/internals/hooks/useField/useField.ts#L462C58-L462C72

This instance is changed in this PR, isn't it? 🤔

[flaviendelangle]

This instance is changed in this PR, isn't it? 🤔

Yes, my point was that hardcoding those smarter solutions can often lead to someone forgetting to use the smart way when creating new code later on

[cherniavskii]

Could you use ownerDocument util instead?

mui-x/packages/x-date-pickers/src/internals/components/PickersPopper.tsx

Line 184 in 4295a7f

const doc = ownerDocument(nodeRef.current);

UPD: didn't see the comments above, feel free to disregard if a different approach was suggested

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[michelengelen]

This PR has been inactive for a while, so we’re going to close it for now.
If there’s any work here that should be preserved, feel free to reopen as a draft or create a fork. Thanks for your contributions!