Clean up tableheader.js [#787670]

After working on overlay a bit, I noticed some things in tableheader.js which I thought could be improved.

Minor changes:

removed the User Agent testing for anything less than IE7, and replaced it with feature detection.
introduced some caching variables. Shouldn't give a dramatic speed increase, but since tracker() is called on every scroll, every micro optimization counts.
Replaced 2 scroll bounded to 2 different elements with 1 scroll bound to an array of 2 different elements.
Namespaced scroll() and resize() events.
removed extra space in a comment.
added context in 2 jquery objects.

Major change:

Took out the eval(). My first thought when seeing the eval() would be that it would be a major speed hog. However, the eval() was probably only there because they wanted to parse the function only there. Since there is no real alternative for eval() to my knowledge, I changed the way it worked.

(By the way, this implementation feels broken, since it only allows 1 function to be passed to tableheader.js. I didn't try fixing this, because I didn't know whether it was by design or not.)

Instead of a function, I'm passing a selector, and duplicated it in tableheader js, where the function takes the selector as an argument. This is justified IMHO, because the function is generic enough to be used in tableheader.js.

I also modified the function with a second if condition for all non IE browsers (if they don't have a css filter property, they don't need to be tested for a specific filter property).

Done some benchmarking to prove my point.

  var start = new Date().getTime(); 
  var i = 1000;
  while (i--) { 
    var topOffset = eval(Drupal.settings.tableHeaderOffset + '()');
  } 
  var end = new Date().getTime(); 
  var time = end - start;

  var start2 = new Date().getTime(); 
  var i2 = 1000;
  while (i2--) { 
    var topOffset = offsetHeight('#toolbar');
  } 
  var end2 = new Date().getTime(); 
  var time2 = end2 - start2;

  alert('Eval: ' + time + ', function: ' + time2);

Windows 7, Firefox 3.6.3: Eval: 83, function: 65
Windows 7, Opera 10.53: Eval: 58, function: 38
Windows 7, Safari 4: Eval: 32, function: 20
Windows 7, IE8: Eval: 202, function: 125

Comment	File	Size	Author
#27	tableheader-cleanup7b.patch	3.73 KB	casey
#27
#22	tableheader-cleanup7.patch	9.82 KB	casey
#22
#20	tableheader6.patch	8.85 KB	casey
#20
#18	tableheader5+anchor.patch	10.34 KB	casey
#18
#17	tableheader4+anchor.patch	10.37 KB	casey
#17
#16	tableheader3-1.patch	8.79 KB	Kiphaas7
#16
#14	tableheader3.patch	9.09 KB	casey
#14
#13	tableheader2-1.patch	9.05 KB	Kiphaas7
#13
#12	tableheader2.patch	9.1 KB	casey
#12
#9	tableheader.patch	9.42 KB	casey
#9
#8	tableheader.patch	8.75 KB	casey
#8
#7	tableheader.patch	8.74 KB	casey
#7
#2	tableheaders-2.patch	5.87 KB	Kiphaas7
#2
#1	tableheaders-1.patch	6.21 KB	Kiphaas7
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Kiphaas7 CreditAttribution: Kiphaas7 commented 2 May 2010 at 10:25

File	Size
tableheaders-1.patch	6.21 KB

Forgot the patch...

Comment #2

Kiphaas7 CreditAttribution: Kiphaas7 commented 2 May 2010 at 10:52

File	Size
tableheaders-2.patch	5.87 KB

Cleaned up the previous patch, and moved the function for calculating the offset into a drupal namespaced function. That way it can also be used outside tableheader.js wihout having to duplicate the code (for example in overlay).

Comment #3

Kiphaas7 CreditAttribution: Kiphaas7 commented 2 May 2010 at 15:36

Status:

Needs review

» Needs work

Setting back to needs work, still some stuff to do...

Comment #4

casey CreditAttribution: casey commented 2 May 2010 at 20:49

@Kiphaas7, I talked to you about a more generic approach for displaced elements. Please have a look at #787940: Generic approach for position:fixed elements like Toolbar, tableHeader first.

Comment #5

Kiphaas7 CreditAttribution: Kiphaas7 commented 2 May 2010 at 20:52

Status:

Needs work

» Postponed

awesome, postponing this issue untill that one lands. No sense doing double work.

Comment #6

casey CreditAttribution: casey commented 3 May 2010 at 15:03

I also have another idea on how to improve/speedup tableheader.js

1. add a div right before the table with position:absolute, overflow:hidden and exactly match table position/size (only recalculate on window.resize)
2. copy tableheader into that div with position:fixed and top:Drupal.displace.getDisplacement("top") (#787940: Generic approach for position:fixed elements like Toolbar, tableHeader)

I think the stickytable header will then only be visible while the table is in your viewport. Advantage is that you don't need to track window.onscroll.

Edit: this idea is not going to work as overflow:hidden doesn't affect children with position:fixed

Comment #7

casey CreditAttribution: casey commented 3 May 2010 at 17:23

Status:

Postponed

» Needs review

File	Size
tableheader.patch	8.74 KB

We can already cleanup tableheader.js. If #787940: Generic approach for position:fixed elements like Toolbar, tableHeader would get committed we only have to change one line.

Patch introduces a Drupal.tableHeader object that can be reused. jQuery selectors are cached and cell width calculation is only performed when the sticky header is actually visible.

Comment #8

casey CreditAttribution: casey commented 3 May 2010 at 23:26

File	Size
tableheader.patch	8.75 KB

I uploaded a wrong patch.

Comment #9

casey CreditAttribution: casey commented 4 May 2010 at 11:34

File	Size
tableheader.patch	9.42 KB

With position:fixed test like in KipHaas7' patch.

Comment #10

jbrown CreditAttribution: jbrown commented 4 May 2010 at 11:54

eval() really shouldn't be in core: http://drupal.org/node/172169

Comment #11

casey CreditAttribution: casey commented 4 May 2010 at 12:07

Please read #7 and #787940: Generic approach for position:fixed elements like Toolbar, tableHeader.

The one line that then will be changed is the one using eval();

Comment #12

casey CreditAttribution: casey commented 13 May 2010 at 17:15

File	Size
tableheader2.patch	9.1 KB

Minor improvements.

Comment #13

Kiphaas7 CreditAttribution: Kiphaas7 commented 14 May 2010 at 10:06

File	Size
tableheader2-1.patch	9.05 KB

patch does not apply anymore since #787940: Generic approach for position:fixed elements like Toolbar, tableHeader landed.

Straight re-roll of #12.

Comment #14

casey CreditAttribution: casey commented 14 May 2010 at 10:07

File	Size
tableheader3.patch	9.09 KB

Reroll as #787940: Generic approach for position:fixed elements like Toolbar, tableHeader landed

Comment #15

Kiphaas7 CreditAttribution: Kiphaas7 commented 14 May 2010 at 11:27

Lol @ 2 identical rerolls within 1 minute :D.

Please see my followup patch in #787940: Generic approach for position:fixed elements like Toolbar, tableHeader. It moves the test for position:fixed to drupal.js, which should increase performance a bit (1 dom operation instead of 2, seperately done by tableheader.js and displace.js).

Comment #16

Kiphaas7 CreditAttribution: Kiphaas7 commented 14 May 2010 at 18:05

File	Size
tableheader3-1.patch	8.79 KB

followup for #787940: Generic approach for position:fixed elements like Toolbar, tableHeader has landed, so here's a reroll with the changed position:fixed test.

Comment #17

casey CreditAttribution: casey commented 15 May 2010 at 15:08

File	Size
tableheader4+anchor.patch	10.37 KB

I found some issues on anchors being invisible due to tableheader (and displaced elements): #546126: Toolbar interferes with anchor links and #567754: Wrong anchor adjustment in tableheader.js.

In the first we discussed using the hashchange event. This is however not sufficient; to reproduce:
- go to admin/people/permissions#module-filter
- scroll away from the anchor
- press ENTER on the addressbar of your browser; window scrolls to the anchor, the hashchange event however isn't fired.

Therefor I tried it to recognize scroll events triggerd to scroll to the anchor. And with success.

Next issue is that there might be several scripts that want to adjust the scrollTop to get the anchor visible (currently tableheader.js and displace.js). Therefor I introduced a new event "drupalScrollToAnchor". I added the trigger to drupal.js because its just a small piece of code.

(Note that this isn't working with overlay yet. For #668640: Overlay shouldn't be based on jQuery UI Dialog it just takes 3 lines).

Comment #18

casey CreditAttribution: casey commented 15 May 2010 at 15:37

File	Size
tableheader5+anchor.patch	10.34 KB

Removed an obsolete line of code.

Comment #19

Kiphaas7 CreditAttribution: Kiphaas7 commented 15 May 2010 at 17:02

looks good, tried it in a few browsers and worked. Obviously overlay needs some love, but without overlay, fine.

Overlay issues shouldn't be fixed in this issue though.

Comment #20

casey CreditAttribution: casey commented 16 May 2010 at 13:44

File	Size
tableheader6.patch	8.85 KB

I also found out that tableheader was hiding the focused element when using TAB-navigation. Therefor I opened a seperate issue: #800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu).

I wasn't really happy to add code to drupal.js and moved it into displace.js; that's what the code is about anyway. Only downside is that tableheader is dependent upon displace.js (but that shouldn't be a problem).

(So to test this patch you also have to apply #800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu))

Comment #21

Kiphaas7 CreditAttribution: Kiphaas7 commented 17 May 2010 at 00:27

Category:

task

» bug

Soooo, this is a bug report now? :)

Comment #22

casey CreditAttribution: casey commented 9 June 2010 at 18:04

File	Size
tableheader-cleanup7.patch	9.82 KB

Removed dependencies on other patches. Lets get this in first.

Comment #23

casey CreditAttribution: casey commented 28 June 2010 at 22:56

Anyone?

Comment #24

casey CreditAttribution: casey commented 12 July 2010 at 12:50

There is also a bug in the current tableheader.js in certain cases triggering the following error:

$("td" + location.hash).offset() is null

To reproduce that error open a page with a table and add anon-existing anchor to the URL, e.g.: /admin/people/permissions#nonexistent-anchor

This patch fixes that bug too (maybe this will get some attention).

But tableheader.js really is a mess.

Comment #25

james.elliott CreditAttribution: james.elliott commented 26 July 2010 at 14:24

Status:

Needs review

» Reviewed & tested by the community

The patch looks good and works well.

Comment #26

Dries CreditAttribution: Dries commented 28 July 2010 at 01:38

Status:

Reviewed & tested by the community

» Fixed

I've tested this patch and I've found it to work.

I've also looked at the code and couldn't find anything wrong with it -- it is slightly easier to read.

Hence, I've decided to commit it to CVS HEAD.

Comment #27

casey CreditAttribution: casey commented 31 July 2010 at 18:43

Status:

Fixed

» Needs review

File	Size
tableheader-cleanup7b.patch	3.73 KB

IE is still having trouble with complex tables (like on the modules page).

Also removed some code that only is supposed there if and only if #787940: Generic approach for position:fixed elements like Toolbar, tableHeader lands.

Comment #28

casey CreditAttribution: casey commented 25 August 2010 at 14:21

#27: tableheader-cleanup7b.patch queued for re-testing.

Comment #29

droplet CreditAttribution: droplet commented 11 November 2011 at 17:29

Version:	7.x-dev	» 8.x-dev
Status:	Needs review	» Needs work

Comment #30

nod_

French

Lille

CreditAttribution: nod_ commented 28 January 2012 at 12:13

Forgot this one…

The main issue was with eval(), I made a patch taking it out a different way over there : #1417378: Remove eval() from tableHeader JavaScript if someone has some time to review that'd be nice.

I'm not sure about the rest of the changes didn't look too closely.