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

Honor target attribute for outbound link tracking #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Send event on ctrl/meta/shift and middle click
  • Loading branch information
Joelius300 committed Aug 10, 2022
commit f25543b27616a11ba9effdfbc643ce5bb062a2ce
3 changes: 2 additions & 1 deletion .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"popstate",
"hashchange",
"signup",
"automagically"
"automagically",
"auxclick"
],
"flagWords": [],
"ignorePaths": [
Expand Down
24 changes: 17 additions & 7 deletions src/lib/tracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,21 +277,29 @@ export default function Plausible(
}
) => {
function trackClick(this: HTMLAnchorElement, event: MouseEvent) {
if (event.ctrlKey) return; // Don't send event on Ctrl + Click, should behave like middle-mouse-click.
// auxclick is fired for any mouse-button except primary (left).
// The event should only be sent on aux (middle) click, return for others (right, etc).
const auxEvent = event.type === 'auxclick';
if (auxEvent && event.button !== 1) return;

trackEvent('Outbound Link: Click', { props: { url: this.href } });

// _self (same as falsy), _parent and _top need a delay for the request to go through.
// _blank opens a new tab -> no need for delay
// no special handling for custom targets -> use the default browser behavior
/* istanbul ignore next */
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
if (
(!this.target || this.target.match(/^_(self|parent|top)$/i)) &&
!auxEvent && // middle mouse click opens a new tab -> no need for delay
!event.ctrlKey && // ctrl + click opens a new tab -> no need for delay
!event.metaKey && // same as ctrl
!event.shiftKey && // shift + click opens a new window -> no need for delay
// don't try to access location object during testing
!(
typeof process !== 'undefined' &&
process &&
process.env.NODE_ENV === 'test'
Comment on lines 299 to 301
Copy link
Author

Choose a reason for hiding this comment

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

What are these for exactly? I assume it's to avoid errors during testing but there are already quite a bunch of errors when running yarn test regarding non-implemented navigation features and I can only assume it's because we're clicking links and that tries to run code to navigate to it's href but I've not found a way to confirm this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure since I didn't write it but I believe the idea is to not call setTimeout in tests. Maybe @Maronato has more information for this

) &&
// _parent, _top, _self (same as none) need a delay for the request to go through
(!this.target || this.target.match(/^_(self|parent|top)$/i))
)
) {
setTimeout(() => {
switch (this.target) {
Expand All @@ -310,7 +318,6 @@ export default function Plausible(
}
}, 150);

// _blank and custom targets should use the default browser behavior
event.preventDefault();
}
}
Expand All @@ -322,6 +329,7 @@ export default function Plausible(
if (node instanceof HTMLAnchorElement) {
if (node.host !== location.host) {
node.addEventListener('click', trackClick);
node.addEventListener('auxclick', trackClick);
tracked.add(node);
}
} /* istanbul ignore next */ else if ('querySelectorAll' in node) {
Expand All @@ -332,6 +340,7 @@ export default function Plausible(
function removeNode(node: Node | ParentNode) {
if (node instanceof HTMLAnchorElement) {
node.removeEventListener('click', trackClick);
node.removeEventListener('auxclick', trackClick);
tracked.delete(node);
} /* istanbul ignore next */ else if ('querySelectorAll' in node) {
node.querySelectorAll('a').forEach(removeNode);
Expand Down Expand Up @@ -362,6 +371,7 @@ export default function Plausible(
return function cleanup() {
tracked.forEach((a) => {
a.removeEventListener('click', trackClick);
a.removeEventListener('auxclick', trackClick);
});
tracked.clear();
observer.disconnect();
Expand Down