eui

eui copied to clipboard

Summary

This PR fixes the issue discovered here https://github.com/elastic/kibana/pull/128239#discussion_r863663964 In React@17 there were changes made to the Event delegation described here https://reactjs.org/blog/2020/08/10/react-v17-rc.html#changes-to-event-delegation In Canvas/Lens app we are using stopPropagation() which in React@17 prevents EuiFocusTrap, EuiOutsideClickDetector to receive the events, so that PR aims to fix it by adding { capture: true } to force the event listeners to receive the event despite the stopPropagation()

Checklist

• [ ] Checked in both light and dark modes
• [ ] Checked in mobile
• [x] Checked in Chrome, Safari, Edge, and Firefox
• [ ] Props have proper autodocs and playground toggles
• [ ] Added documentation
• [ ] Checked Code Sandbox works for any docs examples
• [x] Added or updated jest and cypress tests
• [ ] Checked for breaking changes and labeled appropriately
• [ ] Checked for accessibility including keyboard-only and screenreader modes
• [ ] Updated the Figma library counterpart
• [x] A changelog entry exists and is marked appropriately

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

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

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

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

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

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

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

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

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

Makes sense given the use case, but I'm still trying to work out whether there are further/unintended consequences. Anything coming to mind for you, @chandlerprall?

It wouldn't surprise me if there's an app somewhere that has a stopPropagation that this could break, but the components touched looks like a good set - and it reinforces eui's intention for these events.

I don't think it makes sense to move the resize or scroll events into the capture phase?

I'm also a little worried about possible unintended changes as a result of this fix. I'm wondering if it makes more sense to make this a prop that sets the { capture: true } options, e.g. <EuiFocusTrap isCapture />? So defaulting to non-capture and only adding a capture option for specific consumers that need that behavior?

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

Thanks for the isCapture prop update Patryk, the new changes look great.

Could we potentially scope this PR down further to only affect specific desired components that you need? For example:

• EuiFocusTrap <-- per your PR description, you need this for Lens, so this makes sense to add
• EuiOutsideClickDetector - same as above
• EuiPopover <-- I'm not sure this makes sense to add - we always want capture to be true for scroll events, and not allow false. I'm also concerned about side effects here since EuiPopover is used so widely across our components. Is this something your app needs or could we consider removing this from the PR?
• EuiSelectable <-- I'm also not sure this makes sense to add. Does your app also need this?
• EuiOverlayMask <-- we've had issues in the past with click events on our overlay mask that Greg had to fix, and I'm a little worried about this potentially reintroducing bugs. @thompsongl do you mind taking a look/verifying that this is safe to use?
• EuiWindowEvent <-- I like this addition a lot as a more generic approach! Huge ++

Let me know what you think!

EuiOverlayMask <-- we've had issues in the past with click events on our overlay mask that Greg had to fix, and I'm a little worried about this potentially reintroducing bugs.

Willing to hear arguments against, but I think that EuiOverlayMask should no longer accept an onClick prop. The intention was to give an easy on-outside-click hook, but this is now inconsistent with our other approaches to outside click behavior, which have largely been standardized.

If it's necessary to unblock your React 17 PR, we can add the isCapture prop or prioritize the breaking change to remove onClick.

The suggestion would then be to use EuiFocusTrap in conjunction with EuiOverlayMask as we do in several components (e.g., EuiFlyout and EuiCodeBlock):

<EuiOverlayMask>
  <EuiFocusTrap onClickOutside={onClick} />
</EuiOverlayMask>

I'll open an issue for ^ reglardless

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

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

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

@thompsongl I believe so, if I recall correctly we need that for Lens flyout

@patrykkopycinski can you be a bit more specific about what in the Lens flyout requires either an EuiPopover or EuiSelectable to have a configurable capture event? Is there a popover or selectable in the flyout that's causing issues? I peeked at the linked Kibana issue and it looks like it's just EuiFocusTrap and EuiOutsideClickDetector mentioned in the comments.

Hey @patrykkopycinski I see that https://github.com/elastic/kibana/pull/128239 has already merged. Are these EUI changes still wanted/required?

Hey @patrykkopycinski - we're closing this PR due to staleness, but if you still need this behavior, please feel free to re-open this PR without changes to EuiPopover or EuiSelectable (historically some of our more fragile components that have caused a lot of downstream issues with unexpected changes).