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

Fix ga.trackInterestingLink() on IE11 #2007

Merged
merged 2 commits into from Jun 14, 2018

Conversation

Projects
None yet
3 participants
@toolness
Contributor

toolness commented Jun 12, 2018

Fixes #2005, explanations are in the comments of the PR.

@toolness toolness added this to the Development Sprint 2 milestone Jun 12, 2018

@toolness toolness requested a review from hbillings Jun 12, 2018

const isExternal = el.host !== window.location.host;
// Anchors on most modern browsers actually have properties on
// them that contain parsed values of the URLs, but not IE11,
// so we're going to parse the URL manually.

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 12, 2018

Member

Are you saying window.location.hash doesn't work as expected in IE11?

This comment has been minimized.

@toolness

toolness Jun 12, 2018

Contributor

No, it works fine--it's the HTMLAnchorElement or whatever that is weird. Modern browsers have e.g. a host property that represents the host of the resolved href attribute but IE11 doesn't.

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 12, 2018

Member

so el.hash and/or el.host aren't working correctly? That's really weird. Links have been location objects since forever -- long before IE11. What about el.hostname?

This comment has been minimized.

@toolness

toolness Jun 12, 2018

Contributor

I'm not sure, I didn't dig into too many details but just made sure the tests passed on IE11...

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 12, 2018

Member

I guess if it's passing, it's fine, but my concern is two-fold: 1) I think we're importing a module we maybe don't need to be importing and 2) I'm wary of fixes that aren't fully explainable.

That said, clear out the codeclimate issues and I'd be OK with approving.

This comment has been minimized.

@toolness

toolness Jun 14, 2018

Contributor

That's fair, I can investigate it further... I had run into issues with the cross-browser interface for anchor links in the past and assumed it was due to that, but I could be wrong. Will poke at this when I get back home to my Win10 machine to verify.

This comment has been minimized.

@toolness

toolness Jun 14, 2018

Contributor

Ok, so I made this example page:

<!DOCTYPE html>
<meta charset="utf-8">
<title>Weird test page</title>
<p>Here is a link: <a id="link" href="./blah/a">boop</a></p>
<script>
onload = function() {
    var link = document.getElementById('link');
    console.log(link.href, link.host, link.hostname, link.pathname);

    // Now change the href, and we'll even give the browser extra time to get its
    // act together...
    link.setAttribute('href', './blah/b');
    setTimeout(function() {
        console.log(link.href, link.host, link.hostname, link.pathname);
    }, 1000);
};
</script>

I then started up a local server and visited the page.

On Chrome, Edge, and probably every other browser except IE, the following was logged to the console, as expected:

http://localhost:8080/blah/a localhost:8080 localhost /blah/a
http://localhost:8080/blah/b localhost:8080 localhost /blah/b

However, on IE11 the following was logged:

http://localhost:8080/blah/a localhost:8080 localhost /blah/a
http://localhost:8080/blah/b   ./blah/b

That is super weird; so IE11 does have host and hostname, but it mysteriously sets them to the empty string if href is changed to a relative URL, which I assume is a bug. But unless you can think of a better solution, I think that the only thing I can do is update the comment to be more accurate, but keep the solution of manually parsing the URL in-place...

const event = document.createEvent("MouseEvent");
event.initMouseEvent(
'click', true, true, window,
0, 0, 0, 0, 0, false, false, false, false, 1, null

This comment has been minimized.

@hbillings

hbillings Jun 12, 2018

Member

and they laughed the laugh of the damned

@tbaxter-18f

With code climate happy, I'm happy. Even though I still don't think we should need urlparse() :-)

@toolness toolness merged commit c837aaf into develop Jun 14, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate Approved by Atul Varma.
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@toolness toolness deleted the fix-ga-ie11 branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment