Sticky table headers need to react properly to "show/hide weights column" link

I'm not seeing what you're seeing here ... any chance you can create a video screen capture (using Jing or something) to illustrate the problem?

Ahh ... that *is* a problem. Ok, I just redownloaded HEAD and

a) I can confirm this problem
b) This happens with or without overlay
c) It seems that the sticky table header is picking up the (hidden) weights column (see related: #448292: Drag and Drop for table rows is not accessible to screen-reader users)

Investigating

EDIT: clarifying findings so far

Ok ... it seems that the solution to this problem would be to ensure that tabledrag.js is included *after* tableheader.js. This way the code here:

// Hide weight/parent cells and headers.
  $('.tabledrag-hide', 'table.tabledrag-processed').css('display', 'none');

will also hide the "weights"

I'm not sure how we handle inter-js dependencies like this, so I'm relieving myself of duty... if someone wants to point me in the right direction I'll pick this back up

sub

subscribe.

I have not evaluated the JS code to confirm whether ensuring tableheader.js runs before tabledrag.js is desirable, or whether it fixes the bug. I have also not evaluated at what time this bug was introduced, and if the reason it was introduced was a change from tableheader.js being loaded earlier to being loaded later. Although #954804: All .js in /misc should be registered as a library did introduce other JS ordering bugs, and #39 on that issue now contains a fix for those, that issue specifically did not touch tableheader or tabledrag, so this issue was not caused by that one.

But, assuming #7 is correct (and it seems reasonable that it is), then here's a fix.

I actually found the opposite behavior in Firefox. #10 implements the wrong test, but does successfully put tableheader.js before tabledrag.js. However, that actually *causes* the bug described in this issue. OTOH, HEAD has tableheader.js after tabledrag.js, but I don't see the bug shown in #5.

As a separate issue, on Safari, my table isn't sticky either with HEAD or with #10.

Does this match what others are seeing?

Right. OK. It helps when you enable JavaScript (I usually use Safari as my no-js testing browser).

Here's what I get:
Mac Firefox 3.6:
HEAD: Works ok for me. I do not see what is shown in #5.
#10: WEIGHT column gets added to header when you scroll the page down such that header becomes sticky. When you scroll back up so the header is no longer sticky, WEIGHT column is removed.

Mac Safari 5:
HEAD: Same as video in #5.
#10: Same as #5, except additionally, a WEIGHT column is added, just like above for Firefox (as per #14).

In any case, I think based on this, the analysis from #7 wasn't fully correct, and that changing the relative order of tableheader.js and tabledrag.js, as #10 does, does not fix the problem.

Any other ideas?

@tim.plunkett marked my earlier issue http://drupal.org/node/979646 as a dupe of this, noting that here instead of just doing 'subscribe' :)

Confirmed this with Chrome. On a manage display tab with a content type with a lot of fields on it. Scrolling back up after scrolling down results in the header and table getting messed up.

Subscribing, will look at this later

Subscribing. Interesting chrome bug.

Subscribe.

(Driving me nuts for months)

This issue has been bugging me too. Ugh.

Sorry, but this is definitely not major.

This also happens to me on Windows (XP), Chrome (12.0.742.122), and Safari (5.0.5)
Works fine on Firefox 4, and IE8

This occurred for me editing a list of taxonomy terms (though it has happened in other menus as well).

When the page first loads, the header looks fine, but when you go to scroll, the columns get jacked up. Similar to what is pictured above. JPG attached to post.

Subscribing, this is an niggly UI bug that occurs in the default D7 configuration too, hopefully can be fixed soon.

UPDATE: doing some debugging shows that it's related to the width calculation in tableheader.js, commenting that out allows me to scroll and return to the top with the table header remaining intact.

However, it means the table header is average width when scrolling down the table...

It appears to be related to including hidden columns when calculating the width, am investigating.

Wow, I think I actually fixed the issue.

The problem for me was where the table header breaks when scrolling up and down a page with tabledrag on.

The widths would get all wonky on scroll and then when returning to the top I'd get the broken header as per the image in #17

The issue appears to be caused by tableheader.js applying a CSS width to table header cells, even when they are set to display:none by tabledrag.

This additional width may interfere with the calculation of width on other visible cells..

The attached patch only sets the width when the table cell is actually visible.

The only change is the additional of the conditional check

if($(this).css('display') != 'none'){}

Tested and working, fixes the table header scroll bug for me.

DT

Submitting another version of the patch with prefix included, to see if the testbot likes it better.

Previous patch used:

git diff --relative --no-prefix misc/tableheader.js > filename.patch

Attached uses

git diff --relative misc/tableheader.js > filename.patch

#27 works great ... I'd love to see this finally resolved

Reroll for coding standards, comment should have a fullstop and there should be a space after if. Otherwise identical, still RTBC.

And since the diff is mostly indentation, here is the non-whitespace change for easy review:

diff --git a/misc/tableheader.js b/misc/tableheader.js
index 949ef52..38a8799 100644
--- a/misc/tableheader.js
+++ b/misc/tableheader.js
@@ -97,12 +97,15 @@ Drupal.tableHeader.prototype.eventhandlerRecalculateStickyHeader = function (eve
     this.widthCalculated = true;
     // Resize header and its cell widths.
     this.stickyHeaderCells.each(function (index) {
+      // Only apply width to visible cells, fixes tabledrag conflict.
+      if ($(this).css('display') != 'none'){
       var cellWidth = self.originalHeaderCells.eq(index).css('width');
       // Exception for IE7.
       if (cellWidth == 'auto') {
         cellWidth = self.originalHeaderCells.get(index).clientWidth + 'px';
       }
       $(this).css('width', cellWidth);
+      }
     });
     this.stickyTable.css('width', this.originalTable.css('width'));
   }

+++ b/misc/tableheader.js
@@ -97,12 +97,15 @@ Drupal.tableHeader.prototype.eventhandlerRecalculateStickyHeader = function (eve
     this.stickyHeaderCells.each(function (index) {
...
+      // Only apply width to visible cells, fixes tabledrag conflict.
+      if ($(this).css('display') != 'none'){

Use .filter(':visible') before the .each() instead.

See http://api.jquery.com/visible-selector/

18 days to next Drupal core point release.

Right, that is much cleaner.

+++ b/misc/tableheader.js
@@ -95,8 +95,9 @@ Drupal.tableHeader.prototype.eventhandlerRecalculateStickyHeader = function (eve
+    // Resize header and its cell widths. Only apply width to visible cells,
+    // fixes tabledrag conflict.

6 months from now, everyone in this issue will have forgotten about this issue, and not a single one of you will be able to recall and explain what kind of "tabledrag conflict" is being "fixed" here.

In other words: This .filter() needs a proper summary of this issue in code, to prevent someone from removing it in the future when the code might be refactored or otherwise changed.

18 days to next Drupal core point release.

Hi,

this.stickyHeaderCells.filter(':visible').each(function (index) {}

While more succinct, doesn't appear to fix the issue, problem remains with that approach for some reason.

Here's the patch that works and includes some more commenting info.

Added code formatting space after if () as per tim's earlier adjustment.

Reattaching patch, as testbot ignores those with a -D8 suffix.

This patch also applies on D7 head.

Oh, and this issue title should probably be renamed to something more generic around

"Table headers break on scroll when sticky headers and tabledrag are active."

DT

OK, here's an alternative approach and patch that specifically excludes the hidden tabledrag cells in the tableheader sticky headers calculation.

This also works, and may be slightly faster and loads fewer DOM elements, when compared to the display:none check.

Edit: doesn't work when manual weights are displayed, as cells still have class of 'tabledrag-hide' despite _not_ being style display:none.
Furthermore, using the :visible filter instead in these places doesn't work either.

So the conclusion is it appears the solution in #39 is still preferable.

+++ b/misc/tableheader.js
@@ -26,7 +26,7 @@ Drupal.tableHeader = function (table) {
-  this.originalHeaderCells = this.originalHeader.find('> tr > th');
+  this.originalHeaderCells = this.originalHeader.find('> tr > th').not('.tabledrag-hide');

Can you try the same by replacing th with th:visible in the selector?

17 days to next Drupal core point release.

Hi sun,

Yes, I did try that earlier, but it doesn't appear to work for some reason, that is

this.originalHeaderCells = this.originalHeader.find('> tr > th:visible');

The only thing that appears to be consistently working is the if($(this).css('display') != 'none'){} check

However, after clicking the icon to show manual weight select boxes, while the header scrolls ok, it excludes the 'parent' and 'weight' labels...

It's a tricky one, the patch works for most cases, when using the standard drag+drop tabledrag functionality, but still needs work to work with the manual override option there..

Note, if you want to test this, just visit admin > blocks, assuming it's longer than the viewport, scrolling up and down there should show the table header breakage issue. It's much worse on 'manage fields' content type forms when there's alot of fields.

So I guess the conclusion is the display:none js check appears to be best, but needs work to fit with manual select box weighting with tabledrag. Nevertheless, the patch is still better than no patch at all.

Just checking admin > blocks and
this.originalHeaderCells = this.originalHeader.find('> tr > th:visible');
does appear to work ok there, but not on "Manage fields" for some reason.

I think it's because "Manage fields" includes parent and hierarchy.

I'm not sure, both approaches appear to have merit, but neither works in all cases - a tricky one.

I applied the patch from #39 and it works great for the draggable view, but introduces some issues in the manual view. I prefer having the draggable (default) work at 100%, so it's good for me.

Tested the patch in #39 and here is my feedback and an updated patch:

• The proposed solution by sun in #31 does work, you only need to apply the same filter on the originalHeaderCells too. This was missing in the patch in #32:
  

      this.stickyHeaderCells.filter(':visible').each(function (index) {
        var cellWidth = self.originalHeaderCells.filter(':visible').eq(index).css('width');

  This seems like a better solution indeed so i've used it in the patch.

• Minor: removed some trailing spaces.
• Another issue that is somewhat linked to this one: if you show/hide columns, this is not reflected in the sticky header, which should be the case. Haven't looked into doing this with this patch since it seems safer to first get the white-space issue fixed before making more changes.

#45: tableheader.js-tabledrag-conflict-fix-988930-45.patch queued for re-testing.

+++ b/misc/tableheader.jsundefined
@@ -96,11 +96,13 @@ Drupal.tableHeader.prototype.eventhandlerRecalculateStickyHeader = function (eve
+    // Only apply width to visible cells to fix tabledrag conflict
+    // where table header widths change on scroll and create broken header on return to page top.

Let's wrap this before 80 chars, and try something a little clearer. Here's a start: "Only apply width to visible table cells. This prevents an issue where table header widths change on scroll and cause bugged display of the header when returning to the top."

Also, the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch. It would also be good to have a few people test this fix in different browsers,

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Rerolled the patch for the new folder structure. The new comment was also shortened based on the proposal from @xjm.

+++ b/core/misc/tableheader.jsundefined
@@ -96,11 +96,12 @@ Drupal.tableHeader.prototype.eventhandlerRecalculateStickyHeader = function (eve
+    // Only apply width to visible table cells. This prevents a bugged display of the header when returning to the top.

Bugged? ... should be buggy? Or maybe "This prevents the header from displaying incorrectly when the sticky header is no longer visible."

Also, comments should wrap after 80 chars

27 days to next Drupal core point release.

"Bugged" and "buggy" are both acceptable to me, but @bleen18's suggestion is an improvement in either case. And yes, 80 chars max for comment lines, please. :) Break after the last full word that fits within 80 chars (and be sure to remove the trailing space). Then, add another comment line below it with the rest of the sentence.

Thanks!

The comment by @bleen18 indeed sounds better, and the comment now breaks after 80 chars :)

New patch attached!

Setting to "needs review" so testbot can certify the new patch.

Once it does, I think this can be RTBC

Let's get a couple people testing the patch and confirming the before/after behavior before we RTBC.

I can't speak to 8.x, but I can confirm that patch #52 fixes the specified problem in Drupal 7.10.

@BenStallings: What browser did you test in? Can you supply a screenshot? Things like that will help committer confidence once this issue is marked RTBC. Thanks!

I tested in Chrome 13.0.782.220. Are you asking for a screenshot before or after the patch, or both?

@BenStallings: After would be helpful, since there's screenshots for the "before" part in the initial post. Thank you.

OK, here's a screenshot of sticky headers behaving properly in Drupal 7.10 following application of the patch #52. The headers on this page exhibited the described problem before the patch.

Anyone care to test the patch in #52?

#59 shows that the bug is fixed now in the webkit browsers. It'd be good just to double-check FF and IE (preferably multiple versions) to make sure they don't have a regression with this patch.

OK, screenshots attached of Firefox 9.0.1 and IE 9.0.8112.16421 showing the same thing as #59.

I can also confirm that patch of #52 is working correctly.
Tested with chrome 16.0
and firefox 9.0.1
on ubuntu

I think we have successfully kicked the tires on this one ...

Thanks everyone!

Wow, great work on this, folks!!

Since this requires manual testing and we don't have full browser coverage in here (e.g. IE6 & IE7 are notably missing), I'm nervous about committing it to D7 atm since we have a release coming out on Wednesday. However, once that release is out I'm happy to commit this so it can go into the following release and give us 30+ days of testing.

tested on Chrome

1. Apple patch
2. scroll (show stick header)
3. Show row weights
4. scroll again (same header, it doesn't refresh )

Re: #67--Are you sure this is a regression caused by this patch? We should compare the behavior before and after the patch, not just after the patch. Let's get someone confirming that result.

@xjm,

no. it's not a regression....

sorry if im wrong. Without more context details. "Sticky table headers need to react properly". I don't think it is

anyway, the patch needs a reroll, keeps it as "needs work"

Hi @droplet, thanks for testing.

I also mentioned the issue you have in #45, but too me that also feel like a separate issue. We now fixed a jQuery/webkit bug where unidentified white space is being added (although the issue title is not specific enough, agreed).

The fact that the headers are not refreshed is a lot more complex to solve. I am willing to have a look though, if we want to fix this in the same issue.

Re: #67 I don't see it on:
Windows, IE 8
Ubuntu 10.04 latest Chromium and Firefox 9
OSX Chrome 16, Firefox 10

Small update, it looks to me, but I need to verify on at least one more machine, that the patch breaks the display in content type. So I get to see the fields and values for weight, not drag & drop handles in the manage fieds page; also no filtering for widget per field type - I see them all for all field types.

@jyve,

I looked into the coding, it really needs some more complex solution, a new even handler...

Okay. now reroll it for D8 without IE7 supports.

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

Why did you leave out IE7 testing ? Webchick specifically stated she wanted to commit the patch with the IE7 so we have 30 days of testing.

It's for D8. #1217788: Drop IE7 support in Drupal core

EDIT:
Basically I tested with IE6 too. and found interesting stuff #1422568: Backend is broken under Seven theme (IE6).
I will recap some screenshoot for this patch soon.

ah, crap, completely forgot about that, sorry :)

Thanks @droplet and @Ranko! Let's get another round of testing in with the latest patch:

• IE 8
• IE 9
• FF
• Chrome
• Safari

A pair of (cropped) before/after screenshots would be helpful. (You can attach the images to your post and embed them with the <img> tag). If you find a related bug or regression, please provide (cropped) screenshots for that as well. Thanks!

IE:

table header cut off : #1199774: toolbar layout error in IE

I'm definitely a novice (I couldn't get the patch to work)...

When I manually made the changes, I still get the same behaviour in Chrome, although FF9, IE9 and Safari 5 are OK.

That's actually a bug related to chrome and it's not from this particular patch. It's because of tableheader.js all right but it's not because of this patch, it was there before.

Hmm, is there an existing issue for the bug in the screenshot in #81?

Yes, there is. can't remember exacly where but it exists, i'll update this post when I have time to find it.

Hahaha, yeah it exists, it's this one… it's that or I did not understand the bug you're mentioning xjm.

Alright, there's two separate bugs, one that affects only webkit, and one that affects all browsers.

Steps to reproduce and test

1. Install D8 using the standard profile. Do not apply the patch yet; instead, confirm the following bugs so you know what to look for.
2. Visit admin/structure/block (not in an overlay).
3. Scroll past the table header to activate the sticky header.
4. Scroll back up.
   Webkit browsers exhibit a bug at this point; a gap appears in the normal (non-sticky) table header. Other browsers do not exhibit this bug.
   
5. Click the "show row weights" link.

6. Scroll past the table header to activate the sticky header. All browsers exhibit a bug at this point: the header for the weight column is missing.

   Chrome

   
   

   Firefox

   
   

7. Apply the patch and clear the site cache.
8. Repeat steps 2-6. If you're using a webkit browser, confirm whether #4 is resolves. In any browser, confirm whether #6 is resolved.

Just uploading some cleaner screenshot crops for the issue summary; sorry for the noise.

Updated issue summary.

Alright, I've updated the summary. Based on #81-#86, the patch needs work. We need a patch that resolves both the webkit issue and the missing column issue, and then we need to test that patch in all major browsers and confirm that:

1. The webkit issue is resolved.
2. The missing header issue is resolved.
3. There are no other regressions.

Thanks everyone!

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Confusing.. #2 bug you answered me : #68

Remaining tasks
Come up with a solution that resolves both bugs.

#1 bug is fixed ?

oh yeah \o/

Applied Patch in #988930-90: Sticky table headers need to react properly to "show/hide weights column" link to D8

Tested Step #4 in STR

Safari 5.1.2 (6534.52.7), Chrome 17.0.963.56 & FireFox 10.0.2 showed the same behavior. The bug was not found. See Screenshot 1.

Tested Step #6 in STR

Safari 5.1.2 (6534.52.7), Chrome 17.0.963.56 & FireFox 10.0.2 showed the same behavior. The bug was not found. See Screenshots 8-9, before & after scrolling.

Tested #90 with IE8 and IE9, here are the screenshots. I followed the steps to reproduce posted in the issue summary.

IE8:

IE9:

Perhaps we can get a backport to D7 now and test IE6-9? I am willing to do the testing, but I wasn't able to apply the patch to D7. After my patching attempt, any sticky headers locked up my browser.

Thanks for the testing :)

There is another issue for IE9 somewhere, droplet posted a fix for it.

@Transition, you're missing a piece in your testing: you need to scroll like you did then toggle the weight column, scroll again, check there are all the columns in the sticky and toggle then scroll again.

#1199774: toolbar layout error in IE is that other IE issue.

@nod_, I think Transition's screenshot #9 shows the result of that testing. We can have one more person test it to be sure.

I just confirmed that the Chrome headers respond properly before, during, and after scrolling for the initial state, after clicking "show row weights," and again after clicking "hide row weights." So I think this is RTBC! Thanks @nod_, and thanks to everyone who helped test this manually in its many variations. :)

Awesome! I just hit this bug over the weekend.

Committed and pushed to 8.x, but didn't apply to 7.x. Marking for backport.

rerolled against d7

Needs IE7 testing! I don't have it I don't know if it works, it should be you never know.

I had to add an extra variable to be able to do the IE7 check properly.

haha, well, test patrickd patch first in IE7. If it works then forget my patch. If there is an IE7 issue, test mine. :)

needs manual testing

Well done!! Not tracking while and everything get fixed.

TESTED PATCH #97

IE7 - 9, chrome: okay

+++ b/misc/tableheader.jsundefined
@@ -95,15 +103,23 @@ Drupal.tableHeader.prototype.eventhandlerRecalculateStickyHeader = function (eve
+    var $that = null;
+    var $stickyCell = null;
+    var display = null;

any code stardard for how to init vars ?

0 days to next Drupal core point release.

Thanks @droplet! We need to test IE6 too, unfortunately. Setting back to NR until we can confirm IE6 shows no regressions. (Would also be good to get re-confirmation on FF and Safari for the backport, and that the webkit-specific bug is still resolved.)

IE6 is always not working.

I tried to test in IE6, but things are pretty broken overall, see #1422568: Backend is broken under Seven theme (IE6).

I didn't notice any difference in IE6 with patched vs. unpatched, though. In both cases, IE6 didn't have sticky headers at all in my testing. Not sure if IE6 is supposed to have them or not.

@Cottser,

Thanks for the screenshot and test. do you test #97 or #98 patch ? We expected #97 is the right one and works well in IE7.

I did test before and same results. There isn't a regression. I'd mark it RTBC and fix the IE6 problem in other issue thread.

I tested #97.

Agreed, so long as there are no regressions in IE6, we are RTBC. Thanks @Cottser and @droplet (and thanks for referencing the existing issue for Seven in IE6).

Wow, thanks a lot for all the work and testing on this, folks! In IE, no less. :)

Committed and pushed to 7.x. Thanks!