Sticky tableheaders broken in several ways

The issue seems to be being caused by the following code in tableheader.js

// Adjust width to fit cell/table.
      var ref = this;
      if (!i++) {
        // The first cell is as wide as the table to prevent gaps.
        ref = table;
        div.wide = true;
      }

When it commented out enabled doesn't dissappear - however there are gaps present in headers.

Ok got the wrong lines of code that are causing the issue but it is setting the first column to be the size of the table that is causing the problem.

// Reflow the cell.
        var ref = this.cell;
        if (this.wide) {
          // Resize the first cell to fit the table.
          ref = this.table;
        }
        $(this).width(Math.max(0, $(ref).width() - parseInt($(this).css('paddingLeft'))));
      });

Not sure what the fix for it is but it is as if the text is being rendered further to the right than it should be and is being hidden behind another div.

This problem appears when the first thead cell is centered (modules page for example).
tableheader.js clones the first thead cell with the same width as the table, its content is centered and therefore hidden behind the other thead cells. This patch makes sure the clone of the first thead cell is left-aligned.

shall we use a class instead then?

The reason why the first cell is as wide as the table is to avoid any gap in the clone header. Changing this would imply rewriting the sticky headers, which is out of the question at this stage. Let's try to find a work around.
I thought about the class, but it won't work as the width of the cell is the same of the table, and a right-aligned content will end up hidden behind the other thead cells as well.

Another workaround is to live with the fact that sticky headers don't deal with centered first thead cell, and avoid them for the first column of any table. the patch fixes the modules page which is the only one in core in that case. there is no visual difference since the thead 'Enabled' is wider than the checkboxes.

a better title will hopefully attract more reviewers.

I just came to this bit, while porting my custom theme - and got bitten quite a LOT here:
- Not only that the column 1 of floating table header "goes nirvana" (liked the old issue title BTW)
- Additionally, that underlying whole-table-sized field is protruding out of the table to the right (not dealing well with various margin/padding combinations)
- Even more, as the text inside wraps, vertical size of single cells is not kept consistent.

Conclusion: tableheader.js sucks ;)

But that's not a nice end, so I attempted to fix it somehow. Please note that this is my very first attempt on hacking JavaScript, without even reading any solid documentation before, so this definitely needs review from someone familiar with JS.

Now it keeps both width AND height of cells in sync with originals, the underlying big field is shorter by half the cell size to avoid overflows out of table, and it takes all paddings into account, so the size is now really matching original cells (which was quite not the case as I tested previously).

The problem with centered contents is solved by wrapping it inside another <div>, which is left-floated (inside the big cell) and so emulates the original cell's boundaries at original place. How is this not/RTL-aware should be checked by others, I have no clue.

All I know is, that it took me lots of time, and it works fine for me on Firefox 2.0.0.6/Linux.
Needs review - seriously!

JirkaRybka, you're trying to tackle another issue I notice some months ago: Sticky headers are broken with multiline headers.
maybe we should move to this issue, and try to fix the centered first cell at the same time as both issues seem to be related.
for your information, there are still some issues with your patch in FF Mac OS and safari.

We need someone more experienced with JS, and/or able to test on more browsers... I see these problems as quite critical, currently it's quite broken, but I already reached my boundaries here; I hardly even know what I'm doing, if hacking JS, let alone cross-browser comaptibility. So please don't expect much from me, and feel free to move to one or the other issue. My time is now too limited.

I did another try:

- The screenshots suggest, that perhaps padding is a problem in certain browsers: There was some playing with negative margins on fixed-position elements to make them bigger, which I consider rather risky. So now I use no negatives; this meant abandoning of inherited horizontal positions, and so need to track horizontal scrolling too.

- Also screenshots say, that the height of cells was somehow wrong. On busy resizing, I reproduced myself a case, where cells in the same table-row seemed to have different heights. So now I'm using the biggest cell size consistently for whole row, rather than copying heights from corresponding table-cells directly/separately.

- The problem with underlying big cell protruding out of table in some themes it a tough one. I think that reducing the size by half a cell (previous patch) was a bit dirty hack, which might fail on very different cell sizes in the row. Basically, it looks like the size of table in DOM is quite not necessarily the sum of all cells in the row. Since our goal is to reach the right edge of last cell (rather than emulate table-level things), I now based the size on distance between first/last cell, plus last cell size. This works for me.

- Also on cross-testing between Garland and my custom theme, there were quite problems with various paddings/borders. Garland have no visible borders and white background in general, but once you make cell-borders visible, it all becomes a tricky play to keep it fine across themes. Hopefully, I figured all the paddings/borders and the like well (?)

- Another problem is, that on resizing the vertical position of tables may change, as texts wrap differently, which wasn't tracked by original code - so it was quite possible to have the sticky header out of table (on different table even) after resize (until next scroll).

- Note that it takes a bit of time to adjust after resize, or HORIZONTAL scroll, and that somewhere the dimensions are with +/- 1 pixel tolerance. I can't figure that out, seems like math inaccuracy where the paddings are defined in 'em' units, and so pixel counts are not integers.

But however, this version seems bulletproof in my browser (FF 2.0.0.6 / Linux Mandriva). Please test on other browsers, whoever can. Thanks.

This is not RTL compatible, but that's out of scope: The original version is incompatible in exactly the same way (numerous issues with order of table cells swapped), and this whole thing is just a bit of sugar, not really needed to run everywhere (IE6 and below dropped already). So I disabled it for RTL too - better show nothing, rather than broken thing.

Updating title...

Another round of changes - and this time I *finally* got the nice feeling, that I got it right. We'll see whether that's true or not...

This version supports RTL, and is much more accurate in pixel dimensions - finally it turned out that it was a bad idea to use height() and width(); these often return some weird distorted values. Now I get dimensions through css('height') and css('width'), and it works like a charm. Also removed some cruft from previous struggle, so simplified (as compared to previous patch).

This needs to be tested in different browsers! I tested it on Firefox 2.0.0.6 / Linux Mandriva. IE6 and below not supported (that much is unchanged from existing core version).

Core version is broken on:
- Multiline table headers (possibly caused by just text wrapping after window resize; height of cells in row not the same)
- Horizontal scrolling (make your browser window really *small* and you'll see - no attempt to update even)
- Vertical move of tables caused by window resizing (i.e. text above table wraps differently and so pulls table out of sight, but sticky header doesn't disappear/update)
- Centered table headers (first cell is in fact wide as the table, so the text disappears behind other cells)
- Anything with visible background/borders (dimensions are not exactly matching the table)
- RTL

My patch hopefully fixes all that (on my browser quite fine); the only issue I can't fix is vertical alignment of text inside heading cells, but that's pretty minor IMO. Also my version is NOT relying on negative margins of fixed-position elements (as core version do), which I consider a bit hacky, and #10 screenshots suggest it may be one of the problems.

Now I see that I probably should have made the RTL switch on a per-table basis to support also mixed LTR/RTL pages. Going to to do that tiny improvement ASAP (so assigning myself), but leaving status as CNR, because it doesn't really affect the needed testing (which is primarily browsers compatibility).

Last tiny fixes:
- RTL recognized per-table, allowing for mixed LTR/RTL pages support
- A bit of cleanup regarding borders, so now it works fine even for edge cases like big borders and spacing between table cells.

Also note that I'm avoiding negative margins (on fixed-positioning elements) not only because of my suspicions: W3C say at http://www.w3.org/TR/CSS21/box.html#box-margin-area :

Negative values for margin properties are allowed, but there may be implementation-specific limits.

Now de-assigning myself, because I see nothing more to do here, and I'm not likely to have much time now, too. Needs review and especially TESTING in browsers other than FF 2.0.0.6/Linux.

OK on windows:

FF2: all looks good, no niggles at all. Patched version makes the multiple line table header much, much cleaner. screenshots attached for long table header text.

Opera: Same as firefox - no screenshots but exactly the same improvement on multiple line table headers.

IE7: patched version I lose all stickiness on the table header. unpatched version the width of the header is several pixels too short on each side (screenshot attached)

IE6: no stickiness patch or unpatched (is this by design?)

Safari: unpatched fine. patched - left table cell is a bit too short (visually similar to the left side of the IE7 screenshot). Doesn't fix the tableheader issue (i.e. no change at all).

Leaving at needs review. Not sure why IE7 is giving up with the patched version but if that can be fixed along with the widths, then it'd be a pretty big improvement.

Thanks A LOT for testing!
Another try attached ;)

IE7 unpatched: Actually, that page have an empty <th> cell on the left, so that's why we see a gap on the screenshot (left); there's a tiny cell sqeezed to the top. Right gap is due to inaccurate cell-widths (either due to width() returning wrong values, as I identified on FF too - maybe a jQuery bug? - or due to the risky negative margins; I avoided both in my version).

IE7 patched: The version-switch wasn't changed by the patch, so it probably doesn't work because my code dies for some reason on IE. That's tough one, because I haven't access to any IE install here at home, so can't debug that. But I tried to identify any dangerously changed bits, and the only I can come up with is lack of 'px' units on injected css dimensions. The attached patch is with 'px', but that's just a wild guess. (If that doesn't help, we probably need someone with more experience / debugging tools, here.)

IE6: Yes, IE below 7 is not supported, by design, that's not changed by the patch. As the initial issue for sticky headers say, IE<7 doesn't support fixed positioning, which is the base of the whole functionality. http://drupal.org/node/112605

Safari: Your comment suggests something, so attached patch attempts to work around possible problem (although it goes at the expense of more code needed). Basically, I abandoned height() in favor of css(), because height() - apart from being inaccurate - returned height of cell content, rather than height of whole cell, and so was inconsistent between cells in the same row. Maybe in Safari even css() is so inconsistent(?) So now I tried to put back in my workaround, developed earlier, keeping track of the maximum row height on JS-code level. If that doesn't help on Safari, I would love to remove that again, though (it makes the code long and ugly).

Also note that the unpatched version fails on horizontal scrolling (either manual, or implied on window-resize), screenshot attached.

OK quick testing:

IE7 - still no luck :(

Safari: both the width, and the multi-line table headers looking great :)

And I can confirm horizontal scrolling is broken without patch, and works with it (especially important on the permissions page).

So the only thing not working as far as I can see now is IE7 - leaving this at needs review to get more eyes on it.

Thanks!

As for IE7, some help with debug is needed here. I never laid my hands on IE7 (no luck on my Linux, nor Win98 at work), so I have no clue why it breaks. (I suppose there isn't a way for you to easily reach some error messages in IE7? I remember IE5 used to annoy me with JS-error popup windows continuously, but that's just history...)

Thinking about all the feedback here, it occurred to me that #20 say both width(!) and multi-line headers in Safari are fine now. Since the long-code-workaround in #19 actually didn't do anything with width, it's entirely possible that this was fixed only just by addition of missing 'px' units. So now attaching a version without that workaround again (but with 'px' still) - if this one doesn't break in Safari again, we can keep this simplification. If this breaks in Safari (or no-one test), then we stick with #19.

Additionally, I played a bit with namespace used (adding 'sticky' prefixes), to be absolutely sure there's no naming collision on IE7 (just another very wild guess; my lack of JavaScript skills is quite a problem, I have no clue what names might be used by IE internally. BTW for fun - as non-native speaker, I hate being forced to name things in English, it collide with reserved keywords every other day ;-)) )

Improving title - we're now discussing/fixing much more than the initial post here.
To summarize problems of current tableheader.js for anyone new on this issue:
- Hidden content of the first heading cell, if centered (or right aligned) = the initial post
- Inaccurate size of sticky cells in various browsers (even more visible for RTL)
- Height of sticky cells inconsistent across the row, if content wraps to multiple lines (another issue, now closed in favor of this one)
- No horizontal scrolling support
- No vertical scrolling update on window resize (i.e. vertical move caused by changed content line-wrapping)
- Multiple smaller problems with borders and/or spacing between cells, if present in theme (along the way, the patched version inherits all borders from original table cells, to ensure consistency in all sizes).

As for now (#22), we need some help to figure out, why it breaks on IE7, and confirm whether we can simplify the code without breaking it in Safari.

Marked http://drupal.org/node/205621 as a duplicate of this issue.

thanks for the summary, JirkaRybka. Sounds critical to me.

OK #22 fixes width but not wrapped tableheaders on safari.

Still not firing on IE7.

So I think #19 with an IE7 fix would deal with all issues. JirkaRybka mentioned he may not be around for a few days, so would be handy if someone could step in for that last bit. Leaving at needs review to get some more cross-platform testing. Also agree this really ought to be fixed for final D6 release, especially since it's so close.

Did a quick test of #22 in IE7 on clean install of drupal-6.x-dev

going to index.php?q=admin/reports/dblog gives me:

Error line 90: invalid argument

debug gives me this

in function(z,b): if(a!=undefined)c[d]=a

Reason

a = "NaNpx"

I guess it's caused by this

      $(div).css({
        'left': div.stickyLeft +'px',
        'width': div.stickyWidth +'px',
        'height': div.stickyHeight +'px'
      });

Commenting out this block of code gives me an error on line 119, if i comment out that code as well, nothing happens :p, but no more errors, I'll try to have a look later on

I managed to find a bit of time...

Attaching a new attempt. Based on #26 testing, this is coming from the #19 (long) version, as it seems to be the only one working on Safari.

attiks: Thanks a lot for testing. I just realized, that the block you quoted was missing a [0] at the end, and another block near line 119 (although not exactly there) too. These two are the only ones. So that looks like a possible source of the problem. To be honest, I have no clue what's the [0] good for, it's just a copy-paste from somewhere. But anyway, attached patch uses [0] consistently everywhere.

Tested #29, but no luck :/

$(this).css('width') on line 71 is causing problems, cause it's set to 'auto', so div.cellWidth gets NaN, I assume this will happen for all other parts on line 71 and 72.

I'll keep looking and let you know

A possible solution is to use $(this)[0].clientWidth instead of $(this).css('width') (2 location), this seems to be working on IE7 and FF2, but then I get errors on this code

    $(table.first).children('div').css({
      'z-index': parseInt($(table.first).css('z-index')),
      'borderRightWidth': $(table.first).css('borderRightWidth'),
      'borderRightStyle': $(table.first).css('borderRightStyle'),
      'borderRightColor': $(table.first).css('borderRightColor'),
      'top': parseInt($(table.first).css('borderTopWidth')) +'px',
      'left': (table.first.cellLeft + table.first.innerOffset) +'px',
      'width': table.first.cellWidth +'px',
      'height': table.maxCellHeight +'px'
    })[0];

stating that:
- $(table.first).css('borderTopWidth') is undefined
- table.first.innerOffset is undefined

If I change this to 0 it's working without errors and the tableheader stays at the top :p

I'll have a deeper look into jQuery to see if I can find a solution

it even works in opera

Hmm, perhaps we can try something like parseInt($(table.first).css('borderTopWidth') || 0) or parseInt('0' + ($(table.first).css('borderTopWidth'))) It's sad if we can't get the computed width consistently on IE.

I need to run now, no time for further work today.

Had a deeper look and problem is caused because the borderWidth is set to medium, if set to 2px there's no problem. So the question is how to get the borderWidth in pixels so we can actually use it. I guess if we can solve this all above problems (#27, #28, #30 and #31) are solved as well. I know IE has a currentstyle property, but I think it's IE only :/

Note to self: there's a jquery plugin dimensions that has an innerWidth() method

I got it working in IE, FF2 and Opera9 (all on Vista)

Inspired by the dimension jquery plugin i copied/created the following function to retrieve the css values

function num(el, prop) {
  return parseInt($.css(el.jquery?el[0]:el,prop,true))||0;
};

and changed all calls to css to use this function. I assume the problem is that not all jquery objects remain jquery objects in IE (or Opera and FF don't mind), don't ask why, don't know :/

Update: Had a closer look using big borders (thead th { border: 4px solid red; }) and there are 2 problems:
- the fixed header gets a bit wider (in FF and IE not in Opera)
- the height of the fixed version isn't correct (all browsers)

I have made an extra change to compensate the height (included in patch-10)

      var cellHeight = num($(this), 'height'); //  - div.addV;

I re-created the patch 9 for testing (yes, unified diff needed). The sizes are now broken on my FF 2.0.0.6/Linux exactly in the same way, as it was initially (i.e. while using width() and height()). Also various padding/border sizes are now broken, making the sizes inaccurate. I spent a lot of time on these calculations, to get it right - my theme have really borders on the cells, so all problems are visible.

Going to attach some screenshots soon.

Cross-posted, didn't want to change status...

Screenshots: Inaccurate dimensions on Garland (more visible on RTL, or LTR in smaller window), and on my custom theme (intentionally showing another table below, to illustrate how the tableheaders look like normally). Bottom screenshots with block-level elements outlined. FF 2.0.0.6

This is what I had before several iterations of the width/border/padding math debugging. The effect of the num() function seems to be similar to the jquery bits I avoided; this is beyond my js knowledge :-/ Can we change num() to change the behavior only when necessary (i.e. on IE7)? I guess we might even do a browser-type switch and two separate versions there.

These niggling issues keep popping up with headers. Maybe some aspects of the approach is not so good? I love the functionality but why are the headers cloned and recreated as div's? Browsers treat them very differently.

This is just a guesstimate but moving from one type of object to mimic another seems really fragile. Why not clone the headers cells into a new table and push that as the floating header. Seems like it would prevent these types of issues.

Well, than again the cell widths would be completely removed from the main table. Could be fixed with js but that's beyond me.

Feel free to try, but that's quite beyond me, too. The current approach is kept from existing version. There were negative-margin tricks, which I removed, using just plain zero-margin, zero-padding DIVs to establish the outer dimensions of single fixed cells. This seems bullet-proof to me, the rendering part is as simple and straight, as I can think of. Options like (poorly supported) css tables, and auto layout interference smell really dangerous to me, cross-browser. Also note, that originally the positions were inherited from original cells, but since that denied any chance to track horizontal scrolling, we need to set all dimensions/positions explicitly now. So we need to know them.

The issue here is, that we fail to read exact, currently rendered dimensions of the original cells (only because width is considered inner (content) width, but our DIVs need to overlap outer (border) dimensions, preferably without dirty negative-margin tricks, we're using also padding/border sizes in the calculations. We're getting similar problem with them all, it seems.)

Initially, it was all wrong (on FF at least). And it's not that we get this or that padding excluded or increased by a pixel or two: There seems to be a random, inconsistent distortion (particularly I encountered a large one for RTL), which is different for every single cell. I have no idea what and where gets added to the values, but they are completely unusable. No matter which HTML element we finally use, we need SOME correct dimension as input, which we lack.

By trial-and-error method, on FF2, I identified that width() and height() return distorted values, while things like parseInt($(this).css('width')) are correct. Then I refined the nested DIVs mechanism and dimensions, so that it reflects the original cells exactly. #29 works perfectly for me on FF2. catch tested earlier version of that successfully on various browsers, excepting only IE.

Now we identified the dimensions-reading as IE7 breaker, unfortunately. attiks changed the method, so that it doesn't break IE, but unluckily also re-introduced all these distortions along the way. We effectively circled back to the original state, more or less.

So I think the thing to do here is to:
- Keep attiks' centralized function for dimensions-reads
- Split it into two separate methods for IE/other browser types
- Ensure that method from #29 is used for non-IE
- Implement whatever is fitting for IE

Needs to be done by someone with IE/FF testing environment, as otherwise the testing iterations here on issue are going to be *long*

I looked at the num function and added some browser testing

function num(el, prop) {
  var dim = 0;
  if (jQuery.browser.msie) {
    dim = parseInt($.css(el.jquery?el[0]:el,prop,true))||0;
  } else {
    if (el.jquery) {
      dim = parseInt(el.css(prop))||0;
    } else {
      dim = parseInt($(el).css(prop))||0;
    }
  }
  // console.debug ((el.jquery?el[0].tagName:el.tagName) + '::' + prop + '::' + dim);
  return dim;
};

IE
- borders and padding looks ok, couldn't check width/height in IE
- each cell is too wide
- height changes (increase)

FF
- all borders, padding, width and height are correctly read (checked with firebug)
- paddingLeft (I guess) is off, everything is moved to the left by a couple pixels
- height changes (decrease)

Opera
- each cell is too wide
- height changes (increase)

I'll try to test some more later today, also try to look at another solution, but no promises

Testdata looks like this, so I have multiple lines in header

<table class="sticky-table">
<thead><tr>
<th>short</th>
<th>long long long long long long</th>
<th>very looooooooooooooong</th>
<th>short again</th>
<th>long long long long long long</th>
</tr></thead>
<tr>
<td>a1</td>
<td>b2</td>
<td>c3</td>
<td>d4</td>
<td>e5</td>
</tr>
<tr>
<td>a1</td>
<td>b2</td>
<td>c3</td>
<td>d4</td>
<td>e5</td>
</tr>
</table>

What I mentioned in #40 about div's being used was not entirely correct. All it was doing was gathering it's position based on the cells that contain them. Just wanted to be clear. It should have worked fine. It's only because when Steven created it, RTL wasn't considered. The other issue with unequal cell heights is really hard to avoid with the current implementation.

So, here's a patch at cloning the whole header row. This avoids the unequal cell heights and it reduces the complexity a good deal since it works with thead, not each cell content wrapped in a div.

There is one issue that I can't get working in IE7. This throws an error.

// IE7 chokes on this.
var cellcounter = 0;
$('th', this).each(function () {
  $('th', thead).eq(cellcounter++).width($(this).width() - (parseInt($(this).css('paddingLeft')) + parseInt($(this).css('paddingRight'))));
});

In this context, this is the "thead" row group and thead is a new table wrapping the cloned header row. It's used to calculate the width of each cell so they line up. –whaddyaknow? I figured it out.. sorta.

It works just fine in Safari 3, FF2, Opera 9.2 in both LTR/RTL layouts.

It's late for this kinda fix but I think if this proves to be solid (once the IE7 issue is ironed out), it's better to fix it now.

Forgot to change the resize handler.

The cell realignment code I pasted above will have to be used within the resize handler also. Once that's ironed out, it could be placed as a private function and used in both places.

For now, the overall header widths works.

It now tracks horizontal scrolling. Might be able to optimize. Safari is silky smooth but with FF there's a tiny performance hit.

http://bitard.net/table-header.mov

hrm, last one for today. Promise..

Just added the true parameter to clone() so the floating header carries over all jQuery functionality. For example, the "check all" checkbox.

http://example.com/admin/user/user

It was previously broke.

OK still nothing on IE7 for me. If it's a small bug that just need fixing, great. Otherwise looks like we have a choice of leaving it broken, or some kind of big switch statement (or not supporting IE at all with this although I doubt that'd be popular).

ff, opera both look great. In opera horizontal scrolling doesn't occur until you stop dragging, but it does match up pretty well in terms of widths - in fact it's very close to perfect.

Can confirm that checkboxes in the sticky header work in the last patch as well.

FYI: the problem is caused by -scroll.hscroll on line 86

I think the problem is that scroll is also defined as a function, but apparently FF doesn't mind the undefined in an equation, IE complains but after removing this bit, all works, only the width is of a bit.

Can someone explain the -scroll.hscroll part, is it needed?

I think the 'easiest' is to find an alternative implementation for IE, I'll look into it if/when I have some more time :/

BTW: both in FF and IE the width of the columns changes, total width stays the same. I guess we have to set the width on each th.

Ahh, I did notice something with the widths, but discounted it since I had roles called looooooooooooooooooooooooooong etc. - per th makes sense.

OK, I tested the #46 patch on my environment, which is FF 2.0.0.6 / Linux / real site with custom theme. The same tests as I did on my own patch previously.

Good news is, that this patch passed all my tests regarding table cells borders (including huge ones), and spacing between cells. But there are other problems. See attached screenshot:

1. I'm testing on the Aggregator administrative page (admin/content/aggregator) which is quite busy and have two tables. Observe the normal (non-sticky) blue tableheader.

2a. After scrolling down a bit, the cloned header was off the top of screen. Edit: I found the problem later, see below...

2b. Also width is incorrect. The left edge is OK, but size is bigger, so the cloned header overruns to the right. I guess the cloned size is equal to the containing box around table (?) FYI, my theme define a default for tables in content as .main-content table { width: 99%; margin: 4px; margin-bottom: 8px; border: 1px solid #BDCEE1; }

3. At the bottom, the page have another table.
4. I scroll down. The same happens to the bottom table heading.

5. Now, I resize the window, by just going from fullscreen to smaller windows mode. As the content area is now smaller (tabled layout), content inside the top table cells goes to multiple lines, making that table higher. Although I didn't scroll at all, after this resize I have the top table at the top-edge of window, instead of previously-shown-there bottom table. But however, the sticky tableheader doesn't update, so I see the heading from bottom table on the top one. It also goes a bit off horizontally, as my left sidebar got squeezed too, moving the whole content area to the left. This is why my version of the patch calls the scroll function at the end of resize handler too.

6. Now I scroll a bit (horizontally in this case), and the header updates. But the size of single cells go completely out-of-sync with actual table (compare to case 1.), so the result is useless visually.

Later I found out (after screenshots), that the gap at the top of screen is caused by a general setting in my theme's css (quoted above), where I have some small margin around all tables. I added the following to my style.css: table.sticky-header { margin: 0; } It helps, but should be added to system.css IMO, as the patch relies on that.
But then, the left edge of the sticky header went off, because the margins are now different (I in fact cancelled also the left margin for the sticky header). That's the second screenshot, which also shows that the sticky header is really the size of containing block around table (or even more likely, these 99% defined for original table).

Trying to account for all permutations and edge cases should come secondary (if at all). I'll see what I can do about the minor alignment issues but for now, I need help with IE7. And I don't believe it needs to run large amounts of tailored code. –see #43 for the issue.

Get the base functionality solid so we can throw this out of the critical queue.

@attiks, the -scroll.hscroll is so the header aligns properly when there's horizontal scrolling. I called it wrong in the resize handler. That and a couple of other settings will need to be pushed into a shared function since it's doing a lot of the same when resizing and scrolling. This patch fixes that. It's not shared yet but I'll clean it up soon, still needs work.

Since it was an easy fix it also resolves an issue where the fixed header doesn't show at the right points when scrolling. This happens when the table is within collapsible field sets. Collapsing/expanding throws it off since all the table heights and positions are calculated on load. It now recalculates only when the document length changes. No performance issues with this.

issues fixed so far:

1.) First cell hiding. –original issue.
2.) unequal cell heights.
3.) Horizontal tracking.
4.) Collapsing empty cells in IE7 on the ends.
5.) Issue with collapsible fieldsets. –mentioned above.
6.) Various RTL issues I think.

Todo's:
1.) Fix IE7 error for calculating cell widths. –critical
2.) Refine cell resizing calculation and add it to the resize handler. –currently not added. Calculated on load only.

Please correct and update this list.

JirkaRybka, I think I know what's causing your little misalignment issues specific to your theme. I'll try to get them in.

I think this is ready. Works in IE7. Silly browser was choking on negative integers.

It clones the whole table then removes the table body. This should help with theme specific issues since the copy will carry over all it's css properties.

Please review.

and the patch..

small error fixed.

This disables sticky tableheaders for me on both FF and IE7 now..

crossposted. reviewing now.

Tested permissions + comment pages, FF, IE7, Opera, Safari3 (on xp). header checkbox works, horizontal scrolling works, multiple lines works, width is fine. Bulletproof.

Going to test this evening with all my precious edge cases ;)

Awesome!

Further refinements on cell width calculation. It was previously off even though it was almost imperceptible.

Maybe one more review for this one.

forgot to remove a stray setting.

this is awesome work! I checked First cell hiding, unequal cell heights and horizontal tracking on Mac OS Firefox, Safari 3 and Opera. Really neat! Waiting for JirkaRybka tests to be set to RTBC.

So, tested on my setup (FF 2.0.0.6 / Linux). Generally this one looks good. On Garland really nice, RTL too. The problems I found:

- Still problem in the case, where window-resize throws another table to the top of viewport (due to line-wrapping differences) - I still got sticky header from other table shown. The attached patch fixes this by calling the whole scroll function, not just setPosition(), at the very end of tableheader.js, because vertical scrolling needs update there, too. I took this from my previous version (which was generally more of a trial-and-error), not sure if the syntax is nice.

- (minor) The 100px constant on the bottom of table was too big IMO, while scrolling down, the sticky header disappears, while I have still some 5 table rows on the screen. 50px seems more fitting to me.

- [NEEDS WORK] I have still incorrect size of the sticky header on my theme (screenshot). I discovered, that it's caused by margin set on the table. My tables have left/right margins of 4px. Both are added to the size of sticky header - I guess it have something to do with the fact, that fixed positioning is out of normal flow (?) I had previously problem with the negative margins in original version too...
* If I comment out the margins from my theme, the problem goes away. (Only little misalignments by 1px remain, which I think is due to borders of that size.) That's not a solution, just proves the source of the extra length.
* If I hardcode the size of my margins into setPosition(), subtracting both from table width, and adding the left one to its position, then it fits well.
* But unfortunately, I somehow can't read the actual margins from the table element. This is what needs work. My JS knowledge and time are both very limited.
* Garland doesn't show this problem only because the left/right margins on tables are set to zero there. If I add a few pixels margin to Garland, the header size goes off there, too.

- (untested) We should remove everything from the cloned table, excepting the header. I think that beside TBODY we need to remove also (rarely seen, but possible) TFOOT and CAPTION.

Attached patch contains the first two changes only.

OK I'm marking this back to RTBC. I don't see any reason why table margins can't be fixed in a followup issue if there's a clean solution, but otherwise this deals with every possibility we've found, including both the critical bug and lots of normal ones. I tested JirkaRybka's last patch and it deals with even ultra-minimised windows on all browsers I tested, although I'm not sure if that's the exact edge case he's discussing. Otherwise there's no change on the substantive issue here so this is ready.

eh, sorry.. last one. This should fix any remaining issues. If your custom theme still has alignment problems, I'd say fix it from your theme. Entirely possible.

Rearranged a few functions so it makes more sense but functionally identical with one exception.

- (untested) We should remove everything from the cloned table, excepting the header. I think that beside TBODY we need to remove also (rarely seen, but possible) TFOOT and CAPTION.

This is an excellent point. The only tags it considers is whatever is output through theme_table so I added the caption tag to remove.

This is ready to go in.

Previous commits invalidated the patch. Updated..

Thanks for the work, committed!

I was using an outdated tarball and missed that, sorry. re-tested and it's still good.

cross posted. resetting status.

Great! Thanks for testing guys.

Thanks a lot too, now the sticky tableheader is reasonably fine :) Really big bugfix, and I did very little on the final version.

I'm going to follow-up, though:
1. The only tags it considers is whatever is output through theme_table so I added the caption tag to remove.
I strongly disagree with limiting the removal only to default implementation of theme_table() because:
* tableheader.js (once added to the page) works on *all* tables with a header, not just these rendered via theme_table(). This may include also other tables, such as 3rd party embeds, theme-hardcoded infos, and most importantly full-HTML formatted content submitted to the site. All these are quite allowed to use *all* valid HTML elements, not just theme_table() equivalent set.
* Even more, theme_table() is themable, and may be altered by theme in any possible way.

So attaching a tiny patch, adding TFOOT to the removed elements, so completing the HTML-valid set. (I tested this patch by hardcoding a table with footer to the bottom of Garland page.) It's one-liner, which runs only once per page, so it should be OK.

2. It's also inconsistent that tableheader.js is only added for theme_table(), but fires on all tables on the page then.

All other tables will be sticky/non-sticky depending on presence/absence of a Drupal-rendered table on the same page. This affects even layout-tables in tabled themes, but these have usually no headings, luckily. I guess, that I'm going to have tableheader.js on all pages once my site gets all needed contribs for 6.x, as I use the Event module, with a calendar table in sidebar (it already happened as I tested Event HEAD a while ago). So I'll probably have sticky headers also on HTML tables in my content. (Would be nice BTW)

I think we should either:
- Add tableheader.js to all pages, or
- Limit it to only fire on the theme_table() produced tables, by adding a class to these tables and fire js only on tables with that class.

3. If your custom theme still has alignment problems, I'd say fix it from your theme. Entirely possible.
I tried now: Unfortunately, it's nearly impossible. I tried to theme the cloned header in my style.css, but that simply doesn't work. JavaScript overwrites all the dimensions explicitly later. So this means that Drupal 6.x currently disallows any margins around tables, entirely! Once there is a margin, sticky tableheader breaks (in FF at least), and I can do nothing about it.

So, the only solution that works is to refactor the theme, to avoid margins on tables. Unfortunately, due to optical weight of table borders (visible ones, where table is really a table, and not just lines of text as Garland shows it), it looks like the table being negative-indented then. I'm not going to accept that.

This leads to need for another div wrapper around the whole table, so that the table itself have no margins, and the div serve for them instead. I can't introduce the new div via theme-hardcoded JavaScript, because that will cause the tables theming to break with JS disabled. So finally I can only add the needed div by overriding the theme_table() in my template.php. Then, change my css to define table margins on a div instead :-/ It's a lot of hack, and the theme_table() override is really not so trivial theming to be expected from everyone who wish to indent tables by few pixels. There are css-only themes, even!

For some mysterious reason (probably collapsing of margins and so on), the div wrapper margins doesn't work properly too (on FF at least), but that's easily fixed by defining the gap as padding instead.

I totally failed on creation of this div wrapper inside tableheader.js, because no matter what I do, I can't read margins from the table element. But I seem able to set them to zero at least.

The only solution I can think of:
- Set margins on tables to zero in tableheader.js, enforcing the non-breaking condition. It's nasty to themers, but seems like no alternative is available (?) (Another way is style attribute in theme_table(), but then sticky header will break if fired on non-Drupal-rendered tables.)
- Give themers the div wrapper instead, with a reasonable class, adding it to standard theme_table() implementation.
- Document that Drupal 6.x doesn't support margins on tables, and that these should be defined on the div instead.

It's awful, I know. Do we have a better option? I didn't roll a patch for this bit yet, because I'm really unsure here.

1.) Yes, looking back at it now. tfooter should have been included.

2.) Have you tested this? I assumed whatever's passed as the "context" targeted specific tables.

3.) Use the !important declaration. It'll override anything set by jQuery. I've seen when !important doesn't work but it all depends on how specific the selector is too. If "table.sticky-header" doesn't work then try "html.js body ... table.sticky-header" or anything in between. !important will eventually take and once it does, jQuery should have no say in how it's presented.

This appears to have broken drag and drop: http://drupal.org/node/208312

Not sure whether to close that one or re-open this one, but setting to critical and CNW for now.

Also it'd be handy if Alessandro could confirm if this is pre or post patch, which browser etc.: http://lists.drupal.org/pipermail/development/2008-January/028221.html

OK so I retested #38 with drag and drop and it's fine. So it looks like dvessel's approach is a bit too bulletproof ;)

In that case I see three options:
1. fix dvessel's approach so it doesn't interfere with drag and drop (this'd have to happen pretty fast since it's now blocking rc)
2. roll back the patch completely while we sort this out.
3. roll back, patch with #38 instead, keep this open as "normal" and continue to work on refining either approach.

I'm not able to actually test right now, but here's a possible hint :
- tabledrag uses the table id attribute. If cloning the table creates a duplicate id, then it is likely to cause problems.
- OTTOMH, I'd say tableheader behaviour should be applied before tabledrag. Not sure what's the current order

yched is right, firebug shows duplicate ids on the tables.

attached patch forces a unique ID on the clone inside tableheader.js - I have no idea about core js coding standards and just copied the relevant bit from here, but it works for me. Since the clone is only the head of the table, not anything where functionality is happening, it ought to work I think.

its pre-patch

I'm using Firefox 2.0.0.11

Hard to test any script performance over IE, that lame browser has no abbility at all :)

Cool ! I'd suggest a pattern like the following, though.

  .attr("id", function (arr) {
      return arr[0] + "-clone";
   });

Besides this id handling should only be applied if the table *has* an id - the example on http://docs.jquery.com/Attributes/attr produces ids like 'div-idundefined' :-)

Can we just remove the id attribute (and possibly others) from the cloned header, then, somehow? I think we *should* fix it, rather than rolling back. The now committed version is short and nice, while #38 was still broken in many ways (I only just re-rolled for unified diff, otherwise I didn't like that one much).

As for #71:

2.) Yes, I tested. I hardcoded a plain HTML table at the end of page.tpl.php (plus a bunch of BR's to allow scrolling further down), and it WAS sticky on pages, where Drupal renders some other table (such as various administrative pages).

3.) This is not about css specificity. Of course I double-checked that my settings really apply to the element, as css is concerned. Margins on the fixed-position cloned header have no effect for me (FF2), although the js does nothing about margins. The js writes width of the element, which seems to be unaffected by css margins. I need to adjust the width written from tableheader.js - and that can't be done from theme. I can't hardcode width into css (with !important), as it needs to be dynamic, js-updated. I have three ideas:
a) Find a way to retrieve margin settings from the table element (seems unslovable to me, now), and adjust widths in tableheader.js,
b) Make tableheader.js to accept some extra "position/width correction" setting, to be injected from theme (how?),
c) Consider margins on tables unsupported, set them to problem-free zero value, and introduce some other element around to serve for them. (easiest IMO).

Tested and it works pefect for me on FF 2.0.0.2, linux sles.

@catch: another way would be setting the id with the class:

    var headerClone = $(this.parentNode).clone(true).insertBefore(this.parentNode).attr('id', 'table-header').addClass('sticky-header').css({
      position: 'fixed',
      visibility: 'hidden',
      top: '0px'
    });

@yched: why not set an standard table header id like 'table-header'?Edit: no way, id must be unique on the page.

@theborg: yes we can put it there, but the function generating the unique id would have to be put on one line.

I like the idea of removing the id completely from the clone, .attr('id','') just gives <table id="">, and .attr("id",null) isn't working for me either. According to this: http://dev.jquery.com/ticket/118 it should be possible to remove an attribute with null though.

To remove the id.

    var headerClone = $(this.parentNode).clone(true).insertBefore(this.parentNode).removeAttr('id').addClass('sticky-header').css({

doh!

That works great for me. Patch attached.

OK did some testing on FF2, IE7 and Opera. Seems to fix the issue fine with no side-effects (drag and drop, header checkbox and the original situations here all tested).

One thing is that themers will lose the id for targeting on sticky table headers, but I'm not sure that's an issue, and it'd be the same thing with auto-generated ids. Maybe theme_table() could duplicate the id to a class, or core tables could do this themselves in the definition. Not critical though.

One thing is that themers will lose the id for targeting on sticky table headers

True, but i guess that was already the case with the the previous implemention of tableheaders, right ?

Looks like it. So I can't see any further issues. If someone else could give it a couple of minutes testing and rtbc that'd be handy.

To target table headers we can use the class 'sticky-header' provided.

In case the id is really needed, this is patch (credits should go to catch) that adds '-header' to the original table name when is not empty.

Tested on FF in Windows and it works ok.

.sticky-header doesn't target the same as table#block, but that doesn't matter 'cos #89 is a lovely patch which solves even that minor issue. Tested, works.

theborg: why remove the attr, when you set it right away anyway? Does it not modify it? (I guess it does modify it)

Tested without the removeAttr and it's functionally identical, so yes it appears to be redundant. Quick version with that taken out.

since headerID is defined on the line above, couln't that be :

if (headerID  != '') {

saves a jquery call :-)

Apparently it could.

I can confirm the latest patch fixes the problems in the block admin page that quicksketch discovered last night (empty region message not going away and blocks not saving).

As long as this didn't inadvertently break something else, I think this is good to go and a much-needed fix.

All testing seems to be signing this looks right, and the code was simplified with the feedback, so I went ahead and committed this. Thanks!

Altering the header id really shouldn't break anything else.

@JirkaRybka, your issues are going out of scope IMO and this is getting way too long. Any other issues, should go in a new queue.

cross post. :)

Okay, going to open a new issue, then (not today, lack of time)...

But can we consider the patch at #70 here, please? It got lost somehow in the (more critical) later posts, but otherwise is tightly related to previous progress. And is really small.

Not a breaker anymore ;)

How about putting it all in a new queue. Would be nice to have tableheader.js targeting tables that are actually called by theme_table. :)

Well, allright. As soon as I find time, I'll try to re-summarize it whole on a new issue. Going to take quite long, and I'm still not convinced that it's necessary to close this issue so hastily, without being fully fixed, and re-explain all that on a new one :-/ You know, it somehow hurts and demotivates to have a posted patch buried without consideration like this, time is hard to find.

Going to sleep over it, to get more positive feelings. ;)

JirkaRybka: it is no secret, that RC2 is going to be out in hours, so the rush was to get the fix in in time. Also, people are not keen on reading up 102 follow ups, so therefore the suggestion to open a new issue. If you feel strong about continuing here, believing that the knowledge gathered in other follow ups helps your suggestions, feel free to pursue that.

Yeah, sorry JirkaRybka. Do what you will with this queue. I can't stand overblown queues and I tend to ignore them most times. Assumed others would feel the same.

If you start a new issue, you don't have to explain everything. Go straight to the point on what needs fixing and I'll assist if needed. Keep it simple. ;)

Fair enough, my temper went down, already ;) Thinking of it, the commit of the patch in fact changed my tiny in-discussion follow-up into a brand new bug-report, as the patch is no more under development, it's core code now. I've always difficulty to accomodate to such things quickly, it requires me to start thinking differently in a few minutes (like finding and explaining steps to reproduce rather than contradicting someone's statements directly, or setting an issue CNW...) The tabledrag thing should have been a new issue too, though.

But no time to continue today, anyway. Good night to all.

Follow-up on TFOOT and inconsistent tableheader.js triggering: http://drupal.org/node/208991

see also a related issue: safari crashes when clicking admin/build/modules

And it's broken default values on radio buttons as well:
http://drupal.org/node/208179

Another follow-up: Problems with margins on table element
http://drupal.org/node/209072