Problem/Motivation

The sticky table header on admin/people/permissions The sticky header on the permissions page magically appears in the center of your browser window when scrolling down the page. Attached are some screenshots of the issue once you scroll past the header:
Broken Permissions Table 1
Broken Permissions Table 2

Proposed resolution

Per comment #15:
Change the isNaN() function to (typeof offsetTop === 'number') and trigger the "should be visible" check on Table Header initialization

Remaining tasks

(DONE #18) Review the proposed patch (from #15).
Tested on:
Firefox - 29.0.1
Chrome - 35.0.1916.114
Safari - 7.0.3 (9537.75.14)

User interface changes

Make sure that the table is positioned at the top instead of 'halfway' on the screen when scrolling on the page.

After Patch (comment #15) Screen Shot showing resolution of scrolling issue

CommentFileSizeAuthor
#18 Screen Shot 2014-06-01 at 10.46.08 AM.jpg65.13 KBJamesGrierson
#15 fix_sticky_table_header-2269129-15.patch824 bytesmichaelfavia
#8 sticky_header-2269129-8.patch530 bytessqndr
#4 sticky_header-2269129-4-do-not-test.patch530 bytessqndr
#2 sticky_header-2269129-2.patch541 bytessqndr
broken-permissions-table-2.png201.42 KBsqndr
broken-permissions-table-1.png209.33 KBsqndr

Comments

sqndr’s picture

sqndr commented
Issue summary: View changes
sqndr’s picture

sqndr commented
Status: Active » Needs review
StatusFileSize
new sticky_header-2269129-2.patch541 bytes

This patch fixes the issue. The main problem is that the header is fixed, but no top is set for the element. With the patch, a top value is set for the element; making it sticky.

sqndr’s picture

sqndr commented

Hm. Looking at the issue again. I guess the problem must be in these lines of tableHeader.js:

Line 210:

this.stickyPosition(null, scrollValue('scrollLeft'));

This results in the following function getting executed:

Line 172-181

    stickyPosition: function (offsetTop, offsetLeft) {
      var css = {};
      if (!isNaN(offsetTop)) {
        css.top = offsetTop + 'px';
      }
      if (!isNaN(offsetLeft)) {
        css.left = (this.tableOffset.left - offsetLeft) + 'px';
      }
      return this.$stickyTable.css(css);
    },

If I set a console.log(offsetTop) inside if (!isNaN(offsetTop)) {...}, it logs null to the console every time I scroll. This should not be happening, right? Instead of the previous patch I submitted where I give the top value every time into the function, there's another way of fixing this:

this.stickyPosition(undefined, scrollValue('scrollLeft'));

When changing the null into undefined, the code doesn't go in the if (!isNaN(offsetTop)) {..} any more. The results in a correct positioned sticky header.

In short: I think the problem here is that the isNaN() returns false for isNaN(null); // false. Changing it to undefined fixed the problem. Here's some more examples:

  • isNaN("foot") -> true
  • isNaN(42) -> false
  • isNaN(null) -> false
  • isNaN(undefined) -> true

I'm not a Javascript expert, so I'm just sharing my debugging experience here, hoping that someone can come up with a proper solution. ;-)

sqndr’s picture

sqndr commented
StatusFileSize
new sticky_header-2269129-4-do-not-test.patch530 bytes

Hm, I wrote a patch changing the null into undefined as explained in #3.

sqndr’s picture

sqndr commented
Issue summary: View changes
lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +JavaScript
sqndr’s picture

sqndr commented
sqndr’s picture

sqndr commented
Issue summary: View changes
StatusFileSize
new sticky_header-2269129-8.patch530 bytes

Wrote a new patch to see if it can pass the tests.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

Looks like a slam dunk to me, works for me. Changing to RTBC.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Oh man that's such a wtf. Good catch, no idea null was actually a number :p

sqndr’s picture

sqndr commented

In my search for the answer I came across this issue on StackOverflow. In case anyone's really interested ;-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: sticky_header-2269129-8.patch, failed testing.

Mitson’s picture

Mitson commented

Hi,

I noticed the same problem.
Why to change 'null' to 'undefined'?
This !isNaN != isNumber can lead to problems in the future.

Is there a problem to use the built-in jquery function 'isNumeric'?

--- tableheader.js 2014-05-29 17:49:29.678400000 +0300
+++ tableheader.js 2014-05-29 17:52:15.806400000 +0300
@@ -171,10 +171,10 @@
      */
     stickyPosition: function (offsetTop, offsetLeft) {
       var css = {};
-      if (!isNaN(offsetTop)) {
+      if ($.isNumeric(offsetTop)) {
         css.top = offsetTop + 'px';
       }
-      if (!isNaN(offsetLeft)) {
+      if ($.isNumeric(offsetLeft)) {
         css.left = (this.tableOffset.left - offsetLeft) + 'px';
       }
       return this.$stickyTable.css(css);

This patch works for me.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +Needs reroll
michaelfavia’s picture

michaelfavia commented
Status: Needs work » Needs review
StatusFileSize
new fix_sticky_table_header-2269129-15.patch824 bytes

Ran into this myself today. Attached patch simplifies the check. isNaN() is a broken method at its core. What we are really interested in is if the value is updated and its a number. This check does just that.

It also triggers the "should be visible" check on Table Header initialization as i thought maybe it was overlooked as well. If we dont want to correctly display sticky table headers until the actual onScroll event for some reason i can remove the line.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Now that tableheaders are off by default on table I don't care as much about initializing this on page load. If we want to do that It'd make more sense to rip the whole scroll thing apart.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: -Needs reroll +frontend
JamesGrierson’s picture

JamesGrierson commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screen Shot 2014-06-01 at 10.46.08 AM.jpg65.13 KB

reviewed -

downloaded fix_sticky_table_header-2269129-15.patch
ran git apply -v fix_sticky_table_header-2269129-15.patch in terminal

viewed on page /admin/people/permissions and the sticky header now stays at the top of the window vs. showing up mid page on scroll

Tested on:
Firefox - 29.0.1
Chrome - 35.0.1916.114
Safari - 7.0.3 (9537.75.14)

JamesGrierson’s picture

JamesGrierson commented
Issue summary: View changes

Added after screen shot and noted browser that were tested to the issue summary.

JamesGrierson’s picture

JamesGrierson commented

I also opened the patch and reviewed code. I looked at the changes and they are all within scope and seem reasonable to me to address the issue.

JamesGrierson’s picture

JamesGrierson commented
Issue summary: View changes

Added more detail to the problem/motivation section along with proposed resolution details and performed general cleanup of project information.

sqndr’s picture

sqndr commented

Check, with the recent changes this still works like a charm! Awesome.

vinmassaro’s picture

vinmassaro commented

Tested and works. Thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 625463d and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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