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

Outlink tracking when DOM modified #17522

Merged
merged 8 commits into from May 27, 2021
Merged

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented May 5, 2021

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 flamisz marked this pull request as ready for review May 5, 2021 03:59
@flamisz flamisz self-assigned this May 5, 2021
@flamisz flamisz added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 5, 2021
@flamisz flamisz added this to the 4.3.0 milestone May 5, 2021
@flamisz flamisz marked this pull request as draft May 5, 2021 04:40
@flamisz flamisz removed the Needs Review PRs that need a code review label May 5, 2021
@flamisz
Copy link
Contributor Author

flamisz commented May 6, 2021

build js

@flamisz
Copy link
Contributor Author

flamisz commented May 6, 2021

build js

@flamisz flamisz marked this pull request as ready for review May 6, 2021 03:32
@flamisz flamisz added the Needs Review PRs that need a code review label May 6, 2021
@diosmosis
Copy link
Member

diosmosis commented May 7, 2021

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
Copy link
Member

tsteur commented May 10, 2021

@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

js/piwik.js Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member

Tested locally, works well. Created matomo-org/test-examples#7 for testing.

Also left one small comment, otherwise looks good.

@tsteur
Copy link
Member

tsteur commented May 11, 2021

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
Copy link
Member

diosmosis commented May 11, 2021

@tsteur should we change the milestone?

@tsteur tsteur modified the milestones: 4.3.0, 4.4.0 May 11, 2021
@tsteur
Copy link
Member

tsteur commented May 11, 2021

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
Copy link
Contributor Author

flamisz commented May 11, 2021

build js

@mattab mattab removed this from the 4.4.0 milestone May 26, 2021
@mattab mattab modified the milestones: 4.3.0, 4.4.0 May 26, 2021
@diosmosis diosmosis merged commit 7f250ac into 4.x-dev May 27, 2021
@diosmosis diosmosis deleted the 15780-outlink-tracker-added-links branch May 27, 2021 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
4 participants