Drupal.behaviors.tableHeader doesn't respect the context.
To reproduce the bug:
Do an ahah/ajax update on the page, which does NOT replace the table that tableHeaders is attached to.
Call Drupal.attachBehaviors(content) on the new part of the page.
Floating table headers no longer work. If the headers were already floating, they stay floating when they should go away. If they are not floating, they won't start.

The problem is so much code gets run outside of the area protected by

$('table.sticky-enabled thead:not(.tableHeader-processed)', context)

Drupal.tableHeaderOnScroll gets redeclared, with blank headers in the closure. The connection to the old headers is lost.

It looks to me like tableheaders.js was written before Drupal.attachBehaviors was added to Drupal 6, and then tweaked just enough to make it work when the entire table is replaced. I suspect it needs to be redesigned to more fully work with the Drupal.attachBehaviors concept.

CommentFileSizeAuthor
#22 tableheader-234377-22.patch436 bytesleksat
#15 tableheader.patch508 bytesanrkaid
#14 tableheader.js_.patch508 bytesanrkaid
#10 234377_tableHeader_multiple_attachments.patch1.59 KBhanoii
#7 234377_tableHeader_multiple_attachments.patch1.65 KBhanoii
#3 234377_tableHeader_multiple_attachments.patch1.39 KBhanoii

Comments

starbow’s picture

starbow commented

A workaround is to add this at the top of Drupal.behaviors.tableHeader

  // If there are no sticky tables in this context, just return.
  if (!$('table.sticky-enabled', context).size()) {
    return;
  }
hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Version: 6.1 » 6.16

I just found out this same issue and still happening on 6.16

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Status: Active » Needs review
StatusFileSize
new 234377_tableHeader_multiple_attachments.patch1.39 KB

Well, for me it was always a bit ambitious to patch drupal core, or even have a production site with a core modified, but here is a humble patch for tableheader.js trying to sort this problem. Although the workaround above does seem to help, I rather tried to do something more general. I believe the problem was that the headers = [] was initialized on each drupal.attachBehvaiors(), thus, loosing the the tables to show on scrolling. I have moved that array to a global one inside the drupal object. Not sure if this has any other implication, but this all seem to on my tests.

One thing that I am concern is what should happen if a table is removed by an AHAH/AJAX call and then Drupal.attachBehaviors(). I think that particular use case is not handled at all by the current tableHeader.js. In anyway, I think my patch does fix the issue reported here, so it's here for review/test.

Status: Needs review » Needs work

The last submitted patch, 234377_tableHeader_multiple_attachments.patch, failed testing.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

Anybody knows why the patch above was not able to be applie?

dmitriy.trt’s picture

dmitriy.trt commented

hanoii, I've applied changes from your patch by hands (because I've also added support for IE 6 to sticky table headers) and it resolves my problem. Thank You!

About testing, please try to checkout latest drupal 6 code from repository, make patch for it and change version to 6.x-dev.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Version: 6.16 » 6.x-dev
Status: Needs work » Needs review
StatusFileSize
new 234377_tableHeader_multiple_attachments.patch1.65 KB

Trying agains against cvs

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

Any idea why the test is not being run on the last patch (#7)?

Status: Needs review » Needs work

The last submitted patch, 234377_tableHeader_multiple_attachments.patch, failed testing.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Status: Needs work » Needs review
StatusFileSize
new 234377_tableHeader_multiple_attachments.patch1.59 KB

I really don't get why this patch is failing to be applied, again, can somebody help me figure out why? Anyway, trying it one more time

Status: Needs review » Needs work

The last submitted patch, 234377_tableHeader_multiple_attachments.patch, failed testing.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Status: Needs work » Needs review

#10: 234377_tableHeader_multiple_attachments.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 234377_tableHeader_multiple_attachments.patch, failed testing.

anrkaid’s picture

anrkaid commented
StatusFileSize
new tableheader.js_.patch508 bytes

Here is simple patch, but it works.

anrkaid’s picture

anrkaid commented
StatusFileSize
new tableheader.patch508 bytes

sorry, patch again :)

anrkaid’s picture

anrkaid commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, tableheader.patch, failed testing.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs work » Needs review

#15: tableheader.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, tableheader.patch, failed testing.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

Your patch looks a lot simpler and better than mine :)

+++ ./tableheader.js	2011-03-25 17:32:41.133114704 +0600
@@ -13,7 +13,7 @@ Drupal.behaviors.tableHeader = function 
+  var headers = $('table.sticky-header').not($('table', context));

Why are you removing all the tables in the context? What would happen if sticky headers are kept within the context? Is that even possible?

Powered by Dreditor.

leksat’s picture

leksat commented

subscribing

leksat’s picture

leksat commented
Status: Needs work » Needs review
StatusFileSize
new tableheader-234377-22.patch436 bytes

Hmm... I don't understand why still someone not used solution from #1 . It works perfect.
Attaching the patch, maybe this will hasten elimination of this bug.

leksat’s picture

leksat commented
Status: Needs review » Reviewed & tested by the community
rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Reviewed & tested by the community » Needs review

Sorry, @Leksat - you can't RTBC your own patch.

Sadly, I have to ask if you've tested to see if this bug exists in 7.x/8.x. If it does, this will need to be fixed in 8.x first...

leksat’s picture

leksat commented

It's not my patch actually. I stole code from #1 and made a patch from it.

leksat’s picture

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

Also, I have tested this patch. It works nice. Why shouldn't we commit it and close the issue?

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

yeah, let's commit this, it does work.

skdrupal88’s picture

skdrupal88
Primary language Russian
Location Belarus
commented

subscribe...

gábor hojtsy’s picture

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

Does this apply to Drupal 7/8? Was this fixed in Drupal 7/8 already?

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.