This is another split from http://drupal.org/node/194590 - particularly #70 point 3. On that long issue, we fixed the most critical problems with tableheader.js (thanks all for great work), but however, a few bugs are still unsolved, my follow-up on that got buried under some other stuff, and finally I was told to better open a new issue. So, here is another one - taking it in small chunks ;)

All the way through testing the various tableheader.js versions, I had serious problems with my custom theme. Basically, the fact is that the layout of sticky header breaks, if any left/right margin is defined on the table element. This is unsolvable from theme level, because it involves wrong widths written by JavaScript, so any css-hardcoded sizes will either fail, or break the functionality.

From JavaScript perspective, this is a bit mysterious to me, but I seem to understand that:
- width is not rendered the same on the sticky header, probably because it's fixed positioning (out of normal flow, margins interacting with viewport rather than elements around original table).
- So, it needs a correction in JS, which in turn needs to have the value of margins on the table element. All my attempts on reading these failed. Seems like we simply can't read these by JS.
- This means that sticky header simply just breaks on any non-zero margins, and we can't fix that.

So, the only solution I can see is to have zero margins on tables, and wrap them into some other element (to be able of setting margins on that one). Unfortunately, that's not easy for themes: First, if you set margins on a table, it breaks sticky headers, and no hint what to do. Then, if someone figure out the need of extra element around the table (like me), it's pretty difficult to add one. theme_table() is a monster, really not simple theming, for the sake of a few pixels indentation.

Additionally, also top margin is a problem - if defined on the table, sticky header goes off the top of screen. This one is solvable in theme's css, after a bit of research on DOM, but would be nice to fix once for ever here, too.

So, finally to the attached patch:
- Sets zero margins (left/right/top) on the table, before cloning it. This enforces consistently the non-breaking condition. (Themers won't be under false impression that they can safely use margins there, and users will get nice headers across all site-theming, for sure.)
- Provides a div wrapper around tables instead, class "table-margin", defaults set to zero.
- Adjusts Garland (moving margins to the new element).

The final effect is, that you only just need to define margins on div.table-margin instead of table, and tableheader.js won't break (even if margins are ignored due to stale css).

Tested successfully on my custom theme. If someone wants to see these effects on Garland, css must be hacked to add some left/right margins to tables (otherwise core themes use no margins there, coincidentally).

All my testing was done on FF 2.0.0.6 only. I don't know which other browsers show the bug too, but I guess that patched version should be fine (because it *removes* edge cases, by setting margins to zero, which is additionally equal to Garland, on which tableheader.js was heavily tested before).

CommentFileSizeAuthor
sticky-table-margins.patch2.74 KBJirkaRybka

Comments

dvessel’s picture

dvessel commented

Yeah, this is tricky. I don't think something like this should go in. It's too intrusive.

An alternative is to wrap your table in a div and apply padding. Don't use margins at all especially since the are not calculated the same. IE and possibly even Opera add do not add margins to its total calculated width.

After that, things should flow nicely.

dvessel’s picture

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

Besides, the patch is too specific to your case. Marking won't fix.

JirkaRybka’s picture

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

Too specific to my case???? I've no problem with converting to paddings, but otherwise this is a regression from core changes as I see it, now 6.x-dev denies theming, as I can't define any gap around table without breaking things, and I have no alternate element to work on, and I have no reasonable way to add my own element. Core now makes it mandatory to have table borders glued to content-area borders, in fact. This have nothing to do with the fact, that I got bitten in my particular theme; any similar theme will have the same problem. Garland survives only because it shows everything without visible borders, and I might even say, THAT is a way too specific style.

I'm not happy with this fix, but I can't see a better one. :-/ It only just makes things consistent and solvable.

And BTW I dislike the habit here, that anything broken beyond default core functionality (which is really minimal, when it comes to core themes) is immediately a won't fix. This is why I must hack core heavily every time I upgrade, community is generally not willing to accept fixes of certain sorts, even if provided. Seems to me like that, sorry.

Sorry for the above rant, my English and today's free time doesn't permit for more refined wording. Feel free to briefly explain any substantial bits I'm missing here, if needed...

Basically I would like to discuss this, get some help perhaps, and fix this regression for all in 6.x-dev. If this gets a won't fix again, I'll have to delete sticky headers from my site entirely, most probably (another core hack to maintain, bleh), and I believe I'm not the only one (although I can live with that somehow; it would not be the first one of that sort).

dvessel’s picture

dvessel commented

I mentioned a perfectly sane way to fix your problem. Anyone with a decent understanding of designing for cross compatibility will know that all margins are not equal so it's best to avoid in in situations like yours. There's a place for margins but not in this case.

Your work around adds complexity that's not needed. Finding every single issue and piling up code to work around it is not a smart thing to do. Either get to the root cause and fix it there or know the limitations and find the simplest approach possible. Once something goes in, it's there for a long time while your current problem may not apply in the future but the "fix" you'll have to live with. Who knows, more complexity usually means more things to deal with later. K.I.S.S ;)

JirkaRybka’s picture

JirkaRybka commented

Ok, I see there's not much understanding for this problem... Since the root cause seems to be unsolvable due to JS limitations (correct me if I'm wrong), I'll choose from the other possibilities while upgrading to 6.x. On this issue, I just - seems like that - failed to make Drupal theming less of a nightmare. Or is there still a chance to get this fixed (?)

Rowanw’s picture

Rowanw commented

I agree that this is an issue that needs to be addressed and I don't think the patch is too specific at all, but I'll do some of my own testing later to investigate themer's options.

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented

I'm not sure, but is the 'px'-part really nessecary?

+    $(this.parentNode).css({
+      marginLeft: '0px',
+      marginRight: '0px',
+      marginTop: '0px'
+    });

I mean, '0px' is the same as '0em', or '0pt' or '0'.. I'm not fully up with jQuery and it's .css(), but it's just something that I thought of when reviewing your patch..

JirkaRybka’s picture

JirkaRybka commented

Most probably, 'px' is not necessary. Trying to remember why I added that, I can't come to anything except trying to stay on the safest side, after some problems where I suspected missing 'px' to be a problem, but I think that was never more than just a suspicion... So in the end I must say, that I included that bit without really thinking about it, and we can most probably remove these 'px'. It's pretty minor, though, so leaving CNR now. Let's see Rowanw's feedback before another re-roll.

Thanks for feedback.

Rowanw’s picture

Rowanw commented
Status: Needs review » Needs work

The patch doesn't apply correctly with the latest HEAD.

$ patch -p0 < sticky-table-margins.patch
patching file ./includes/theme.inc
Hunk #1 FAILED at 1228.
Hunk #2 succeeded at 1308 (offset 6 lines).
1 out of 2 hunks FAILED -- saving rejects to file ./includes/theme.inc.rej
patching file ./misc/tableheader.js
Hunk #1 FAILED at 10.
1 out of 1 hunk FAILED -- saving rejects to file ./misc/tableheader.js.rej
patching file ./modules/system/system.css
patching file ./themes/garland/style.css
dpearcefl’s picture

dpearcefl commented

Is this issue still a need?

Status: Needs work » 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.