Honor target attribute for outbound link tracking

Joelius300 commented 2 years ago

Description

As discussed in #12, currently the outbound link tracking implementation doesn't honor the target attribute of links. This PR aims to fix this by differentiating between _self (default), _top, _parent and _blank inspired by this workaround by @abett.

I've successfully included this fix in my fork which I'm now using in my project (through a git submodule).

Unfortunately, there are no tests covering this change yet. I tried to get something working with the existing tests (I don't know jest) but failed. I think it's because navigation and the properties like location.href etc. aren't implemented/mocked in the testing environment and thus can't be used.
So if anyone could assist me in this or point me in the right direction I'd be happy to add these before merging.

Related Issue

Fixes #12 Repro etc. can be found there.

Implementing #16 would make this change obsolete but this should work as a solid workaround until the time for #16 comes.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to change)

While it's technically a breaking change in that links with target attribute now behave differently than before the fix, they are now behaving as one would expect. AFAIK the behavior now matches the default browser behavior without interception albeit with a 150ms delay (for everything except _blank). This delay was also present before the fix and could be removed by #16 IIRC.
In general no action should be necessary when this is implemented except maybe removing old workarounds. I'm assuming that nobody that knows of this bug is relying on it. That's why I checked both boxes.

Checklist:

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation. Update the disclaimer about this issue in the main docs here. API docs don't need to change I'd say.
[ ] I have updated the documentation accordingly. No, because it's not in this repo. I think this can be done once this PR is discussed and ready to merge.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes. See above.
[ ] All new and existing tests passed. See above.

metmarkosaric commented 2 years ago

However, I don't know for sure if the events are received properly on plausible.io because my account is disabled until I get my credit card to finally buy the subscription :)

thanks for the contribution @Joelius300! i've extended your trial for couple more weeks to give you more time to figure out your credit card plus allows you to test whether this works the way it should

Joelius300 commented 2 years ago

Thank you very much!

I'm now doing a bit of testing. I've setup a very simple application with webpack, html and typescript to test this feature. Repo and Plausible Dashboard. I didn't setup GH pages, I'm just using the domain because I have control over it (I'm testing with trackLocalhost: true). If you want to use it yourself, clone the repo, edit the domain in /src/index.ts, install the packages (this includes calling yarn && yarn build in /plausible-tracker) and run npx webpack to compile. Then you can serve the dist folder (I'm using vs code live server to do so).

I will post my findings below.

Joelius300 commented 2 years ago

Tested with Firefox 94 and Chromium 94 on Linux Manjaro (Arch) 64-bit:

I don't have a setup to verify that _top and _parent work as expected when iframes are involved. If anyone can help with that, I'll gladly include that in testing.

Good

All targets (none, _self, _top, _parent, _blank) now work and feel the same with and without outbound link tracking activated when using normal left click on the link.
All targets correctly send the outbound click event (+ correctly received on plausible.io) when using normal left click on the link.
All targets correctly get opened in a new tab when using middle-mouse-click or Ctrl + Click (since 4d289d2). This doesn't send the outbound click event which I'd consider correct.

Joelius300 commented 2 years ago

I think I'm done with this PR for now. I'm using the current state in Joelius300/werewolf-guide and it seems to work fine.

The fix is complete as far as I can tell:

Normal click works as expected (honors target attribute and sends event)
Ctrl + Click and middle-mouse-click work as expected (open a new tab without focus and don't send event)

However the PR is not complete yet:

Tests covering this bug need to be written
Tests regarding the behavior within iframes need to be done. I'm assuming it'll work because we're writing to window.top and window.parent when needed but I've not tested it.
Maybe more browser and os tests?
Maybe we could do tests with fully custom events because I think those would still be broken. Imagine a link with no target that has an event attached which opens a pop-up and then call preventDefault(). Plausible would still assign location.href. Since this only applies to links with a href attribute pointing outside the current host, I don't think we need to consider it in the tracker implementation, it's just something that came to mind.

When the PR is done, the disclaimer in the main plausible docs can be revisited (removed?).

Let me know if you'd like me to change anything, have questions or would like to help me with the missing pieces. I also enabled edits by maintainer so you should be able to directly commit to this PR if you want to. I'm happy to continue with this PR when you have time for it :)

Joelius300 commented 2 years ago

I updated the PR to send the event on ctrl/meta/shift and middle mouse click as well. I retested with Firefox on my Manjaro system and it seems to work fine. Tests are all green as well. Because of the istanbul ignore next above the if (not by me), it's not reflected in the coverage but basically all the issues this PR addresses are currently untested as far as automated tests are concerned.

As you can see in the latest commit, I also organized the if conditions and added some comments to explain why we're checking for them. On top of that I removed a @ts-ignore because it worked fine when I removed it on accident :) I don't know why it was there in the first place but I'm happy to add it again if it's supposed to stay. I also added a comment in the PR for the conditions regarding tests and non-implemented errors.

It would be nice to have more manual tests and some automated ones as well. What do you think feature wise?

ukutaht commented 2 years ago

@Joelius300 Looks good to me, I'd like to get it integrated. Tests are the only thing missing now I think

paulschreiber commented 2 years ago

Can this PR get approved/merged? It's been 9 months.

Joelius300 commented 2 years ago

Rebased. Again, I do not feel comfortable writing tests for this by myself and would love to get some guidance to finalize this fix.

ukutaht commented 2 years ago

@RobertJoonas Is working on outbound links in the main tracker. Could you also take a look at this one and make sure their behaviour is identical? No big rush but I thought it's quite relevant to what you're working on anyways.

RobertJoonas commented 2 years ago

@RobertJoonas Is working on outbound links in the main tracker. Could you also take a look at this one and make sure their behaviour is identical? No big rush but I thought it's quite relevant to what you're working on anyways.

Sure, will do.

Joelius300 commented 1 year ago

Since the last update on this PR, the outbound link handling of the original script in the main repo has changed (namely https://github.com/plausible/analytics/pull/2208). Would it be worth reworking the behavior in this script as well, to match the main script exactly?

On top of this, I'm still doubtful that maintaining two different implementations of the exact same script is a good idea. The handlebars script of the main repo is supposed to be minimal and your main focus as it's the one people use when they add Plausible to their site using a script tag, I understand that. Unlike that script however, the library here allows you to integrate the code directly into an applications code which, apart from other benefits, improves reach as it's not blocked as often by tracker blockers. From my limited experience with building scripts like this, I would assume that it's possible to have a "heavy-weight" library (with TypeScript, multiple files/modules, etc.), that can yield all the same minimal scripts that you have in the main repo thanks to tree-shaking and bundlers. With one initial bulk of effort, this would help your users and you wouldn't have to worry about two scripts maintaining parity when adding new features or improving one. \ Is this something you have looked at?

long2ice commented 1 year ago

So when this can be merged?

plausible / plausible-tracker