Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change EuiOutsideClickDetector to use mousedown and mouseup events #1761

Merged
merged 4 commits into from
Mar 26, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Mar 22, 2019

Summary

#1747 raises the potential need to move away from click events in certain cases (EuiOutsideClickDetector) due to browser vendors moving closer to the event target spec. When mousedown and mouseup target different elements, Chrome 73 now returns the closest common ancestor as its target. Other browsers may soon follow (see below)

References:

Checklist

  • This was checked in mobile
  • This was checked in IE11

- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately

- [ ] This was checked for breaking changes and labeled appropriately

  • Jest tests were updated or added to match the most common scenarios

- [ ] This was checked against keyboard-only and screenreader scenarios
- [ ] This required updates to Framer X components

@thompsongl
Copy link
Contributor Author

@chandlerprall Thoughts on just moving forward with this regardless of what comes from the Chromium issue? They've labelled it WontFix and have indicated that more browsers will move toward this interpretation of the spec

@chandlerprall
Copy link
Contributor

Yeah, I think this is the right thing to do.

@thompsongl thompsongl marked this pull request as ready for review March 25, 2019 15:35
@snide
Copy link
Contributor

snide commented Mar 25, 2019

Will we have to backport this? Potentially to older versions of Kibana?

@chandlerprall
Copy link
Contributor

Will we have to backport this? Potentially to older versions of Kibana?

That seems like a good idea. We'll have to manually backport only the changes to EuiOutsideClickDetector and re-update snapshots as EuiFocusTrap doesn't exist in 6.10.5

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; pulled locally and tested that changes to EuiOutsideClickDetector keep functionality in Chrome 72 and have the same functionality after I updated to Chrome 73.

@thompsongl
Copy link
Contributor Author

@snide How far back would we need to go?

@snide
Copy link
Contributor

snide commented Mar 25, 2019

@thompsongl I'll follow up with @chandlerprall. Depending on how bad the issue is we might need to go pretty far back :/

@thompsongl
Copy link
Contributor Author

I think the original issue describes the most common case (that I can think of) pretty well: highlighting text in a popover or flyout and having the mouseup even happen on a different DOM element than mousedown.

It's hard to know how common that action & result are

@snide
Copy link
Contributor

snide commented Mar 25, 2019

@thompsongl Given that it's a smaller issue that doesn't break UI (just makes it slightly annoying) I think we should be OK applying it to 6.7.x and 7.0.x kibana. We can monitor github for reports on older versions to see if it's a super large annoyance to people.

@thompsongl thompsongl merged commit 16edaac into elastic:master Mar 26, 2019
Shigawire pushed a commit to Shigawire/eui that referenced this pull request May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants