If you link to a table row with an #anchor, tableheader.js obscures the actual row you're linking to - because it's doing what it's supposed to: putting a fixed position element at the top of the page. Visually you end up looking at the row below the one you're targeting which makes the #anchors not so useful.

To see this in action, go to: /admin/user/permissions#module-node

This came up on: http://drupal.org/node/184010 - and it's a bit more obvious on the modules administration page with that patch applied.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Title: tableheader.js interferes with #anchors in table rows » Remove tableheader.js from core
Status: Active » Needs review
FileSize
914 bytes

I am so sorry, I thought a lot about this (even lost some sleep) and while I find tableheader.js a real interesting idea currently it's more a proof of concept than a solid Drupal JS addition. Steven, I do not want this to be "another kick in the gut" I really appreciate your work but you are not around, this file is very broken, noone seems to be able or willing to fix so what can I do?

  • It breaks the select all checkbox.
  • It does not work in IE6.
  • It slows down Opera.
  • It breaks page anchors.
anders.fajerson’s picture

The select all bug has a working patch at http://drupal.org/node/154593.

It might be possible to check if there is an hash e.g if(location.hash) and then scroll down the height of the tableheader.

chx’s picture

Title: Remove tableheader.js from core » tableheader.js interferes with #anchors in table rows
Status: Needs review » Active

That's good news. Opera slowdown does not occur to all platforms I hear (and I tested too) and IE6 is being phased out anyways.

quicksketch’s picture

I'd much rather find a solution than take out the table header. This particular problem could be extra tricky though. I'll see what options we've got, though fajerstarer's idea: "It might be possible to check if there is an hash e.g if(location.hash) and then scroll down the height of the tableheader.", sounds like the approach I'd try to take also.

Gábor Hojtsy’s picture

The select all issue is now fixed in core.

ScoutBaker’s picture

Just noting that this still applies with the latest CVS. Going to /admin/user/permissions#module-node
shows the "access content" permission, rather than the "node module" section header. With all of the recent changes to tableheader.js it seemed like a good time to check.

dvessel’s picture

Accounting for anchors is really tricky but the urls mentioned here are really minor I think. Hopefully, the originating link is descriptive enough so it won't be confusing since it does fall into the right group. It only obscures the group heading, not any of the choices.

catch’s picture

Version: 6.x-dev » 7.x-dev

Bumping this to D7.

lilou’s picture

redndahead’s picture

Status: Active » Needs review
FileSize
1.41 KB

This patch will scroll the page up the amount of the height of the table header when an anchor is selected. I have tested this to work except when an anchor is chosen that is the same as the previous chosen anchor.

So if you go to admin/user/permissions#module-filter it will scroll correctly. If you then go to the url bar and click enter it will not scroll correctly. I think this will be a very rare occasion and this patch fixes 99% of the situations.

catch’s picture

Patch works well, our ids on these pages have got a bit longer - it's now: admin/build/modules#edit-modules-Core---optional-search-enable which isn't so pretty, but that's not affected by this. Not scrolling correctly when you've already been to the anchor already gone away and come back again doesn't seem significant enough to worry about. Noticed a space missing in the first code comment. My js isn't that hot, but this looks pretty sane.

redndahead’s picture

FileSize
1.41 KB

Fixed comment spacing

redndahead’s picture

FileSize
11.76 KB
11.72 KB

Some screenshot love

EDIT: some better screenshots

yoroy’s picture

So, this keeps the module (tablesection?) header visible where currently the scrolling tableheader covers it. Can't comment on code quality but this is an obvious usability improvement. Nice little fix. RTBC from the ux-team. (but leaving needs review for one more js-check :)

quicksketch’s picture

Nice work redndahead. Is there a reason why the effect needs to be animated? We could make it immediate (since its going to happen really suddenly anyway) with:

$('body, html').scrollTop(scrollLocation);
redndahead’s picture

FileSize
1.36 KB

Because I was at home and didn't have my jquery book and I thought I read that scrollTop was an extension. So the answer to your question is no, but hopefully the answer to this attached patch is RTBC.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

JS looks good. Tested in:
- Firefox 2/3 (Mac/Windows)
- Opera 9 (Mac)
- IE 7 (Windows, IE6 is not supported by tableheaders.js)
- Safari 3 (Mac)

webchick’s picture

Version: 7.x-dev » 6.x-dev

Nice. This has bugged me for a long time.

Code looks good, verified that it fixes the problem, and has a thumbs-up from a JS guru and a UX guru. Good enough for me. :) Committed to HEAD. Thanks!

Moving back to 6.x for consideration.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

tableheader.js seems to nicely align to coding standards with the string concatenation in Drupal 6, so I needed to make 'td' + location.hash into 'td'+ location.hash, but otherewise committed the patch as-is. Thanks!

Noticed that although the PHP string concatenations were fixed to the new standards of always whitespace around them, this JS file and possibly other JS files have the concatenation in the D6 style still. Sounds like work to do there :) (in another issue)

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

gunzip’s picture

Version: 6.x-dev » 6.6
Status: Closed (fixed) » Needs work

With this patch applied to tableheader (drupal 6.6) I get one error in pages linked with #fragments:

$("td" + location.hash).offset() is undefined
var scrollLocation = $('td'+ l...tion.hash).offset().top - $(e).height();

easily fixed with

      if (location.hash != '') {
+        if($('td'+ location.hash).length > 0) {
          var scrollLocation = $('td'+ location.hash).offset().top - $(e).height();
          $('body, html').scrollTop(scrollLocation);
+        }
      }
redndahead’s picture

Status: Needs work » Closed (fixed)

gunzip can you open a new issue and post a new patch?

Thanks

egfrith’s picture

gustav has posted the patch requested by redndahead at #367088: tableheader.js error with anchors and it has one positive review so far.