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 inputs in shadow DOM in disableOnInputFields mode #578

Merged
merged 1 commit into from
Jan 22, 2022

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Jan 19, 2022

I tried #557 with the new release, but it failed for us due to a special circumstance in our particular app: we embed it into a 3rd party pages, and for better encapsulation we wrap the whole app inside a shadow DOM.

This however comes with some side effects, also encountered in other places, especially due to the way events are handled across shadow boundaries: when the key event traverses the shadow root, its event.target is not anymore the (input) element the event originated from, but the shadow root (to "hide" the internals of the shadow DOM for the outer DOM). So now it's tagName is not INPUT anymore, breaking the input/textarea detection.

ℹ️ Here is a good explanation of events and shadow DOM, if someone is interested: https://pm.dartus.fr/blog/a-complete-guide-on-shadow-dom-and-event-propagation/

This PR tweaks the input detection just a little bit, to also support inputs inside a (open!) shadow DOM, by looking for the "real" event target using compoundPath.

Note that the change here is not only related to our particular use case of having the whole app wrapped in a shadow DOM. It should work also in the more general case, e.g. when rendering a custom element that contains an input. This scenario is replicated in the test I added here!

cc @st-h

@@ -70,10 +70,10 @@ export default class KeyboardService extends Service {
@action
_respond(event) {
if (this._disableOnInput && event.target) {
const tag = event.target.tagName;
const target = event.composedPath()[0] ?? event.target;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that composedPath is supported in all browsers except IE. But I believe this addon does not support IE anyway, right? Otherwise we could add a better check and fall back to event.target if not supported...

@lukemelia
Copy link
Collaborator

Looks good @simonihmig. Thanks for your work on this. Should we also have a positive test case of an event being handled properly in a shadow DOM situtation? I would hate to accidentally break this in a future change.

@simonihmig
Copy link
Contributor Author

Should we also have a positive test case of an event being handled properly in a shadow DOM situtation?

Sure, I can do that. I would submit a separate PR, if you agree, as this is not so much about the fix here (specific to detecting inputs), ok?

Someone still needs to enable the CI workflow to run btw! 😉

@SergeAstapov
Copy link
Contributor

@simonihmig CI is green!
I would defer to @lukemelia to land this and agree that happy path scenario is needed sooner than later

@lukemelia lukemelia merged commit 047c125 into adopted-ember-addons:master Jan 22, 2022
@lukemelia
Copy link
Collaborator

Thanks @simonihmig and thanks for reviewing @SergeAstapov!

@simonihmig simonihmig deleted the support-shadowdom branch January 24, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants