Closed Bug 1558616 Opened 2 years ago Closed 2 years ago

Page is scrolled to a bad place on load

Categories

(Webtools :: Searchfox, defect)

Product:
Webtools
Component:
Searchfox
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mstange, Assigned: asuth)

Details

Steps to reproduce:

  1. Load https://searchfox.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#985

Expected results:
After loading, line 985 should be on the screen.

Actual results:
Line 985 is below the viewport. The viewport is showing me 3 lines of sticky context, then half of line 901, and then lines 902 to 938. So almost 50 lines more to go until I see what I came for.

I just noticed this too. The page is doing a document.getElementById('scrolling').scrollTo(0, <value>) call where the <value> appears correct. However FF is not scrolling all the way to the desired scroll offset. So it actually seems like a FF bug at first glance.

:bz reports it's happening in Chrome too, so not a FF bug then.

To restate investigation from IRC, it turns out we have potentially racing logic:

The script tags are in the order "dxr.js" followed by "code-highlighter.js" so it's likely the above ordering is also the ordering in which the code runs.

The scrollIntoView implementation is basically trying to get the browser to do its own native scrolling to an anchor position. If an element exists with the given id already, it just leaves it up to the browser. If the element doesn't exist, it creates a div with the given id and inserts it into the DOM and hopes that the browser will then automatically scroll to that. If the browser isn't Firefox, the logic redundantly writes to window.location to trigger Chrome's native anchor scrolling.

Because the actual line-number id's are of the form l${lineNum}, we do expect the scrollIntoView logic to create a div with id ${lineNum} the first time the event fires for the line number. So for single-line highlighting, it is doing something. We also expect this to end up being somewhat of a no-op during interactive use where the user clicks around because the browser should see that the line number is already visible.

It seems like the logic running concurrently at startup breaks things, presumably due to APZ and stale offset values interfering with what code-highlighter.js is trying to do manually with scroll offsets.

The simple quick hack we just tried on IRC was to make scrollIntoView() simply immediately return. This stopped it from interfering with the sufficiently correct code-highlighter.js ready logic.

dxr.js hooks up scrollIntoView to window.onhashchange and so I think the main question is whether the hashchange logic is doing something helpful when navigating within the file via "go to definition" or using the browser's forward/back buttons (within a file).

Ah, and the answer is "define" is not an issue because when you click on "go to definition" you're clicking on a link like: https://searchfox.org/mozilla-central/define?SYMBOL&redirect=false which means that even if looks like you're going from "foo#100" to "foo#200", you're actually bouncing through the define mechanism. This results in a 301 redirect (the redirect=false thing is ignored).

Assignee: nobody → bugmail
Status: NEW → ASSIGNED

The tl;dr here is that I broke scrolling by removing position: relative from the .line-number selector, eliminating a containing block essential to the operation of the dxr.js scrollIntoView logic which is arguably brilliant when understood. My fix is going to be putting back the thing I broke and adding appropriate comments.

Analysis

Moving Parts

Firefox context

  • Per bz, if you hit enter in Firefox's location bar with an anchor present (ex: "#220"), Firefox will re-scroll to that anchor but will not generate any events visible to the page (other than the scrolling).

dxr.js

  • scrollIntoView(id, navigate=true):
    • Takes an element id. If the element already exists, it returns immediately. Note that it's intentional that this function expects to get a bare line number like "220" but that the actual line-number id's we use in the source are "l220" (line number) and "line-220". Because...
    • It finds the first line-number in the hash. So we net 220 from all of: "#220", "#220-224", and "#220,223".
    • It locates the "l" prefixed line number, and creates a div of the form <div id="220" class="goto" /> as a child of the "l220" div where the .goto selector notably contains top: -200px; position: absolute;. The intent clearly being that the browser would do its naive seeking to the synthetic anchor, putting the thing we actually wanted displayed 200px down.
      • The bug: I removed position: relative from the .line-number selector in an attempt to clean up undocumented CSS that didn't seem to have a reason to exist. This made the closest containing block for the "goto" div be the "nesting-container" which had to be position:relative to create a containing block for the position: sticky. That's why the scrolling seemed random and broken; because it was dependent on where we generated sticky lines.
    • It also does window.location = window.location if navigate is truthy and the browser is not Firefox to attempt to make Chrome seek to the anchor.
  • On jQuery ready(): scrollIntoView is invoked if there was a hash. This helps create the synthetic anchor on load.
  • "hashchange" event listener is hooked up to scrollIntoView. This helps create the synthetic anchor anytime the hash changes not performed by setWindowHash in code-highlighter.js as triggered by line number clicking.

code-highlighter.js

  • setWindowHash:
    • This method rebuilds the current state of selected lines per the DOM and sets it using history.replaceState. This does not fire the "hashchange" event and this will not trigger Firefox's scroll-to-anchor behavior. It is triggered both by clicking on line numbers as well as at startup to normalize the URL.
    • scrollIntoView(..., navigate=false) from dxr.js is invoked directly. This is done for side-effect so that if the user presses "enter" in the location bar and Firefox wants to scroll somewhere without consulting content, the anchor (which could look like "#220-225,400-500") already exists.
  • On jQuery ready():
    • The highlight is applied to line-number ("l220") and source line ("line-220") DOM lines.
    • Manual JS logic attempts to locate the correct scroll position and scroll to there with a hard-coded 150pixel offset. This is redundant assuming that the scrollIntoView mechanism is operating correctly.
    • setWindowHash() is called to normalize the URL based on the updated DOM state above. This will call scrollIntoView(..., navigate=false) which ensures that a synthetic anchor will exist if the hash did get modified.

Conclusions

  • dxr.js' scrollIntoView and its synthetic anchor nodes are necessary given that we need to support anchors of all of these varieties: "#220", "#220-224", and "#220,223"
  • The JS-directed scrolling at the bottom of code-highlighter.js may not be necessary. I'll see if I can remove it since it might make people reading the code not expect scrollIntoView to exist and be relevant. (This logic combined with the assumption that dxr.js might be full of dead code that came from the DXR project but was not relevant to searchfox helped confuse me at least. :)

https://github.com/mozsearch/mozsearch/pull/228
and just triggered on the "dev" channel so in ~3 hours https://dev.searchfox.org/ should have working line number seeking.

Fix is merged and deployed. Thanks for chasing this down and documenting all the magic!

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.