Skip to content

Fix auto-scroll "bounce" when using the Jump To Next Unresolved Discussion button

Thomas Randolph requested to merge tor/defect/jump-to-next-scroll-bounce into master

What does this MR do and why?

For #343484 (closed)

The main purpose of this MR is to change the behavior of the Jump To Next Unresolved Discussion button on the Merge Request Diffs page to put the hash of the Note into the URL rather than the file blob.

Timeline

MR Description
👉🏻 We are here! Remove some bounce from automated scrolling so that the final position is the correct note that it should have jumped to.
!71508 (merged) Use an intersection observer (this is why the final pixel values matter!) to update "seen" versus "unseen" for discussions as the user scrolls.
This adds more utility to the Jump... button and makes it behave more consistently with what a user expects.

Why

By placing the file blob in the URL, we create two problems:

  1. The URL doesn't change to reflect what the user just did (jumped to a discussion)
    • Sharing this URL will not bring a new user to the same location the current user is viewing
  2. Because we're using a virtual scroller, we need to scroll manually to a fixed pixel location. Setting the file blob in the URL results in another scroll when the browser-default behavior takes place, which scrolls to the wrong location.

By updating to use the ID of the note itself, the URL reflects the location the user is looking at, and any browser-native behaviors leave the scroll position exactly at the correct element.

Solution

Unfortunately, this is a bit more complicated than just changing the set from the file blob to the note ID.

In this MR, you'll see a few things happening:

  1. When scrolling to the file, there are fewer scenarios that put that file blob into the URL
  2. When jumping to a discussion on the Diffs page, we also pass along the ID of the first note in that discussion
  3. Once the jump has finalized, the hash is updated in the URL
  4. In this URL update, if virtual scrolling is enabled, the note element has its ID masked while the hash updates
    • This is to circumvent the default browser behavior of scrolling to an ID that matches the hash, which is broken when virtual scrolling is enabled.
  5. Once the hash has updated in the URL, the ID is unmasked in the DOM.
  6. If virtual scrolling is disabled, the hash is updated directly in the URL without any other steps.
    • The browser correctly scrolls to the note in this case.

Caveats

There is still a small amount of jitter during the automated scrolling.

However, the final scroll always ends at the relevant Note, which fixes downstream bugs that rely on the answers to questions like "what happened in the last scroll event?"

Previously, the answer was sometimes, "the user scrolled up above a discussion." While that's technically true, it wasn't the intent, and only happened because of the large bounce.
Now, the answer is "the user has scrolled to a discussion," even though there's a small jitter. That the answer to that question is correct in context is all that really matters for this fix.

Screenshots or screen recordings

This deals mostly with small pixel values that are hard to see in videos and screenshots.

The easiest part for me to watch is the browser scrollbar. It will usually have a small "stutter" when it moves to the first location followed very shortly by moving to the second location.

How to set up and validate locally

  1. Create an MR with diff files
  2. Add three separate discussions to some lines in the MR
    • These should each be separated by a page or so of scrolling so the bounce effect can be seen
    • The bounce is often most pronounced on discussions that are not at either the top or the bottom of the page, because these locations often have viewport-based limitations
  3. Click the "Jump To Next Unresolved Discussion" button
  4. Repeat the previous step as necessary

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Thomas Randolph

Merge request reports