[ClickAwayListener] Refactor click-away detection

[eps1lon]

Initial plan was adding a test for #20409

I originally thought we should rather check node.isConnected instead of looping through the composed path and realized that documentElement.contains isn't a meaningful check since it can only be false if:

1. event.target is not in the same frame as nodeRef.current
2. event.target is not connected?

Point 1 is impossible since the event handler is attached to the ownerDocument of nodeRef.current.

Point 2 is only an assumption from #20409. I need to check if the new test was failing prior to #20409 and obviously verify the new build on the sandbox in #20197

Edit:

• Still passes [ClickAwayListener] failed to rasing onClickAway if you have an element that is removed from DOM when clicked #20197: https://codesandbox.io/s/icy-waterfall-oxede?file=/src/Demo.js

[oliviertassinari]

@eps1lon I see what you are doing here, you want to incentive me to provide a failing test case 😄. You are right, the linked pull request misses a test case, if not two. My bad for merging it too quickly. Here is it:

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js
index 96585ad50..cf360e9b1 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js
@@ -5,7 +5,7 @@ import { createClientRender, fireEvent, screen } from 'test/utils/createClientRe
 import Portal from '../Portal';
 import ClickAwayListener from './ClickAwayListener';

   const render = createClientRender();

   it('should render the children', () => {
@@ -259,5 +259,23 @@ describe('<ClickAwayListener />', () => {

       expect(handleClickAway.callCount).to.equal(0);
     });
+
+    it(`${eventName} does not trigger onClickAway if an inside target is removed and disableReactTree`, () => {
+      const handleClickAway = spy();
+      function Test() {
+        const [buttonShown, hideButton] = React.useReducer(() => false, true);
+
+        return (
+          <ClickAwayListener onClickAway={handleClickAway} disableReactTree>
+            <div>{buttonShown && <button {...{ [eventName]: hideButton }} type="button" />}</div>
+          </ClickAwayListener>
+        );
+      }
+      render(<Test />);
+
+      screen.getByRole('button').click();
+
+      expect(handleClickAway.callCount).to.equal(0);
+    });
   });
 });

I hope it helps. It was meant to better support disableReactTree.

[eps1lon]

You are right, the linked pull request misses a test case, if not two. My bad for merging it too quickly. Here is it:

I already added tests for the linked PR. Your test is a new one since disabelReactTree wasn't available when #20197 was reported.

[eps1lon]

Could somebody please add documentation for disableReactTree? The name is not descriptive and the documentation simply copied from another prop.

[oliviertassinari]

What do you think about this description for the prop?

  /**
   * If `true`, the React tree is ignored, only the DOM tree is considered
   * for detecting click outside of the child.
   * This prop changes how portaled elements are handled.
   */
  disableReactTree: PropTypes.bool,

[eps1lon]

Why was this even introduced? It was introduced in a PR that linked an issue in which we decided to not do it:

We should delay the introduction of the disableReactTree prop, it would be great to wait for the outcome of facebook/react#11387 to settle on this prop. We are working on the edges of React, it's better to maximize for future flexibility. We want to reduce the chance of introducing a breaking change in the future.

We could've had a nice transition to React Scope API but now we have a feature that cannot work with it. What a mess.