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

Support clicking inside drops in a shadow root #4241

Closed
wants to merge 5 commits into from

Conversation

brentropy
Copy link
Contributor

@brentropy brentropy commented Jun 29, 2020

If a drop is rendered inside a shadow root, clicks in the drop were still triggering onClickOutside. This is is because the host element for the shadow root is the target on the document mousedown event making it appear to be outside of the drop.

What does this PR do?

When a shadow root is attached in open mode, it is possible to get the real target by using event.composedPath(). This change uses composedPath to get the target if it is supported, and gracefully falls back to event.target when it isn't supported.

What testing has been done on this PR?

This has been tested in storybook with components that use a drop with onClickOutside and in my application that uses drops in a shadow root. This commit also includes a new test to cover this.

How should this be manually tested?

The biggest thing is ensuring no existing functionality with onClickOutside is broken. If you would like to test with a shadow root you can set up a containerTarget that is in a shadow root and render a Menu making sure onClick for the items is still triggering.

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

Maybe?

Is this change backwards compatible or is it a breaking change?

Yes

If a drop is rendered inside a shadow root, clicks in the drop were
still triggering onClickOutside. This is is because the host element
for the shadow root is the target on the document mousedown event
making it appear to be outside of the drop.

When a shadow root is attached in open mode, it is possible to get
the real target by using event.composedPath(). This change uses
composedPath to get the target if it is supported, and gracefully
falls back to event.target when it isn't supported.

Signed-off-by: Brent Burgoyne <brent47@gmail.com>
@brentropy
Copy link
Contributor Author

It looks like chromatic picked up some imperceptible text rendering changes with my latest merge of master. When that happens do you usually just accept them or do you re-trigger it?

@ShimiSun
Copy link
Member

ShimiSun commented Jul 4, 2020

Thanks buddy! The branch wasn't up to date with the master branch, once I've merged it with master, it resolved the Chromatic changes.

Copy link
Member

@ShimiSun ShimiSun left a comment

Choose a reason for hiding this comment

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

Thank you for this PR and for helping us improve the library.
Overall, I understand the rationale of this PR, and I agree with the motivation to fix the issue you presented.

That being said, reading through the composePath functionality, I noticed it doesn't support the IE browser, as much as we would love to drop the support for IE, we do have users that are relying on it and this change will be perceived as a regression.
image

Any ideas or alternatives would be more than welcome.

@brentropy
Copy link
Contributor Author

Hi @ShimiSun. Thanks for the review. I definitely understand the need to maintain support for IE. In this case I handled that by falling back to event.target if event.composedPath() is not supported. IE would continue working exactly as it is right now. Since IE doesn't support shadow dom anyway, the original bug is a non-issue for applications that need to support IE 11.

@ShimiSun
Copy link
Member

ShimiSun commented Jul 9, 2020

We are trying to learn about root shadow and see how it would be used in the context of grommet.
Can you please share a codesandbox that reproduces the problem you are trying to solve for us to learn how one might use the library?

@brentropy
Copy link
Contributor Author

@ShimiSun sorry for taking a while to get back to you. I will get something put together.

@brentropy
Copy link
Contributor Author

Here is an example that demonstrates this specific bug, and the style isolation advantage of using shadow dom.

Example

I am working on a browser extension that injects UI in to any page. Using shadow dom has made it easy to not have to deal with trying to reset everything and fight specificity wars.

@brentropy
Copy link
Contributor Author

brentropy commented Jul 27, 2020

Hi @ShimiSun, is there anything else I can help with on this?

@ShimiSun
Copy link
Member

Thanks for the reminder @brentropy , and for your patience. I believe it is on us at this point.

@jcfilben
Copy link
Collaborator

Hey @brentropy sorry this pull request has been sticking around for a while, just wanted to check in and see if there was still interest in getting this merged

@jcfilben
Copy link
Collaborator

jcfilben commented Oct 5, 2021

I'm going to close this PR for now. If there is still interest feel free to reopen or add a comment.

@jcfilben jcfilben closed this Oct 5, 2021
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.

None yet

3 participants