Rewrite tableheader.js [#1574738]

tableheader has several problems:

complicated computation during the onscroll event
hard to extend for contrib
messy HTML

What I'd like to see is:

No computation during scroll, listen to a custom event for updating the offset value
initialize tableheader on first scroll, not on page load.
Offload all offset computation to the modules adding the elements, (toolbar)
use data- attributes to store and get offsets
Only initialize tableheader for table longer than half screen space available (or something), tableheader is not useful for small tables (like the attached files table in issues…)
(bonus) try to get rid of the extra table by playing with the original <thead>

With that we should be able to remove to reduce tableheader footprint quite a lot.

Comment	File	Size	Author
#16	no_sticky_until_scroll.png	59.12 KB	Kiphaas7
#16	old_header_visible_under_sticky.png	65 KB	Kiphaas7
#13	core-js-rewrite-tableheader-1574738-13.patch	16.75 KB	nod_
#13
#4	core-js-rewrite-tableheader-4.patch	14 KB	nod_
#4
#3	core-js-rewrite-tableheader-3.patch	13.66 KB	nod_
#3
#2	core-js-rewrite-tableheader-2.patch	13.76 KB	nod_
#2
#1	core-js-rewrite-tableheader.patch	13.69 KB	nod_
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

nod_

French

Lille

CreditAttribution: nod_ commented 13 May 2012 at 14:36

Status:

Active

» Needs work

File	Size
core-js-rewrite-tableheader.patch	13.69 KB

Ok first try.

It's pretty different style of code but that's what it takes to clean it up. tableheader is a library, take a look at yui source, openlayers and co to see how it's usually done.

works IE8+ and the rest, it could actually work on IE7, didn't want to go to the trouble of testing.

This superseeds #1417378: Remove eval() from tableHeader JavaScript. Profiling this rewrite give a significantly better perf than the current code (and about twice better than the previous patch).

Should be commented enough. It's easier to read the patched file, pretty much everything changed. I've put a good number of named function because profiling is useless otherwise.

(edit) I've fixed a super annoying 1-2px offset that was driving me crazy too.

File	Size
core-js-rewrite-tableheader-2.patch	13.76 KB

Comment #3

nod_

French

Lille

CreditAttribution: nod_ commented 13 May 2012 at 15:00

File	Size
core-js-rewrite-tableheader-3.patch	13.66 KB

small tweak to for mobile positioning onscroll

Comment #4

nod_

French

Lille

CreditAttribution: nod_ commented 13 May 2012 at 19:34

File	Size
core-js-rewrite-tableheader-4.patch	14 KB

We should introduce throttle and debounce for scroll and resize events respectively. http://benalman.com/projects/jquery-throttle-debounce-plugin/ as well as for autocomplete.

Forgot about the toolbar toggle button. Fixed.

Comment #5

andypost

he/him

Russian

CreditAttribution: andypost commented 13 May 2012 at 20:21

does not woks in overlay
EDIT: IE9(8) and ff14a2 works fine without overlay

Comment #6

nod_

French

Lille

CreditAttribution: nod_ commented 13 May 2012 at 20:24

indeed, have not changed anything for overlay. Just waiting for some feedback on this before finishing it.

(edit) It's no big trouble to make overlay work, just got lazy towards the end.

Comment #7

Kiphaas7 CreditAttribution: Kiphaas7 commented 26 July 2012 at 09:11

Slightly offtopic: saw the change from .once() to .one(). Since this was already in jQuery 1.1, why have we (by we I mean drupal) been using .once() exclusively in the past? There must have been a good reason... Drupal 6 already came with jQuery 1.2.

+ for (var i = 0, il = tables.length; i < il; i += 1) {
Not as a complaint, but genuinely interested: why are you using i += 1 instead if i++?

+function tableHeaderReizeHandler(e) {
Why global functions? Backwards compatibility?

+ attach: function(context, settings) {
No de-attach/unattach-ish property?

Big +1 for .debounce() and .throttle() in core as a library, incredibly useful, and very small size.

It's pretty different style of code but that's what it takes to clean it up. tableheader is a library, take a look at yui source, openlayers and co to see how it's usually done.

I've not been following core issues for some months, but is that something that was already agreed upon for D8 js? i.e. use yui source/openlayers and co codingstyle?

Comment #8

nod_

French

Lille

CreditAttribution: nod_ commented 26 July 2012 at 11:39

.once and .one don't do the same thing. For drupal use, .one won't work, think of new AJAX content. .one would have been triggered with the old elements not the new ones. Same thing if you delegate.

i += 1, see jslint/hint configuration options. We decided not to go with that later on, it was unclear at the time of the patch though. should be i++ to be consistent with the rest of core.

they are not global functions, they are in the closure scope. Naming them helps with debugging/profiling so it's better than anonymous function for that purpose and don't really hinder readability, for me it actually helps.

detach: totally right, missed that part. it should be done properly :)

Still nothing deceided on that level about code style.

Comment #9

Kiphaas7 CreditAttribution: Kiphaas7 commented 26 July 2012 at 13:09

they are not global functions, they are in the closure scope. Naming them helps with debugging/profiling so it's better than anonymous function for that purpose and don't really hinder readability, for me it actually helps.

Ouch, can't believe I missed that. :)

Thanks for the update!

Comment #10

Kiphaas7 CreditAttribution: Kiphaas7 commented 27 July 2012 at 01:58

(also needs a reroll, doesn't apply against toolbar.js anymore)

Comment #11

Kiphaas7 CreditAttribution: Kiphaas7 commented 27 July 2012 at 12:20

Sorry for the third post in a row, but found this typo:

function tableHeaderReizeHandler(e) {

ReizeHandler -> ResizeHandler. Typo is consistent (e.g. you named it ReizeHandler everywhere), probably why you didn't notice it.

Comment #12

nod_

French

Lille

CreditAttribution: nod_ commented 27 July 2012 at 12:28

It's all constructive feedback, don't worry. Nice to know someone is reviewing :) Hopefully I'll have some time to reroll soon.

Comment #13

nod_

French

Lille

CreditAttribution: nod_ commented 13 August 2012 at 19:15

Status:

Needs work

» Needs review

File	Size
core-js-rewrite-tableheader-1574738-13.patch	16.75 KB

Closed #1728122: Sticky Table header tables don't inherit parent table's outer width and #1724534: Sticky Table header rows incorrectly sized if browser resized between detachments since the patch fixes both issues.

Reroll of the patch, overlay works too.

Comment #14

lorique CreditAttribution: lorique commented 21 August 2012 at 14:07

Status:

Needs review

» Reviewed & tested by the community

Test OK. Code is sane.

The tableheader has been prototyped correctly, and there is no longer any unnecessary computing going on during scrolling.

Comment #15

Dries CreditAttribution: Dries commented 24 August 2012 at 12:22

Status:

Reviewed & tested by the community

» Fixed

Committed to 8.x. Thanks.

Comment #16

Kiphaas7 CreditAttribution: Kiphaas7 commented 30 August 2012 at 12:21

Status:

Fixed

» Active

File	Size
old_header_visible_under_sticky.png	65 KB
no_sticky_until_scroll.png	59.12 KB

Think this was all introduced in this patch, therefore re-opening.

See screenshots:

Original header is visible under sticky header (setting a z-index fixes this, but that might open a whole new can of worms
If for some reason you load the page halfway a table, the sticky table will only show up upon the first scroll. Maybe trigger scroll once on pageload if the table is in the viewport?

Comment #17

Kiphaas7 CreditAttribution: Kiphaas7 commented 31 August 2012 at 03:27

So I talked a little bit with nod_ about this, and he said the sticky table creation upon scroll is on purpose, to delay execution as much as possible.

A middleground might be to execute tableHeaderInitHandler unconditionally on attach. Move the 'on scroll' bind to createSticky, after which we can test for checkStickyVisible: if it returns true, trigger scroll on the table.

This way the actual creation and insertion of the sticky table, which I think is the most costly operation, is still delayed if the table is not visible.

If that sounds good, I'll try to whip up a patch.