Fix clickaway bugs

[Will-Howard]

This is the specific bug this is fixing (before), there were a couple of other bugs like this which have already been worked around:

Screen.Recording.2024-05-08.at.09.05.12.mp4

The fix in this PR is essentially this line:

   // Events from MUI form components have the base event under the "nativeEvent" field
   bubbledEventTarget.current = ('nativeEvent' in event) ? event.nativeEvent.target : event.target;

There are also some changes related to how LWClickAwayListener interacts with portals (where an element is created at the root level of the document rather than in the place it is instantiated) to hopefully make it a bit more robust.

Details of the bubbledEventTarget fix

This is the bit of code that handles document-level click events (i.e. events anywhere on the page), to decide whether they count as a clickaway for the particular element that is wrapped in LWClickAwayListener:

const handleEvents = (event: ClickAwayEvent): void => {
      if (!mountedRef.current) return;

      const isContainedByNode = node.current && node.current.contains(event.target as Node);
      const isBubbledEventTarget = bubbledEventTarget.current === event.target;
      const notContainedInDocument = !nodeDocument.contains(event.target as Node);

      if (isContainedByNode || isBubbledEventTarget || notContainedInDocument) {
        return;
      }

      onClickAway(event);
    };

isContainedByNode is checking whether the clicked element is a child of the LWClickAwayListener wrapped element in the DOM tree (i.e. in the actual html), isBubbledEventTarget is supposed to check whether the clicked element is a child of the wrapped element in the React tree.

In the case where portals (Dialogs, Poppers) are involved as the wrapped element, these can be different, because a portal creates the element at the root level of the document, so it won't be a DOM-child of the LWClickAwayListener. It will be a React-child however, because portals are designed to maintain the logical tree structure by bubbling events up through nested portals even if they end up separated in the DOM.

The bug was that clicks on various MUI components generate a synthetic event that doesn't have a proper target field, so bubbledEventTarget.current wouldn't be set properly and isBubbledEventTarget would wrongly be false. Hence this change to use the "nativeEvent" in that case:

   // Events from MUI form components have the base event under the "nativeEvent" field
   bubbledEventTarget.current = ('nativeEvent' in event) ? event.nativeEvent.target : event.target;

Details of the other changes related to portals

Given the fix above, the case where isContainedByNode is false, but isBubbledEventTarget is true (sufficient to correctly not trigger the clickaway) would be in situations like this where there is a LWClickAwayListener outside a portal-generating component (in this case LWPopper):

      {isOpen && <LWClickAwayListener onClickAway={onClickAway}>
        <LWPopper
          open={isOpen}
          anchorEl={anchorEl}
          className={classes.popper}
          clickable={true}
          allowOverflow={true}
          placement={"bottom-start"}
        >
          <SideCommentHover post={post} commentIds={commentIds} />
        </LWPopper>
      </LWClickAwayListener>}

The LWPopper will be portal-ed away somewhere else so won't be a DOM child of the LWClickAwayListener. I've changed all cases like this to instead have the LWClickAwayListener inside the portal-generating component like so:

    <LWPopper
        open={isOpen} anchorEl={anchorEl}
        className={classes.popper}
        clickable={true}
        allowOverflow={true}
        placement={"bottom-start"}
      >
      <LWClickAwayListener onClickAway={onClickAway}>
        <SideCommentHover post={post} commentIds={commentIds}/>
      </LWClickAwayListener>
    </LWPopper>

This way it's always the case that the clickaway-able component is both a DOM child and a React child of the clickaway listener. This should make things easier to think about, and also potentially prevent bugs in future where there is some issue with the click event being bubbled all the way up to LWClickAwayListener, because it will then just fall back to the isContainedByNode in most cases (except when there are also nested portals involved). This would have independently prevented the specific bug in the video above.

...

Separately, there are also various MUI components that create portals (e.g. Dialog, Select), and these have their own way of handling clickaways for themselves[1]. In the case of useDialog we were adding a LWClickAwayListener (controlled by noClickawayCancel) on top of the clickaway already provided by the MUI Dialog. I thought it was better to leave it to the default MUI behaviour in that case, so I have removed the noClickawayCancel option and made it so the dialog is responsible for disabling the clickaway itself (by not passing an onClose prop to the dialog).

[1] The logic in this case appears to be that only the topmost MUI portal element will ever be considered for a clickaway. So if there is a Select open over a Dialog the first clickaway will only close the Select even if it's outside the Dialog bounds also.

┆Issue is synchronized with this Asana task by Unito

[Will-Howard]

Note that the onClickAway function above doesn't actually close the popper unless the click is on the button to do so. This seems like a roundabout way of doing it, but it's how it was before

[Will-Howard]

This was to fix a bug with choosing a custom date range in the author analytics page (which would close the modal as soon as it was opened). I'm not sure exactly what fixed that but it has now gone away

[Will-Howard]

This was the one case where removing the external clickaway required changing something in a dialog component. All the others already had both the external LWClickAwayCancel and this onClose which would independently close the dialog

[Will-Howard]

This one I didn't test, I'm fairly confident it will work based on all the others

[Will-Howard]

When Sarah and I were testing we ran into some cases where focus events would wrongly trigger the clickaway. I'm not sure if these would still happen with the other bug fixes, but I don't think we benefit from having clickaways triggered on focus events anywhere so I decided to remove them to avoid future problems

[oetherington]

Nice - LGTM!