[FocusTrap] Fix disableEnforceFocus behavior

[mnajdova]

Codesandbox with the broken demo from #38684: https://codesandbox.io/s/condescending-dan-xmxsxw?file=/Demo.tsx

Fixes #21569

Before: https://mui.com/base-ui/react-focus-trap/#disable-enforced-focus
After: https://deploy-preview-38816--material-ui.netlify.app/base-ui/react-focus-trap/#disable-enforced-focus

[mui-bot]

Netlify deploy preview

https://deploy-preview-38816--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 2d4f3e6

[mnajdova]

I spend way too much trying to test this with unit test, for some reason it just don't work. Intuitivly I was thinking to add another test case regarding clicking, basically replicating one of the existing tests:

    it('should not return focus to the children when disableEnforceFocus is true on click outside', () => {
      const { getByTestId } = render(
        <FocusTrap open disableEnforceFocus>
          <div tabIndex={-1}>
            <input autoFocus data-testid="auto-focus" />
          </div>
        </FocusTrap>,
        // TODO: https://github.com/reactwg/react-18/discussions/18#discussioncomment-893076s
        { strictEffects: false },
      );
  
      expect(getByTestId('auto-focus')).toHaveFocus();

      act(() => {
-       initialFocus!.focus();
+       initialFocus!.click();
      });
  
      expect(initialFocus).toHaveFocus();
    });

however, this doesn't work. In the end I added one e2e test, there are other related to the FocusTrap already (I verified that the newly added test fails on master, just to be sure that I am not going crazy 😄)

[oliviertassinari]

Would this fix work?

diff --git a/packages/mui-base/src/FocusTrap/FocusTrap.tsx b/packages/mui-base/src/FocusTrap/FocusTrap.tsx
index 2889f118ba..67f0419fa4 100644
--- a/packages/mui-base/src/FocusTrap/FocusTrap.tsx
+++ b/packages/mui-base/src/FocusTrap/FocusTrap.tsx
@@ -212,7 +212,7 @@ function FocusTrap(props: FocusTrapProps): JSX.Element {
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [open]);

-  const contain = React.useCallback((nativeEvent: FocusEvent | null) => {
+  const contain = React.useCallback((nativeEvent: FocusEvent | null | false) => {
     const rootElement = rootRef.current;
     const doc = ownerDocument(rootRef.current);

@@ -225,13 +225,16 @@ function FocusTrap(props: FocusTrapProps): JSX.Element {
     if (
       !doc.hasFocus() ||
       !isEnabled() ||
-      disableEnforceFocus ||
       ignoreNextEnforceFocus.current
     ) {
       ignoreNextEnforceFocus.current = false;
       return;
     }

+    if (disableEnforceFocus && nativeEvent !== false) {
+      return;
+    }
+
     // The focus is already inside
     if (rootElement.contains(doc.activeElement)) {
       return;
@@ -347,6 +350,8 @@ function FocusTrap(props: FocusTrapProps): JSX.Element {
       nodeToRestore.current = event.relatedTarget;
     }
     activated.current = true;
+
+    contain(false);
   };

   return (

I tweaked a bit your playground:

Screen.Recording.2023-09-06.at.21.24.52.mov

All the existing tests and the one added here seem to pass. I didn't understand why we needed this much logic, so explored a bit how it could be solved. One detail though, this diff relies on a refactoring of the logic (moving code only, keeping the same behavior): https://github.com/oliviertassinari/material-ui/commits/foo-2.

[mnajdova]

The diff seems to work. I was debugging step by step and the logic makes sense. Two things

• ✅ I am looking into adding some comments now that I am a bit deep in the logic, to make my life (and hopefully other's) easier long term :D

• I personally don't like that we are using the same function for three different purposes, and we depend on the nativeEvent being false or null for branching. We have these three use-cases

  • interval with check if the focus is on the body
  • document's focusin event handler
  • now a third place - sentinel is focused

  I will try to restructure a bit the code to see if I can make it easier to digest in a follow up PR

[mnajdova]

@oliviertassinari I changed a bit your logic, no need to invoke contain again with a different arg, we can just check if the currently active element is one of the sentinel nodes and return earlier based on this. As far as I could test this works too. In my opinion this yeilds better readibility, you can immediatelly understand the condition instead of having to check where the method was invoked with a nativeEvent being false.

[michaldudak]

This seems to be working well. Good job adding the comments to clarify what the code is doing!

[oliviertassinari]

no need to invoke contain again with a different arg, we can just check if the currently active element is one of the sentinel nodes and return earlier based on this.

@mnajdova Oh, I missed that, smart 👍.

I personally don't like that we are using the same function for three different purposes

I don't know, personally, I like that it's all centered in a single place, restoring the focus or not depending on the case.

see if I can make it easier to digest

I suspect we can remove this logic by using React.useLayoutEffect, the cleanup will be synchronous.

      // Cleanup functions are executed lazily in React 17.
      // Contain can be called between the component being unmounted and its cleanup function being run.
      if (rootElement === null) {
        return;
      }

It also sounds more accurate to avoid a painting with the focus on the wrong location, it could potentially be a flash.

[mnajdova]

I don't know, personally, I like that it's all centered in a single place, restoring the focus or not depending on the case.

Agree, I mentioned this when we added it third time with different arg, it's removed now, so let's keep as is.

I suspect we can remove this logic by using React.useLayoutEffect, the cleanup will be synchronous.

I will check this and include it in a follow up PR if it doesn't break anything.

---

Another thing I want to check is if we can avoid using both nativeElement.target and doc.activeElement, we should be able to always use the doc.activeElement, I am not sure if there is a situation where doc.activeElement wouldn't be the correct node.

I am merging this one, seems like everything is good now.