@flamisz opened this Pull Request on May 5th 2021 Contributor

Description:

fixes #15780

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@flamisz commented on May 6th 2021 Contributor

build js

@flamisz commented on May 6th 2021 Contributor

build js

@diosmosis commented on May 7th 2021 Member

I see how this works, using the root element to always handle clicks (and it's definitely simpler), but I think there's potentially an issue if event bubbling is cancelled (ie, via stopPropagation()) somewhere along the way. In that case we might not track any outlinks. Is this something we want to handle @tsteur?

@tsteur commented on May 10th 2021 Member

@diosmosis that's something we can't prevent but we set the third parameter in addEventListener to true so we should be receiving the event as they are usually not cancelled in the useCapture mode.

From https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

A Boolean indicating whether events of this type will be dispatched to the registered listener before being dispatched to any EventTarget beneath it in the DOM tree

it basically starts in the html/body elements and then goes down before it would bubble up again AFAIK. And usually someone might stop propagation only at a later stage (they have default useCapture=false). It works quite well eg in TagManager etc and it shouldn't be a problem

@diosmosis commented on May 10th 2021 Member

Tested locally, works well. Created https://github.com/matomo-org/test-examples/pull/7 for testing.

Also left one small comment, otherwise looks good.

@tsteur commented on May 11th 2021 Member

fyi because the 4.3 release is soon and the RC is already out we might want to merge that one only after the 4.3 release just in case anything is breaking etc. And then we could directly update the SPA guide etc to mention the method no longer needs to be called etc

@diosmosis commented on May 11th 2021 Member

@tsteur should we change the milestone?

@tsteur commented on May 11th 2021 Member

Changed it for now 👍 Hoping it won't be later renamed to 4.5.0 after releasing 4.3. Then we would change the milestone again.

I could create a 4.3.1 milestone but there we'd mostly want fixes and not changes in there.

@flamisz commented on May 11th 2021 Contributor

build js

Powered by GitHub Issue Mirror