Think this was all introduced in the rewrite: #1574738: Rewrite tableheader.js.

See screenshots:

  1. Original header is visible under sticky header (setting a z-index fixes this, but that might open a whole new can of worms
  2. 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?
  3. There should be a detach behavior to unregister tables that have been removed/replaced.
  4. 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.
Files: 
CommentFileSizeAuthor
#25 core-js-tableheader-fix-1764912-25.patch1.75 KBManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,763 pass(es). View
#19 core-js-tableheader-fix-1764912-19.patch1.78 KBjain_deepak
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-js-tableheader-fix-1764912-19.patch. Unable to apply patch. See the log in the details link for more information. View
#5 core-js-tableheader-fix-1764912-5.patch1.83 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 40,705 pass(es). View
no_sticky_until_scroll.png59.12 KBKiphaas7
old_header_visible_under_sticky.png65 KBKiphaas7

Comments

Kiphaas7’s picture

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.

Kiphaas7’s picture

Issue summary: View changes

typos

nod_’s picture

Category: task » bug
Kiphaas7’s picture

made it a task for the last line in the OP, (no detach or events), but if it should be a bug report, fine by me :).

Kiphaas7’s picture

Aha!

table th a {
    display: block;
    position: relative;
}

Seems to be the culprit for the bleed-through. It only happens with links, and the position: relative; is responsible

Removing it from the original table (not the sticky table) seems to fix it.

nod_’s picture

Status: Active » Needs review
FileSize
1.83 KB
PASSED: [[SimpleTest]]: [MySQL] 40,705 pass(es). View
Kiphaas7’s picture

Looks like a very good and simple solution. Though I would prefer to set offsetTop to null instead of undefined. Semantically makes more sense to me (we set the variable, we just don't know what value it should have).

Should the bleed through bug be deferred to the theme/css folks?

nod_’s picture

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 :)

Kiphaas7’s picture

If they want to use it, and it's null at init, they should check if it's not null :). But that's the same if undefined was used... Not checking would be sloppy on their end :P. And yea, the code is still brand new, I doubt anyone is already depending on it :).

nod_’s picture

Issue tags: +Needs JS testing

tag

Kiphaas7’s picture

Just as a reminder for myself (or others), might want to add this to the patch as well by means of feature testing?

http://drupal.org/node/1440628#comment-6455104

var nativeSticky = window.getComputedStyle(document.querySelector('.sticky')).position.match('sticky') ? true : false;
if (!nativeSticky) {
  // execute regular js code.
}
.sticky {
  position: -o-sticky;
  position: -moz-sticky;
  position: -ms-sticky;
  position: -webkit-sticky;
  position: sticky;
}
Kiphaas7’s picture

Made a new issue for the bleed-through issue: #1780852: Sticky tableheaders bleed-through.

seutje’s picture

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

seutje’s picture

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?

seutje’s picture

Issue summary: View changes

detach

quicksketch’s picture

Issue summary: View changes
quicksketch’s picture

Issue summary: View changes

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

quicksketch’s picture

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.

nod_’s picture

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?

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
jain_deepak’s picture

FileSize
1.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-js-tableheader-fix-1764912-19.patch. Unable to apply patch. See the log in the details link for more information. View

Re Rolled patch

jain_deepak’s picture

Status: Needs work » Needs review
pwieck’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 19: core-js-tableheader-fix-1764912-19.patch, failed testing.

nod_’s picture

Issue tags: +Needs reroll
Manjit.Singh’s picture

FileSize
1.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,763 pass(es). View

rerolling a patch :)

Manjit.Singh’s picture

Status: Needs work » Needs review
googletorp’s picture

Issue tags: -Needs reroll
mgifford’s picture

Status: Needs review » Needs work

Needs re-roll.

droplet’s picture

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.