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

Unable to attach event listener to element if theres already an event listener that stops bubbling #221

Closed
peterbo opened this issue Feb 5, 2020 · 6 comments · Fixed by #222
Labels
Bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@peterbo
Copy link
Contributor

peterbo commented Feb 5, 2020

Per definition: If an element has multiple event handlers on a single event, then even if one of them stops the bubbling, the other ones still execute.

But that doesn't seem to be the case. Is this caused by a certain implementation detail or could this be a non expected behavior?

Example:
<a href="#" class="new-message-button text-uppercase"><span class="short">Nachricht</span><span class="long">Ihre Nachricht an uns</span></a>

The a- element has an event-listener that stops bubbling:
function() { return $(this).parents(".new-message-box").addClass("open"), !1 }

Attaching an "All Links Click" Trigger has no effect. As soon as the first event-listener is deleted, the "All Links Click" trigger starts to fire onclick.

@tsteur
Copy link
Member

tsteur commented Feb 6, 2020

@peterbo The AllLinksTrigger is listening to all clicks on the body so this would maybe explain it? see https://github.com/matomo-org/tag-manager/blob/3.x-dev/Template/Trigger/AllLinksClickTrigger.web.js#L12

I don't think we really want to DOM modifications etc. Wonder how eg GTM works around this or whether they just recommend people to not stop bubbling or whether they do it differently all together. Finding eg https://stackoverflow.com/questions/45751616/google-tag-manager-not-working-on-link-clicks and other posts where they suggest to change stopPropagation to preventDefault but whether the user can actually do this depends on the wanted behaviour.

@peterbo
Copy link
Contributor Author

peterbo commented Feb 6, 2020

@tsteur Hi Thomas, ah, thanks for clarifying! That definitely makes sense.

Google seems to have fixed this issue, but I don't know how complex such a solution can be. They're attaching a Click listener in capture mode to the root element:
capturing

But I'm currently also checking, if bubbling can be re-enabled for the given elements. Just wanted to bring this topic up, because most of the people don't know anything about bubbling/propagation and that can be quite confusing :)

EDIT: Apparently, just the All Elements Click listener is attached in capture mode. The Link Click Event Listener is attached in bubble mode.

@tsteur
Copy link
Member

tsteur commented Feb 6, 2020

That makes total sense and be an easy fix. We'd just need to define the fourth parameter from our addEventListener eg in https://github.com/matomo-org/tag-manager/blob/3.x-dev/Template/Trigger/AllLinksClickTrigger.web.js#L12 but also for similar use cases and set it to true.

@tsteur tsteur modified the milestones: Current sprint, Matomo 4 Feb 6, 2020
@tsteur tsteur added Bug Something isn't working help wanted Extra attention is needed labels Feb 6, 2020
@peterbo
Copy link
Contributor Author

peterbo commented Feb 6, 2020

Perfect, thanks for the feedback, @tsteur! I changed & tested this in my codebase already, but I wasn't quite sure if there would be other implications. But then, it also makes sense that this is only usable for the All Clicks event and not for the All Links Click (because it has to bubble up to the nearest parent A element). So this makes sense and makes things much easier. Thanks!!

@tsteur
Copy link
Member

tsteur commented Feb 6, 2020

and not for the All Links Click (because it has to bubble up to the nearest parent A

I'm probably not understanding the useCapture fully just yet or the problem. Created PR but thought it be safe to do it there as well or can at least not harm?

@peterbo
Copy link
Contributor Author

peterbo commented Feb 7, 2020

Shouldn't be a problem. I think in GTM they wanted to comply with the "standard" and also stop the tracking request once the propagation is stopped. So probably more a philosophical than a technical question.

But mostly stopPropagation is used wrong or unconscious today (mostly in form submits). The developers simply mean preventDefault, but set e.g. a "return false", which also stops the propagation.

So +1 for changing that in all EventListeners or making it configurable as a parameter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants