Floating header in PR files view covers selected line

[maxrothman]

In the PR files view (e.g. https://github.com/<owner>/<repo>/pull/<number>/files), when a line has been selected, the floating file header covers the selected line.

• Browser: Chrome
• Operating System: MacOS 10.13.3

Steps to reproduce:

• Go to the files view for a PR with a relatively large change (several pages of scroll)
• Click a line. Note the URL changes to something like https://github.com/<owner>/<repo>/pull/<number>/files#diff-<sha>-L<num> and the view jumps to the selected line.

Expected result:

Actual result:

• HTML of the section where the issue occurs:

<tr>
  <td id="diff-4e08e0b4eed84c9a9d19372de85fa474aL3" data-line-number="3" class="blob-num blob-num-deletion js-linkable-line-number selected-line"></td>
  <td class="blob-num blob-num-deletion empty-cell selected-line"></td>
  <td class="blob-code blob-code-deletion selected-line">
    <button class="btn-link add-line-comment js-add-line-comment js-add-single-line-comment" data-path="src/Cron.dockerfile" data-anchor="diff-63f2befbc4fcc350af2072e9060ca77a" data-position="3" data-side="left" data-line="3" type="button" aria-label="Add line comment">
      <svg class="octicon octicon-plus" viewBox="0 0 12 16" version="1.1" width="12" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M12 9H7v5H5V9H0V7h5V2h2v5h5z"></path></svg>
    </button>
    <span class="blob-code-inner">-<span class="pl-k">RUN</span> apt-get update &amp;&amp; apt-get -y install cron</span>
  </td>
</tr>

This problem occurs because when the anchor is added to the URL (via a click event handler) the view jumps to put the element at the top of the screen. Since the floating file header is also at the top of the screen, it covers the line.

I played around with various solutions to this problem, but most of the ones I tried depend on being able to change the height and margin-top of the :before pseudo-element, and github is already using it to put the line number before the line.

[Mottie]

Hi @maxrothman!

The problem is GitHub uses JavaScript to position the element on click. And since we're dealing with table rows, I haven't been able to find any tricks to get the element to scroll into view - see StylishThemes/GitHub-FixedHeader#9 (comment).

Do you have any ideas?

[maxrothman]

The problem is GitHub uses JavaScript to position the element on click.

That doesn't appear to be true anymore. I did a little debugging and here's what I found:

The event listener triggers the scroll is this one in https://assets-cdn.github.com/assets/frameworks-70361af718ac8e05b3aaf9cf0c4cc8bd.js:

function s(e) {
    var t = function(e, t, n) {
        var r = []
          , o = t;
        do {
            if (1 !== o.nodeType)
                break;
            var i = e.matches(o);
            if (i.length) {
                var a = {
                    node: o,
                    observers: i
                };
                n ? r.unshift(a) : r.push(a)
            }
        } while (o = o.parentElement);return r
    }((1 === e.eventPhase ? u : c)[e.type], e.target, 1 === e.eventPhase);
    if (t.length) {
        n(e, "stopPropagation", r),
        n(e, "stopImmediatePropagation", o),
        a(e, i);
        for (var s = 0, h = t.length; s < h && !l.get(e); s++) {
            var p = t[s];
            d.set(e, p.node);
            for (var v = 0, m = p.observers.length; v < m && !f.get(e); v++)
                p.observers[v].data.call(p.node, e)
        }
        d.delete(e),
        a(e)
    }
}

This basically seems like some kind of event dispatcher. After it filters to the node it's looking for, it calls p.observers[v].data.call(p.node, e), which is this function in https://assets-cdn.github.com/assets/github-6df5d6f70023b7d0db8303ef9484889c.js:

i.on("click", ".js-linkable-line-number", function(e) {
    window.location.hash = this.id,
    e.preventDefault()
});

So it seems they're relying on native browser behavior to scroll the element into view.

[Mottie]

Yeah, it's difficult to read & make sense of minified code. Either way, I wasn't able to find a solution that would allow offsetting the scroll position.

[maxrothman]

Here's a thought: most of the tutorials I found about how to solve this issue used a relatively-positioned :before, but they're already using :before to put in the line number. What if we instead put the line number in with :after and used :before to bump the scroll down?

[Mottie]

If you do that it distorts the table row and makes it as tall as the :before definition. And if you position it absolutely, the :before isn't included in the scroll position calculation.

[maxrothman]

Hm, you're right. Oh well, custom userscript it is.

[the-j0k3r]

@maxrothman see #612 though for me the annoying portion was fixed, its the same floating header that causes your issue. you can fix your issue also by implementing that code in #612 (comment)

[maxrothman]

Yeah, I figured I could disable it, but I don't really want to. I like the sticky behavior, I just also want the line anchors to work properly 🙁

[the-j0k3r]

Yes, ;) Hows GitHub support nowdays? I rather stick hot needles into my eyes and fillet my own skin than to even consider goin in https://github.com/contact.ever again 👁️ 📌 🔪 ☠️

[the-j0k3r]

its not perfect but a top position of 30px instead of 39px sort of makes this a non issue.

  .pull-request-tab-content .diff-view .file-header {
  position: sticky !important;
  position: -webkit-sticky !important;
  top: 30px !important;
  z-index: 6 !important;
}

The caveat is, if you hit refresh the selected line will be covered again or if you select the same line twice until you select another line.

[Mottie]

Setting the top to 30px makes the buttons get overlapped by the sticky header. We could move those buttons down, but I've had a hard time with adjusting them in the past, particularly the first file.

And it still doesn't fix anything if you're also using the fixed header userstyle.

[the-j0k3r]

I did say it wasnt perfect, I would move the buttons down tbh,

As for using yet another script as a reason not to do this, Im not sure what to say to that considering this sticky behaviour is not even default either.

