Comments

hass’s picture

Status:Active» Postponed (maintainer needs more info)

But we need to bind the click event to all links.. if we don't do we cannot track clicks to them!?

sun’s picture

Status:Postponed (maintainer needs more info)» Active

No, not all links. What you really want is to attach a click event handler to certain links.

The lag time I stated was wrong. Actually, users with super admin-alike permissions (and hence, a full-blown admin_menu containing hundreds of links) experience a lag of approx. 15-30 seconds on each page. With "lag" I am referring to a complete browser freeze/lock-down, as it seems the JavaScript is blocking the system process completely. In Firefox, a message appears after 30 seconds that warns the user about a JavaScript that might be in an infinite loop (with the option to break all scripts).

Regarding admin_menu, the jQuery selector could simply be prefixed with a :not(#admin-menu) (note the trailing space). However, handling regular, expanded menu trees, commonly used for nice_menu and similar dropdown menus is not that simple. Without having tested this idea - maybe using jQuery's attribute selector(s) to match only links that actually need to be processed would be faster?

Something along the lines of

// mailto link tracking
$('a[href^=mailto]')...

// download link tracking
$('a[href$=pdf|zip]')...

// external link tracking
$('a[href^=http]')...

Note that I'm not sure whether *= would allow for a regular expression though - if it does, then a regex like ^mailto|^http|pdf$|zip$ would match all links of interest in one selector.

hass’s picture

I'm also a friend of a positive list... it's typically easier to maintain and saver to use. But we need to keep all current settings intact and we have this file extension regex. Maybe something like $("a[href^=mailto],a[href$=\\.(' + ga.trackDownloadExtensions + ')],a[href^=https?|ftps?]", context).click( function() { might be easier to build based on settings. Not sure and also not tested. Otherwise there is a .filter()... see http://docs.jquery.com/Traversing/filter

hass’s picture

I'm not sure if the .filter() would really help. It would also take time to evaluate... and I'm not sure if the searching for a is the most time consuming or the event attaching...

Additional it seems not possible to add a regex like a[href^=https?|ftps?]. From documentation side and testing it seems only a string compare.

hass’s picture

Try this with Firebug and you will see in the console that every link is evaluated. Therefore I expect no real improvement with filtering.

  jQuery.each($('a').filter(function() {
        console.log('EvaluateFilterOn: ' + this);
        return isDownload.test(this.href) || isInternalSpecial.test(this.href) || $(this).is("a[href^=mailto:]");
      }), function() {
    console.log('BindEventTo: ' + this);
  });
edward.peters’s picture

Thank you very much, guys, for working on this. It could bring huge page load savings for us - and we non-developers are much in debt to those of you who understand how to make things function most efficiently.

hass’s picture

If you'd like to get this issue fix, please provide a patch very soon.

hass’s picture

It'd like to share this link I was pointed to in jQuery IRC http://www.danwebb.net/2008/2/8/event-delegation-made-easy-in-jquery. It might be a solution as it wouldn't bind a listener to all links on page load.

hass’s picture

Component:User Interface» Code
Status:Active» Needs review
StatusFileSize
new3.54 KB

This changes GA JS to bind the onclick event to the "document" and every click on a page checks if this is a link and if we need to track the click or not. This does no longer bind n-events to all links of a page - it simply binds one event only - what should speed up the process n-times-1. I hope this do the trick as all .filter() and :not() stuff seems to complicate things heavily and do not work reliable.

Please test and provide feedback.

hass’s picture

There is one bug... a right click on an element will also be tracked.

hass’s picture

Binding the event to document.body seems to solve the right click issue. New patches attached.

hass’s picture

There seems to be a bug. see http://drupal.org/node/330616#comment-1124073

For example this markup

<a class="document-link" href="/download/import/27699f921a4be03fb03e5e60d77de3d6a0c66de6">
  <img src="/sites/all/modules/custom/document_ext/images/ppt.png" title="Agenda.ppt" alt="xls"/>
  <div class="link-label">Agenda.ppt</div><div class="clear-block empty"></div>
</a>

The var element is reported as

[object HTMLDivElement]

hass’s picture

Changing var element = event.target; to var element = $(event.target).parent('a').get(0) || event.target; seems to solve the issue in #12, but I'm not sure if this is the right way.

Maybe this should be something like var element = $(event.target).filter(function () { return $(this).is("a"); }).get(0) || $(event.target).parent('a').get(0); but this looks to complex.

Any idea how to make this easier?

hass’s picture

var element = $(event.target).parent('a').andSelf().get(0);

hass’s picture

StatusFileSize
new2.15 KB

Here is a reworked version with console debugging for D6.

dldege’s picture

Subscribing

hass’s picture

As nobody gave feedback to the new JS file I've tried to install admin_menu and verify myself the behaviour and the speed differences you are complaining about. It's very annoying that I haven't done this earlier as I'm not able to repro this 20-30 seconds event binding delay today. I see totally *no* delay if events are bound to all links. I've tested with the latest release and the new body event bind file above.

But I have had l10n_client enabled and this slowed down the page loading by 20 seconds! After I've disabled this module everything was very quick and I'm not able to repro the above issues.

I tend to give this a WON'T FIX in 2 weeks if there is no feedback.

sun’s picture

Sorry for coming back to you so late. And thanks for your investigation. Unfortunately, l10n_client is not installed, so this cannot be the cause.

I will test your latest patch now.

hass’s picture

Status:Needs review» Closed (won't fix)

Closing this time wasting and useless effort as there was no feedback at all and the issue does not exists.

quicksketch’s picture

StatusFileSize
new1.64 KB

After reading through this issue, it's unfortunate this never made it through last time around. I can assure you it's a very real problem. Here's an example page in question that's suffering from the super-slowdown: http://www.tribalfootball.com/newswire (1700 links).

Though my original patch has been applied to the site as a stop-gap, you can still emulate the problem by attaching a simple click handler to all links through Firebug:

$('a').click(function() {
  console.log('click');
});

Here's a reroll that takes into account link parents, finds the first one, then executes the link handler on that value.

quicksketch’s picture

Status:Closed (won't fix)» Needs review
quicksketch’s picture

StatusFileSize
new1.3 KB

Hm, seems I inadvertently removed some unrelated code. Here's a cleaner version.

Note that I've switched from using Drupal.behaviors to just using a $(document).ready(), since this even should occur only once, there's no need to use a behavior.

quicksketch’s picture

StatusFileSize
new1.3 KB

Holy moly, I'm having patching troubles. I needed to switch the .andSelf() method around in the previous patch.

cwgordon7’s picture

Status:Needs review» Reviewed & tested by the community

The patch in #23 works properly for me, and although I cannot notice any changes in speed, I can see how there would be performance improvements on pages with many links. Overall I think this is a worthwhile and useful patch.

cwgordon7’s picture

Also, just to confirm, running the snippet in #20 on the given page was very slow, demonstrating the potential for attaching a function to the event handlers of a lot of different elements to be a performance issue.

hass’s picture

Status:Reviewed & tested by the community» Needs work

Drupal.behaviors should always be used for attaching something in D6. Aside why are you not trying my variant?

<?php
 
$(document.body).click(function(event) {
   
// Catch only links and parent links of clicked child elements.
   
$(event.target).parent('a').andSelf().filter('a').each(function() {
     
console.log("Clicked link: " + this.href);
?>
hass’s picture

StatusFileSize
new1.99 KB

Here is a re-roled developer version with debugging of the #15.

hass’s picture

StatusFileSize
new1.98 KB

Ah, context is useless

hass’s picture

Patch attached

hass’s picture

Status:Needs work» Needs review
hass’s picture

Title:Massive page rendering slowdown with admin_menu and/or nice_menu» Massive page rendering slowdown with man links
quicksketch’s picture

Status:Needs review» Needs work

Context and behaviors are not needed in this implementation since there is only one body tag on a page. With the latest patch, behaviors will be unnecessarily fired every time an AJAX call is made. Try applying your patch from #29, visiting add/node/story, then uploading 5 files (or just clicking the Attach button 5 times will work). Because you've used Drupal.behaviors, every time an upload is made, the same behavior is attached again to the body. Now when you click on a link, the handler will be fired 6 times (one for each AJAX call plus the original one loaded on the page).

That's what I meant by not using the Drupal.behaviors because they're not necessary. Drupal.behaviors exist so that JavaScript can reattach itself to a new part of the page. But since the BODY tag isn't likely to be replaced and there is only one of them, there's no point in using behaviors. If you go with behaviors anyway, then context should be used to prevent attaching the behavior multiple times.

Another small problem, this selector will fail if there are multiple tags inside of the link:

$(event.target).parent('a:first').andSelf().filter('a')

For example this will not work on a situation like this:

<a href="http://example.com"><span><strong>Link title</strong></span></a>

.parent('a') will only go up one level, which is why I used .parents('a:first'), which will find the first parent that is a link.

hass’s picture

About the behaviours attaching. In D7 there is a function to detach all behaviours. If someone would detach event listeners - the GA function it's no longer attached, isn't it? I wasn't aware that it's attached multiple times... if this is the case what will happen today? every link of your 1700 get's 6 events attached? I wonder why the debug line haven't shown this with 6 console out prints...!?

parent <> parents: I wasn't aware of this... thank you. I will try it out myself, too.

Nevertheless I like the one liner a bit more than your variant with sub function.

sun’s picture

You've misunderstood the concept of "detaching behaviors". Only behaviors that need to detach have to detach (logically).

quicksketch’s picture

Fortunately we shouldn't have the "multiple attach" problem on the current code because the current version uses context as such: $('a', context). It's a tad bit risky because there's nothing stopping someone from doing something like: Drupal.attachBehaviors(document). In which case we'd end up with the multiple attach problem. But since nearly all implementations only use Drupal.attachBehaviors on the new content on the page, this isn't terribly likely. However, this obscure problem should be fixed by whatever solution we decide on here.

I'm not at all opposed to nixing my sub function, I just used that because it made the least amount of code change and could optionally be attached on a link if desired, even though we don't need that functionality for any reason as far as I can think.

hass’s picture

Found this in D7... maybe we can implement the same logic today to make the backporting easy. Not sure if this makes sense. If any change goes in we need to have a D7, D62 and D5 (working with and without jquery_update) version ready.

* Behaviors should use a class in the form behaviorName-processed to ensure
* the behavior is attached only once to a given element. (Doing so enables
* the reprocessing of given elements, which may be needed on occasion despite
* the ability to limit behavior attachment to a particular element.)

hass’s picture

Latest patch. We should also comment on why we are not using behaviour anymore if we do so...

hass’s picture

Err, wrong file uploaded...

smk-ka’s picture

I like that quicksketch's patch factored out the click-tracking into its own function, I can imagine this could be useful for developers in certain situations.

hass’s picture

@sun: Why haven't you used $(document).ready(function() { in admin_menu? I saw you have used the css class admin-menu-processed logic... is this more core friendly?

quicksketch’s picture

To use a website that's a little closer to home, this problem is clearly evident on http://drupal.org/project/usage when trying to click on any link on the page.

sun’s picture

That explains why my FF add-ons always crash on that page. I didn't even know that d.o has GA installed...

@hass: Why do you care for core? This module won't ever go into core. JS for D7 uses a completely different structure, so it's not very comparable to D6 anyway.

I'd agree that this should use a behavior, since other AJAX/AHAH-driven scripts can add new content with new links to a page and we also want to track those. So the question seems to be whether we can - reliably - find a DOM element in context to add a class to? If not, building a array stack of already processed DOM context references that's compared with any new context passed via Drupal.attachBehaviors() might work out (wild assumption though).

hass’s picture

Only as a note - I try to develop every module in a core worthy way... doesn't matter to me if it goes in core or not. Code quality and readability is most important for me and I'd like to keep all versions in sync for maintenance reasons. Something that's proven to be the future way cannot be wrong if backported... that's why I think it wouldn't hurt if we use D7 logics in D6 and D5 if possible in an easy way. The [module]-processed class seems to be one good way.

I currently have no idea why we need to have a context if we always bind the event to the body.

quicksketch’s picture

Priority:Normal» Critical
Status:Needs work» Needs review

I currently have no idea why we need to have a context if we always bind the event to the body.

We don't need context at all if we go with $(document).ready() rather than Drupal.behaivors. But if we go back to behaviors, we'll need to respect the context to prevent the double binding problem.

#38 looks like it'll do the job to me. This problem is becoming increasingly frustrating on Drupal.org as major issues (200+ replies) are now all exhibiting this behavior. The approach we use to attach to the page is extremely minor in comparison to the problem that Google Analytics completely cripples pages with a lot of links. Can we get the main problem fixed and worry about the virtues of $(document).ready() vs. behaviors later?

quicksketch’s picture

Title:Massive page rendering slowdown with man links» Massive page rendering slowdown with many links
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new4 KB

The patch in #38 works for me. Applied and re-tested again and I can't find any problems with the implementation. Here's a reroll identical to #38 except it removes the debugging console.log() call.

quicksketch’s picture

Heh, okay that was the EXACT same patch. This one removes the console.log().

hass’s picture

How about a D5 patch? It was hard to get this working in D5 and the old patch needs a re-roll.

Bojhan’s picture

I don't see how a critical bug, can be open for a year. I am sorry but this is also affecting d.o, and people are seriously considering to just disable it. I am someone who is using the GA of Drupal.org intensively for the work on redesigning the IA of our Docs.

There is a lot of work done on the d.o redesign, when we step to the new design I wish to be able to compare metrics from before and after so that we can measure the successfulness of the change as a whole - and improve where it doesn't.

I am not a coder, but merely a power user of GA - I would be really sad to see to not see this fixed shortly.

@hass Could you please pick up this issue? It's fairly critical and the code should be almost identical to D5.

Damien Tournoud’s picture

We need a fix for Drupal.org, and we need it as soon as possible. @hass, please do what it takes to fix this issue.

Damien Tournoud’s picture

Better tagging.

hass’s picture

I'm not sure if you understand the issue for sure, but critical is a bit extreme to me. I have also no idea what this have to do with UX or redesign. You may complain here about MySQL Server performance issues of d.o.

If you are not a coder and you do not have Firebug installed and no HTML validator and/or other developer tools that parse and monitor the site code I do not trust that you are able to see the slowdowns as I wasn't able to repro them in past, see #17. Nevertheless I wasn't able to repro this issue I worked days on it only for the reason that I've got the idea that this could become an issue and a global handler is better.

The case was open for only 6 months and the last time someone (second developer of 40.000+ installations) considered it's a problem is only ~1 month ago. I wasted days of my spare time and waited than for about 6 months for sun's review, but there was never any feedback and nobody else jumped in. This change could ***POTENTIALLY*** break the GA module tracking at all and if it's broken *I'm* the guy you need to address a fix and receive 40.000 complains. I'm sure I wouldn't receive any help very soon if it's broken... we can see how many people shown interested in the review of this change in past.

I have no plans to commit this code without *more* reviews and a well working D5 version as this is also a problem in D5 and it's much more difficult to fix in D5 as jQuery do not have andSelf().

I'm also still considering to commit this to a new 3.x branch as experimental patch and keep the stable 2.x branch untouched. Not yet sure where to go...

Bojhan’s picture

I was merely addressing the need for this, if people chose disabling over fixing it - there is usually something seriously wrong. As said I cannot speak on the code related issues, but to me the criticality seems clear if it will interfere with ongoing tracking on a large site.

hass’s picture

I have no idea why someone likes to disable a module without understanding the issue. Who chose to disable it?

8 months ago - this issue has been debugged with 20.000 links on a single page and the slowdown was nearly unreproducible. Before committing code I also need to verify myself that the issue really exists. In past I wasn't able to repro this with admin_menu at all.

JohnAlbin’s picture

The infrastructure team does understand the issue: #512892: Patch or Disable Google Analytics Module The effect of the bug is that d.o. is almost unbearable to use for some people.

I have no plans to commit this code without *more* reviews and a well working D5 version as this is also a problem in D5 and it's much more difficult to fix in D5 as jQuery do not have andSelf().

Geez, an RTBC about a jquery patch from quicksketch is like the gold standard of RTBCs for me. :-)

hass’s picture

I wasn't aware that quicksketch is a jQuery guru... sorry. But nevertheless - couldn't one of you please explain me, why this stats show me more or less that the d.o server is slow? See the attached screenshots, please. Maybe I misinterpret something, but 28 seconds on GET /project/usage is self speaking for me (-> Apache/PHP server is very slow). Safari also show the same 5,12s of 6,39s for the page. I tried to analyse this myself in past, but as said I wasn't able to identify the long running JS. Need to try with Jiffy...

WGET also shows the same:

>wget "http://drupal.org/project/usage"
SYSTEM_WGETRC = c:/progra~1/wget/etc/wgetrc
syswgetrc = C:\Programme\GnuWin32/etc/wgetrc
--2009-07-08 15:20:12--  http://drupal.org/project/usage
Auflösen des Hostnamen "drupal.org".... 140.211.166.6
Verbindungsaufbau zu drupal.org|140.211.166.6|:80... verbunden.
HTTP Anforderung gesendet, warte auf Antwort... 200 OK
Länge: nicht spezifiziert [text/html]
In "usage" speichern.

    [                               <=>                                             ] 1.721.756    316K/s   in 6,4s

2009-07-08 15:20:23 (261 KB/s) - "usage" gespeichert [1721756]

EDIT: This is nearly 2 MB HTML code... this "download" takes time to transfer... how fast are your lines? :-)

Please shed some light to me how we are able to measure the event binding slowdown.

smk-ka’s picture

@hass
This issue is not about page loading time, but about JavaScript stalling the browser UI. I think this hasn't been clearly said before, so please do the following steps to reproduce:
1. Visit http://drupal.org/project/usage
2. Move away from that page by clicking a link.
3. Your browser UI stalls because it has to unregister thousands of event handlers, before finally displaying the "unresponsive script" popup. *This* is what the patch is about to fix.

hass’s picture

But we discussed about event binding times first - not unbinding. Binding the events to every single link should take some measurable time. I've clicked on the CCK link and the CCK usage stats are quickly shown. Clicking on a browser tab while the CCK usage stats are loading is not blocked (if this should be the stale browser). There is no "unresponsive script" popup shown. I cannot repro...

hass’s picture

Ah, now I've got the popup... ok, but why happens this only if you click away to a new page and not on page load if all events are bound. The main source of this issue seem to be #390494: Slow unbinding in jQuery 1.2.6 and the fix may make the unbind working as fast as the bind.

hass’s picture

Status:Reviewed & tested by the community» Fixed
hass’s picture

Version:6.x-2.x-dev» 5.x-1.x-dev
Status:Fixed» Patch (to be ported)
hass’s picture

Version:5.x-1.x-dev» 6.x-2.x-dev
Status:Patch (to be ported)» Reviewed & tested by the community

@quicksketch: How do we need to change the D7 version?

hass’s picture

Version:6.x-2.x-dev» 5.x-1.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
quicksketch’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new4.34 KB

Thanks for committing hass. Here's a D5 reroll, it's not elegant but at least it matches the code in the D6 version so they should be easy to maintain in sync.

quicksketch’s picture

Drupal 7 can work with exactly the same JS as the D6 version without any changes since we're not using behaviors.

quicksketch’s picture

StatusFileSize
new5.48 KB

In my last patch I had blown away changes that were unique to the 1.x version (such as urchin.js support). This patch starts with the existing code from D5 and just makes the same body-binding change.

hass’s picture

THX, but how would the D7 version looks like with behaviors? As discussed earlier we may need to provide the ability to bind again. I'm not sure why someone should unbind the events from body, but if it happens a re-bind seems to be required.

hass’s picture

Any chance to see the latest D6 DEV deployed on d.o for a final production test? Rolling new releases may take one or two weekends...

hass’s picture

StatusFileSize
new5.52 KB

Added the area changes to the D5 patch, CNR. Why has if (Drupal.jsEnabled) been removed? What does this break?

hass’s picture

Status:Needs review» Needs work

$(link).filter("a").each(function() { (patch in #65) and $(link).filter("a,area").each(function() { is not working at all under jquery_update 1.2.6. Not tested with D5.x jquery 1.x

hass’s picture

http://drupal.org/update/modules/6/7#local_settings_behaviors may requires some changes for D7 version as Drupal.settings is not available!?

Tony Sharpe’s picture

subscribe

hass’s picture

Status:Needs work» Closed (won't fix)

As nobody is interested in backporting I'm closing for now.

hass’s picture

We may need to undo this change to slowdown the browser moving to the next page.

We have several issues where the high speed hurts back now. Would be happy to get some feedback on this from some JS/jquery gurus in here. Please join #1836370: Link clicks (example downloads) not beïng tracked and #1580144: Links not tracked in some browsers as jquery click event exits before tracking code finishes. Thanks a lot.