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

When clicking an element within a shadow DOM, use it as the target instead of the shadow root #544

Conversation

multikoop
Copy link
Contributor

@multikoop multikoop commented Sep 3, 2022

Description:

This code change enhances the AllElementsClickedTrigger to fire event on the actual clicked target when inside an (open) shadow dom.
Fixes: #543

Review

@MatomoForumNotifications

This pull request has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/event-tracking-inside-webcomponents-shadowdom/47205/2

@snake14 snake14 changed the title #543 use the clicked target when in shadow dom When clicking an element within a shadow DOM, use it as the target instead of the shadow root Sep 4, 2022
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Successfully tested this change and it looks good to me. Any concerns @AltamashShaikh ?

@AltamashShaikh
Copy link
Contributor

@snake14 Changes look good, but we need to add this in other click triggers too AllDownloadsClickTrigger.web.js, AllLinksClickTrigger.web.js,

@justinvelluppillai
Copy link
Contributor

Is it going to work in older browsers?

@snake14
Copy link
Contributor

snake14 commented Sep 5, 2022

@justinvelluppillai That's a good point. According to Can I use, it's supported by pretty much all newer browsers. So, it doesn't look like it would work for IE or some older browsers. Is that a deal breaker?

@justinvelluppillai
Copy link
Contributor

This line of code would just need to not run and not break or error for older browsers, so just making sure it's adequately handling that is the key.

@snake14
Copy link
Contributor

snake14 commented Sep 5, 2022

@justinvelluppillai I would expect target.shadowRoot to evaluate as undefined for older browsers. So, I think it would be alright.

@multikoop
Copy link
Contributor Author

Thanks for the discussions. Yes I also was thinking of an enhancement for other relevant triggers (like mentioned by @AltamashShaikh ) Since the AllDownloadsClickTrigger the clickCallBack has advanced implementation I do not feel experienced enough with the needed code change. Maybe someone else.... :-)

@multikoop
Copy link
Contributor Author

As mentioned by @AltamashShaikh I have added the code change (to deal with the target element inside shadow dom) to AllDownloadsClickTrigger.web.js and AllLinksClickTrigger.web.js. so that the change is consistent in available All*ClickTriggers

@multikoop
Copy link
Contributor Author

Hhhm, any idea why 1 build job failed?

@snake14
Copy link
Contributor

snake14 commented Sep 5, 2022

@multikoop Yep. It looks like there was a random server error during one of the specific builds. I restarted it and it ran successfully this time. It's all good now.

Copy link
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

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

@multikoop Left 1 minor comment to make code more readable

@AltamashShaikh AltamashShaikh added this to the Current sprint milestone Sep 6, 2022
@AltamashShaikh AltamashShaikh merged commit 42639c7 into matomo-org:4.x-dev Sep 6, 2022
@AltamashShaikh
Copy link
Contributor

@multikoop Thank you for your contribution, your changes should go live with release of Matomo 4.12

@multikoop
Copy link
Contributor Author

Thanks for the discussion and accepting the contribution. Looking forward to the release 🤩

@innocraft-automation innocraft-automation removed this from the Current sprint milestone Feb 16, 2023
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.

Handle Click Events correctly in shadowDOM (web components)
6 participants