Problem/Motivation

When using multiple top-level domains, the Javascript for google_analytics attempts to match the hostname of a given href of a link against the defined set of top-level domains to determine whether a link off-domain should be registered as a link or as an outbound link event. However, the JS does not correctly check the href, and so a link will never be registered as a cross-domain link appropriately.

The relevant piece of code is:

      // where $(this) is an <a> element
      // http://drupalcode.org/project/google_analytics.git/blob/refs/heads/7.x-1.x:/googleanalytics.js#l42 for code in context

      if (ga.trackDomainMode == 2 && isCrossDomain($(this).attr('hostname'), ga.trackCrossDomains)) {
        // run _link()
      }

The problem with this is that 'hostname' is not a valid or standard attribute of an <a> element, and there is no apparent code within google_analytics that would place this non-standard attribute on all links. Thus, the first argument of isCrossDomain() is always null, and so the check for cross-domain links always fails.

Additionally, isCrossDomain() checks against domain literals; it should probably be checking the top-most domains given, to allow for subdomain links being possible cross-domain link candidates. This may or may not be the right strategy, but minimally it seems it should check for presence or absence of 'www.' and not treat those as separate top level domains.

Proposed resolution

The logic of the cross-domain link check should be changed to pass href to isCrossDomain(); and, isCrossDomain() should be modified to match top-level domain fragments against the test hostname, rather than doing a literal string match.

Remaining tasks

Patch submitted for review.

CommentFileSizeAuthor
#11 google_analytics-crossdomain-link-bug-1995922-11.patch729 bytestimcosgrove
#1 google_analytics-cross_domain_link_failure-1995922-1.patch2.01 KBtimcosgrove

Comments

timcosgrove’s picture

timcosgrove commented
Issue summary: View changes

code snippet edit

timcosgrove’s picture

timcosgrove commented
Issue summary: View changes

code snippet edit

timcosgrove’s picture

timcosgrove commented
StatusFileSize
new google_analytics-cross_domain_link_failure-1995922-1.patch2.01 KB

Patch attached. I patched this against 7.x-1.x; the same fix probably applies to 7.x-2.x etc, but I am working with 7.x-1.3 and patched that.

hass’s picture

hass commented

Have you really tested it? It works here. It only does not match wildcards or substrings. This is a known limitation, not a bug.

timcosgrove’s picture

timcosgrove commented

How would it work on an attribute 'hostname'? This doesn't exist.

hass’s picture

hass commented

Have you tried it? This is jquery... It works.

timcosgrove’s picture

timcosgrove commented

Yes of course I tested it. I worked with this bug for several hours, thinking that there was a problem with my own Google Analytics setup, before I traced the bug to that line.

http://api.jquery.com/attr/

This function grabs attribute values off elements. In your case, if you had dynamically added a 'hostname' attribute to link or area elements that you were applying the callback to, this would work fine. However, nothing in your module does this, and nothing in Drupal core does this, to my knowledge.

I attempted to grab .attr('hostname') off an arbitrary link, for your benefit:
http://jsfiddle.net/VWTsv/1/

If you have test domains (two top-level domains) that are linked by cross-domain functionality that I can poke at, I would love to see them. Really, just here to help!

The other alternative is for me to get 2 spare domains, exposed to the internet and with nothing on them, to put clean Drupal installs + Google Analytics module on them to test this. But, since you claim it works, you no doubt have test domains that I can look at to see it working. It is very easy to confirm that cross-domain tracking is working; confirmation can be done client side.

Any help appreciated! Seriously, I would like to be wrong about the way you're using attr(), because if you are right I will have learned something completely new about jQuery, which I always welcome.

hass’s picture

hass commented

http://stackoverflow.com/a/14613849

Place this code after $(event.target).closest("a,area").each(function() { and you will see the hostname of the clicked link:

alert('Hostname: "' + $(this).attr('hostname') + '"');
event.preventDefault();

Can you share a screenshot of List of top-level domains setting, please?

jec006’s picture

jec006 commented
Status: Active » Needs review

Hi,

This doesn't work - it is jquery and it is valid, however, hostname is not an attribute so jquery just returns empty string / undefined.

See http://jsfiddle.net/jec006/AR4cw/ for confirmation.

timcosgrove’s picture

timcosgrove commented

As requested: https://dl.dropboxusercontent.com/u/379975/ga_crossdomain_js_bug.mov

Really, just trying to help!

echeese’s picture

echeese commented

It is a property of <a> so if you change .attr('hostname') to .prop('hostname') it works for me

timcosgrove’s picture

timcosgrove commented
Title: Cross-domain link check employs deprecated use of $.attr() » Module JS does not handle links cross-domain links correctly

Hey! Apologies for the back and forth on this. I think I've found the problem, which is due to differences in jQuery.

http://api.jquery.com/attr/

Specifically:

As of jQuery 1.6, the .attr() method returns undefined for attributes that have not been set. To retrieve and change DOM properties such as the checked, selected, or disabled state of form elements, use the .prop() method.

$(this).attr('hostname') works in some cases because prior to jQuery 1.6, $.attr() would retrieve properties as well as actual attributes. Once you move to 1.6 or up, that is no longer the case.

If I run clean core Drupal, with your code, it works. If I install jQuery Update and run 1.5, it works. If I run 1.7, it fails.

You could argue that that is not your problem, and that's your prerogative; however, it'll be possible to keep the code compatible with old and new jQuery versions with a very small change.

I'll be back in a bit with a revised patch that only addresses this one issue.

timcosgrove’s picture

timcosgrove commented
Title: Module JS does not handle links cross-domain links correctly » Cross-domain link check employs deprecated use of $.attr()
StatusFileSize
new google_analytics-crossdomain-link-bug-1995922-11.patch729 bytes

Hi there-

As I said, I had hoped to learn something new about jQuery today, and I did: $.attr() would read non-attribute properties off elements prior to jQuery 1.6, which is unexpected and interesting.

I've changed the title of the issue to reflect the diagnosis, and attached a much less drastic patch which ensures that the cross-domain check will work for jQuery version both prior to 1.6 and from 1.6 on.

I do feel like the cross-domain handling could be done differently, but I'll submit that separately as a feature request.

Thanks!

timcosgrove’s picture

timcosgrove commented
Title: Module JS does not handle links cross-domain links correctly » Cross-domain link check employs deprecated use of $.attr()

Note that this is because of the jQuery version. JSFiddle for better or for worse doesn't allow jQuery versions prior to 1.6.4, so it's difficult to confirm this there, but I was able to confirm that this sort of thing *would* work 1.5 or prior.

hass’s picture

hass commented
Title: Cross-domain link check employs deprecated use of $.attr() » Module JS does not handle links cross-domain links correctly
Status: Needs review » Postponed (maintainer needs more info)

I have tested it with chrome 4h ago.

hass’s picture

hass commented

Oh, what about prop() as attr() replacement?

timcosgrove’s picture

timcosgrove commented

$.prop() was added in 1.6. You would need to sniff for its existence or branch off jQuery version somehow, so that $.attr() was used when it was available and working for this particular case, and then replacing that somehow with $.prop() for versions when it becomes available.

Replacing `$(this).attr('hostname')` with `this.hostname` is both slightly more accurate and works both before and after the change. There's nothing to be gained by converting your element to a jQuery object just to get a property off it; standard Javascript does this just fine.

hass’s picture

hass commented
Version: 7.x-1.3 » 7.x-1.x-dev
Status: Postponed (maintainer needs more info) » Needs review

So we need a review if this safer than the current in all browsers down to IE6, FF3.5

I'm open to change the isCrossDomain function to substring match like google does, too. I just found it a bit difficult to implement.

hass’s picture

hass commented

#11: google_analytics-crossdomain-link-bug-1995922-11.patch queued for re-testing.

hass’s picture

hass commented
Title: Module JS does not handle links cross-domain links correctly » Cross-domain link check employs deprecated use of $.attr()

Cross post

hass’s picture

hass commented
Status: Needs review » Fixed

D7.1: http://drupalcode.org/project/google_analytics.git/commit/048072d
D7.2: http://drupalcode.org/project/google_analytics.git/commit/2146414
D8.2: http://drupalcode.org/project/google_analytics.git/commit/a60a5a6

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

tag abuse