[EuiFlyout] Scope close events to mask when ownFocus=true

[thompsongl]

Summary

Restores previous decision to only consider an click event as "outside" when EuiOverlayMask is the target and ownFocus=true.

• EuiOverlayMask accepts a React ref via maskRef
• EuiFlyout takes advantage of ^ to scope outside click events in the case of ownFocus=true (the default case)
• EuiFlyout provides event data to onClose callback (derived in most cases from EuiFocusTrap)

When ownFocus=false and outsideClickCloses=true, we will not scope the close event to any particular element. Use the shards prop to prevent closing when clicking certain external elements.

Checklist

• Checked in Chrome, Safari, Edge, and Firefox
• Props have proper autodocs and playground toggles
• Checked Code Sandbox works for any docs examples
• Added or updated jest and cypress tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5876/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5876/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5876/

[1Copenut]

This should get us exactly what we're needing for the specific cases in Kibana. I appreciate the Cypress specs (with real events!) to test the overlay and EuiToast cases. I'll defer to Constance for the code changes, but wanted to chime in the experience matches a mockup I put together to test UX quickly.

[cee-chen]

Mostly looks great to me! My comments are mainly around the onClickOutside logic and if we're in a hurry can be skipped, but I think it's worth clarifying for developer experience/understanding

[cee-chen]

This still feels pretty hard for me to read personally 😅 Since this is a function now, can we prefer early returns?

const onClickOutside = (event: MouseEvent | TouchEvent) => {
  if (outsideClickCloses === true) return onClose(event);
  if (outsideClickCloses === false) return undefined;
  // If outsideClickCloses is not specified, only close on clicks to the overlay mask
  if (isDefaultConfiguration && event.target === maskRef) return onClose(event);
  // Otherwise if ownFocus is false, outside clicks should not close the flyout
  return undefined;
}

[thompsongl]

I'll refactor for readability, but here's the logic:

• If ownFocus=true (default), only clicks on the mask should close the flyout, even if outsideClickCloses=true
• If ownFocus=false, the flyout will only close if outsideClickCloses=true, and in that case clicking any external element will close the flyout

[cee-chen]

If ownFocus=true (default), only clicks on the mask should close the flyout, even if outsideClickCloses=true

That's just not what the current branching is doing though?? So you'd need to change this logic if that's the behavior you want. outsideClickCloses === true will always evaluate to returning onClose(). Am I reading this the same way as you?

[thompsongl]

const onClickOutside = (event: MouseEvent | TouchEvent) => {
  // Do not close the flyout for any external click
  if (outsideClickCloses === false) return undefined;
  if (isDefaultConfiguration) {
    // The overlay mask is present, so only clicks targeting the mask should close the flyout
    if (event.target === maskRef.current) return onClose(event);
  } else {
    // No overlay mask is present, so any outside clicks should close the flyout
    if (outsideClickCloses === true) return onClose(event);
  }
  // Otherwise if ownFocus is false, outside clicks should not close the flyout
  return undefined;
};

[thompsongl]

That's just not what the current branching is doing though??

Yep I had the conditional wrong. #5876 (comment) is correct; will push the change once we get confirmation from @cchaos

[cee-chen]

@thompsongl Nice! Definitely much easier to grok now, especially w/ comments. For the last comment/return, I might make it a bit more specific to help follow the possible branch paths:

// Otherwise if ownFocus is false and outsideClickCloses is undefined, outside clicks should not close the flyout

[cee-chen]

I also think we should be more specific about the isDefaultConfiguration mask behavior:

// The overlay mask is present, so only clicks on the mask should close the flyout, regardless of outsideClickCloses

It's not that the business logic/UX doesn't make sense, it's just that it's a bit tortuous so more comments should help with that (I think?)

[cchaos]

Oh man, I remember how tricky this logic was. I'm going to repeat the logic just to ensure I understand.

• Default ownFocus = true: The overlay mask renders and the flyout will only be closed when clicking mask (or close button)
• ownFocus = false && outsideClickCloses = false: No overlay mask and can only be closed via close button (or some other in-page toggle)
• ownFocus = false && outsideClickCloses = true: Clicking anywhere outside will close flyout (does this include dragging with mouse up or does this now mean a special extra configuration?)

I feel like that's how it was before, but maybe I'm remembering another config wrong?

[thompsongl]

ownFocus = false && outsideClickCloses = true: Clicking anywhere outside will close flyout (does this include dragging with mouse up or does this now mean a special extra configuration?)

By default the close event will happen if mousedown happens outside the flyout. Consumers can change this to occur on mouseup if they have some reason to do so.
By default the close event will not happen if mousedown happens inside the flyout, even if mouseup happens outside the flyout (e.g., mousedown inside, drag outside, release. The flyout remains open)

[thompsongl]

43ce709 incorporates all suggestions in this thread

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5876/

[thompsongl]

I think this name change will be super helpful for understanding

[cee-chen]

🤩 ++++++ this is great!!

[cee-chen]

Pulled down collapsible nav to QA locally and that's now working great as well.

[thompsongl]

Thanks as always for the thorough review, @constancecchen!

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5876/