I found this one while trying to track down: http://drupal.org/node/213064

Everytime you call Drupal.attachBehaviors on a page with a table that tableheader.js acts on, the lines 

  $(window).scroll(scroll);
  $(document.documentElement).scroll(scroll);

attach another event to the page.
I am not entirely sure how this is related to my "parent has no properties" issue, but if I put in these two line in my code

    $(window).unbind('scroll');
    $(document.documentElement).unbind('scroll');

before I call Drupal.attachBehaviors, the error goes away.

I think this script need to be rethought, moving the any attaching of events to global objects out of the behavior.

CommentFileSizeAuthor
#7 tableheader-lint.patch2.29 KBstarbow
#1 tableheader.patch1.48 KBstarbow

Comments

starbow’s picture

starbow commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new tableheader.patch1.48 KB

I am bumping this up to critical, not because it's effects are so bad, but because having it in core could lead other people into making the same mistake.

This patch fixes the problem by only attaching the event to the scrollbar once, and then updating the function that the event calls whenever Drupal.attachBehaviors is called (instead of attaching a new event each time).

This patch also fixes http://drupal.org/node/213064, without forcing me to unbind all the events attached to the scrollbars (which could have side effects).

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

I see you are adding a proxy function basically which would call the right event handler when required. Is there a precedent in Drupal for this kind of solution already? How is the same problem solved elsewhere in Drupal?

starbow’s picture

starbow commented

I don't know of a precedent. I could be wrong, but I think this is the only place in Drupal where this particular problem has come up.

The whole idea of the Drupal.attachBehavior($context) call, is that the Drupal.behaviors.whatever functions are only supposed to act on the $context (ei: the part of the page that has just gotten replace by an AHAH call). However, tableheader.js needs to attach an event listener to the scrollbar, which is outside of the $context. And that event listener needs to somehow effect the table headers that is (potentially) inside the $context that is being replaced.

So, yeah, my proposal is to attach the proxy function just once, and then just inform that proxy each time the table headers get replaced.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

All right, understood. Then let's get this tested in a few browsers and get fixed.

catch’s picture

catch
he/him
Primary language English
commented

starbow: what exactly needs to be tested? Just tableheader.js or tableheader.js in conjunction with something else?

starbow’s picture

starbow commented

The most important thing to confirm this that this patch doesn't break any of the tableheader's functionality in any of our supported browsers. I have tested on FF 2, IE 7, Safari 3beta and Opera 9.25 (all on Windows XP). I have mostly tested on the Block's admin page. I tested scrolling, resizing the browser, and scrolling after resizing. I am not sure if there are any other behaviors to test.

Secondly, there is testing that the patch does what it says it does. I tested with this bit of code added add the bottom of tableheader.js:

// Global Killswitch on the <html> element
if (Drupal.jsEnabled) {
  $(document).ready( function() {
    document.title = '0';
    $('#edit-submit').click( function() { 
      $.get(Drupal.settings.basePath + "admin/build/block", function(data) {
        var $response = $('<div></div>').html(data);
        var content = $response.find('#block-admin-display-form').html();
        var $c = $('#block-admin-display-form').html(content); // Inject the new content into the page.
        Drupal.attachBehaviors($c);
      });              
      return false;

    });
  });
}

And then inserting this at line 63 (first line of the scroll function): 

    document.title = parseInt(document.title) + 1; // show on double call.

If you add this to the original file, then you will see that each time you click the up or down scrollbar arrows, the page title increments by one. Then click the "Save block" button, which will replace the block table with an identical version of itself and reapply the behaviors. Now, when you click the scrollbar the counter increments by two. To test the patched version, put the first block of code at the bottom, and insert the second after the line

  Drupal.tableHeaderOnScroll = function() {

Now the counter in the title of the page should only increment by one, both before and after you click the "Save block" button. I found this test works as expected on Firefox and Safari, in Opera the counter increments by 3 instead of by 1 (both before and after click, so good enough), and in IE7 I the screen goes blank on the line var $response = $('

').html(data); (which is annoying but not a show stopper).

starbow’s picture

starbow commented
StatusFileSize
new tableheader-lint.patch2.29 KB

I am not sure if this is appropriate, but jslint.com threw up a couple of red flags on this file (a couple places where var is used on the same variable twice, unneeded semicolon, no radius on parseInt). I find that keeping jslint happy really helps crossbrowser compatibility, so here is a patch that rolls those in with my other fix.

catch’s picture

catch
he/him
Primary language English
commented

OK tested #7 in firefox, IE7 and Opera 9.25 and 9.5, all seems fine.

Didn't implement the counter since I'm in a hurry, but I did do some cursory tests alongside the latest patch here: http://drupal.org/node/210131#comment-709763 and confirmed it doesn't break anything associated with the update either.

I leave it as needs review for someone a bit more js savvy.

David_Rothstein’s picture

David_Rothstein commented
Status: Needs review » Reviewed & tested by the community

I tested #7 in Firefox (1.5 and 2), Internet Explorer 7, Opera 9.25, and Safari 3 (Windows). I did my tests on the Permissions admin page, and everything seemed to work fine (scrolling, resizing, etc).

(I did a sanity check of Internet Explorer 6 too, which probably wasn't necessary since the tableheader scrolling isn't used there, but just to make sure that the extra javascript code didn't introduce any other problems, and it didn't.)

I also tried the counter test from #6 on the blocks admin page and got the same results: always increments by 1 in Firefox and Safari, always increments by 3 in Opera, and the test doesn't work correctly on IE7 (blank page after hitting the submit button).

Hm, I hardly qualify as "JS savvy" (not by a long shot), so I can't do much to review the code itself, but it looks like this has been tested by enough people to set it to RTBC...

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Great, committed, thanks.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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