It works for my use case and likely for everyone else who doesnt use that fixed header script, which should also have some similar hack.

I vote for one less impinging pestering bug.

[the-j0k3r]

Again not perfect, please adjust to your liking and does not account for non default sticky header styles/scripts,

If there is a problem with those its beyond the scope of this issue IMO and they need to be addressed separately.

@supports (position: sticky) or (position: -webkit-sticky) {
  .pull-request-tab-content .diff-view .file-header {
    position: sticky !important;
    position: -webkit-sticky !important;
    top: 39px !important;
    z-index: 6 !important;
  }
  .pull-request-tab-content .file-header {
    height: 39px !important;
  }
  .pull-request-tab-content .diff-view .file-actions,
  .pull-request-tab-content .diff-view .file-info {
    margin-top: -2px !important;
  }
}

The caveats still apply and I dare repeat its not perfect but its MUCH better than as is with just GHD.

/me waves fist at prettier-github meddling bot which should wait 5 minutes before fiddling.

[maxrothman]

I've tested #690, and it seems like it works pretty well! @Mottie I'm not sure I understand your comment, what other visual changes are caused by this change? I didn't see any visual overlapping. @the-j0k3r perhaps you could make a side-by-side screenshot comparison?

[the-j0k3r]

perhaps you could make a side-by-side screenshot comparison?

before

after

[the-j0k3r]

I have to reopen this due to new information available from #724 (double height file headers)

So the issue is found in areas like https://github.com/wpilibsuite/allwpilib/pull/1022/files#diff-b0bc4c1c897b5abdb841f75a8f7896f7R3 when we encounter these extra height file-headers the line selection is lost behind the height of that header.

Everything works in normal height lines and #724 is fixed

So question is, do we live with this bug and hope its not so common or do we revisit the fix for this issue again or do we just remove the whole block and dammed be sticky file-headers or what.

You need to apply 8405aea because that also fixes #724 in this issue area and perhaps to better understand the issue.

@Mottie @silverwind

[silverwind]

Untested theory:

.selected-line {
  position: relative;
  top: 30px;
  display: none;
}

.selected:after {
  content: attr(data-line-number);
  position: relative;
  top: -30px;
}

This should move the active line number (which is the link target) 30px below, hide it, and introduce another line number element in the old place. Probably need to tweak styles on the pseudo element but I think this may work and solve this issue reliably.

[the-j0k3r]

I like where your head is at :)

Testing this out added that inside/outside the solution code (for good measure)
the attribute is definitely wrong, what this does is changes the selected line number only it doesnt actually show any selected line.

But its a step in right direction whatever is happening is well after the header.

gif with that code inside solution

[silverwind]

This works as a pure CSS solution:

  [id^=diff-].selected-line {
    position: relative !important;
    top: -120px !important;
    left: -10000px !important;
  }
  [id^=diff-].selected-line:before {
    content: attr(data-line-number) !important;
    position: absolute !important;
    top: 120px !important;
    left: 10000px !important;
    background: #2e2e2e !important;
    color: #7b7b7b !important;
    box-sizing: border-box !important;
    display: block !important;
    padding: 0 10px !important;
    width: 100% !important;
  }

But GitHub's JS is interfering and repositioning the scroll offset on page reload, so I think this is not possible without JS unfortunately. Maybe move the sticky header to it's own userscript and fix it properly there, possibly by disabling GitHub's repositioning thingy.

[the-j0k3r]

Page reloads be dammed, Is it not very unlikely you're chugging away reviewing away and select a line and then hit refresh for the hell of it? If it was accidental refresh, is it a huge deal to find the line again?

I dont think either are important deal-breakers and that issue was already present from the get go before any of these fixes.

So if it works sod it. :)

  @supports (position: sticky) or (position: -webkit-sticky) {
  .pull-request-tab-content .diff-view .file-header {
    position: sticky !important;
    position: -webkit-sticky !important;
    top: 40px !important;
    z-index: 6 !important;
  }
  [id^=diff-].selected-line {
    position: relative !important;
    top: -120px !important;
    left: -10000px !important;
  }
  [id^=diff-].selected-line:before {
    content: attr(data-line-number) !important;
    position: absolute !important;
    top: 120px !important;
    left: 10000px !important;
    background: inherit !important;
    color: inherit !important;
    box-sizing: border-box !important;
    display: block !important;
    padding: 0 10px !important;
    width: 100% !important;
  }
}

That works very well the inherited colors also work funnily enough, So I say we go with this revised version.

[the-j0k3r]

Better yet the padding should be

      padding: .03em 10px !important;

That way the number aligns behind the selected line just right I couldn't get there with pixels to remove the darker what looks like border but its not.

[silverwind]

Found the actual JS that offsets the scroll:

    function Xn(e) {
        var t = e.ownerDocument;
        e.scrollIntoView(),
        t.defaultView.scrollBy(0, -Qn(t))
    }
    function Qn(e) {
        Vn();
        var t = e.querySelectorAll(".js-sticky-offset-scroll");
        return Math.max.apply(Math, le(Array.from(t).map(function(e) {
            var t = e.getBoundingClientRect()
              , r = t.top
              , n = t.height;
            return 0 === r ? n : 0
        })))
    }

Basically, they query the height of .js-sticky-offset-scroll which we did not modify previously and which was stuck on the default 60px. I've now changed its height to accomodate for both sticked elements (40px + 32px), which should properly fix this issue.

[the-j0k3r]

Add to reopen this since

a) its not fixed (see comments in 5a246df
b) the gap between the sticky header and top is back part of discussion in #612 (fixed in ba6adbf and now broken again)
c) 5a246df causes a larger gap at top of files changed tab.

#730 really does fix #598 this and #612