Fix regressions and further improve tableheader.js

Yeah I wasn't sure, I went for undefined so that it wouldn't break so horribly if someone tries to use it as a number before it's computed.

On the other hand, it's new code, it can be fixed right now :)

tag

@#10: makes sense, we don't want to end up breaking natively supported implementations.

after doing some more analysis, I've come to the conclusion that this won't break native implementations as it'll just swap it to fixed once we're past a certain point.

so if we have consensus on this undefined vs null thing, this is good to go, yes?

detach

Hi guys, just reviewing the tableheader.js changes from 2012 and thought I'd provide additional input on the changes that I discovered:

- The onScroll behavior says that "This function is throttled to once every 250ms to avoid unnecessary calls." But it is not. Both the onScroll and onResize methods are fired repeatedly with no throttle/debouncing.
- The onScroll call to this.stickyPosition(null, scrollValue('scrollLeft')); is having unexpected results. Because in stickyPosition !isNaN(offsetTop) returns true for a "null" value (see these Mozilla examples). The code seems to expect that null will not be considered a number, but oddly isNaN actually *does* consider null to be a number. Very weird.

I'd suggest changing the conditionals to add "&& offsetTop !== null" to prevent the weirdness:

      if (!isNaN(offsetTop) && offsetTop !== null) {
        css.top = offsetTop + 'px';
      }
      if (!isNaN(offsetLeft) && offsetTop !== null) {
        css.left = (this.tableOffset.left - offsetLeft) + 'px';
      }

Though maybe there is a better way of checking if a number is not, this StackExchange post seems pretty definitive, but I'm not sure comprehensive checking is really necessary in the first place.

EDIT: Sorry I've noticed that all the problems with isNaN are fixed by using ECMA Script 5's "use strict". Which means we don't have any problem on any browsers except IE9, which doesn't support use strict. http://caniuse.com/use-strict

Another regression: The tableheader behavior will no longer be applied to tables loaded via AJAX, since the use of $(window).one() won't refire when new content is attached. Although you'd have to be replacing a really big table for this issue to occur. Most of the time you'd want to add/replace rows within an existing table rather than replace it, but it could still be an issue in some situations.

For #1 I don't know what I had in mind at the time. Was supposed to be.

For #2 there is already #2269129: Permissions sticky header is not place correctly.

(edit) and for the init are you sure? because to me any new ajax call will bind the tableinit function and it's just that it's executed on the first scroll event that happens. Can you reproduce your bug?

Re Rolled patch

rerolling a patch :)

Needs re-roll.

Original header is visible under sticky header (setting a z-index fixes this, but that might open a whole new can of worms

Not sure if it's still exist.

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?

Can't reproduce (also @see #17)

There should be a detach behavior to unregister tables that have been removed/replaced.

this is make sense.

On scroll/resize handlers should be debounced. The inline docs currently state they are debounced to execute every 250ms, but they are actually executed every possible scroll/resize.

For page with many sticky tables, it helps a bit. If only one or two, it's slightly bad for UX IMO.

I think we should open new issues for each problem.

Note that: No re-roll is needed. I reviewed the patch and that's not right approach now.

What about the invalidation of all tables on each DOM change?

I'm going to hijack this issue, I'm proposing some improvements in the name of helper functions.

Imagine your sticky table header works great on first page load, then you grab some table from ajax, replace the table in the DOM with the new table from ajax, well, that just broke the references in the tables collection in Drupal.TableHeader.tables

So, to solve this, I've added three helper functions, one is called Drupal.TableHeader.validateReferences()
what this function does is remove the unreferenced tables in the collection of Drupal.TableHeader.tables (tables that are no longer found in the DOM)

This explains how , if you call recalculateSticky with broken references (tables no longer present in the DOM) the table cell widths will all be returned 0 , and the whole recalculateSticky will be broken.

So to solve this, you call Drupal.TableHeader.validateReferences() and then re-initialise by calling Drupal.TableHeader.addTable(tableSelectorParam);

see patch

Also, if you have multiple sticky headers on a page, I found it useful to add another function called Drupal.TableHeader.recalculateStickys(); rather than recalculateSticky on one item , this does all of them in a for loop.

So three functions added altogether.

This is 100% backwards compatible.

Works very well now that I finally figured out how to use this library.

I'd like to provide some example code at some point as well, however the above instructions should be helpful, would probably be good to add comments to the tableheader.js file with this explanation , or add a link to documentation with an example or two.

a quick example

make a global scope variable , set it to false
like

var checkTargetStickyInit = false;
var jqWrapper = '.some-class-for-your-block';
var jqSticky = '.sticky-header'; // This is the 'copy' created by TableHeader, the sticky one, thead only.
var jqTableEvents = '.sticky-table'; // This is the table with both the header thead and rows of table data.

then as an example, some js functions I use:

  function resetStickyHeader() {
    // Since the table may have been replaced (if this was a sort operation for example.
    // Then we have to call validateReferences() because we replaced the table when we called ajax.
    var invalidTableRef = Drupal.TableHeader.validateReferences();  // the new function from the patch I just added.
    if (invalidTableRef || !$(jqWrapper + ' ' + jqSticky).length) {
      prepareForNewStickyHeader(); // validateReferences() removed the previous table, assign this event.
      Drupal.TableHeader.addTable(jqWrapper + ' ' + jqTableEvents);
      $(jqWrapper + ' ' + jqTableEvents).trigger('columnschange');
      $(window).trigger('scroll'); // This actually sets off the re-init of the sticky header.
    }
  }

  function prepareForNewStickyHeader() {
    checkTargetStickyInit = false;
    $(window).on('scroll.TableHeaderInit', {context: document}, function(e) {
      if (checkTargetStickyInit) {
        $(this).off(e); // Turn off this event;
      }
      else {
        // If you have special setup to do to initialise events on the sticky-header to it here.
        if (debuggingEnabled)
        console.log('call initialiseStickyTableHeader();');
        initialiseStickyTableHeader(); // some function that is going to add click events to the header or whatever you need post setup.
      }
    });
  }

in the function initlialiseStickyTableHeader() you do something like this:

  function initialiseStickyTableHeader() {
    // Initialise all event search results sticky-header table headers.
    $('div.some-css-class .sticky-table').each(function(index) {
      // You're in, this means the sticky table was found.
      checkTargetStickyInit = true; // Ding ding ding! See .off().
      $(this).addClass('target-sticky-header');
      // Make sure that the column for actions is added if it isn't already.
      Drupal.TableHeader.recalculateStickys(); // Now that it's initialised, recalculate.
      // Call some function here for setting up click events on your table header, or 
      // whatever post processing you need initialised on the table header
    });
  }

see comment # 38 for a verbose explanation.
#1764912-38: Fix regressions and further improve tableheader.js

interdiff shows minor change, removing setTimeout (not needed).

attaching updated patch

see comment # 38 for a verbose explanation.
#1764912-38: Fix regressions and further improve tableheader.js

interdiff shows minor change, putting comment before code.

attaching updated patch

@Quicksketch, these helper functions and detailed explanation in comment #38 (with examples) explain and provide a strategy to deal with and explain what happens when doing ajax calls (replacing a table for example) what you describe in #16.

see comment #38 and patch 39

#1764912-38: Fix regressions and further improve tableheader.js

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.