Fix scrolling to anchor when loading the diff viewer.

Review Request #13908 — Created May 29, 2024 and submitted — Latest diff uploaded

Information

Review Board
release-7.x

Reviewers

We have a variety of ways that we link to specific places in the diff
viewer, and one of the most common and useful is clicking on the link
from a comment to jump straight to the comment in context. This was
broken a while back.

The fundamental cause here is that the selectAnchor() method is
checking whether the comment flag's element is shown, but
TextBasedCommentBlock defers its own show operation in order to let
layout settle. We therefore were always bailing early.

This change makes two fixes:

  • We now defer the whole operation for the #startAtAnchorName thing.
    This lets it run after comment flags have shown themselves.
  • We check the return value of selectAnchor() and verify that we
    actually did something before clearing out the saved anchor name.
  • Ran js-tests
  • Clicked on a bunch of links from diff comments and saw that they
    now properly scrolled the page.
  • Checked explicitly that a link to a comment on the last file on the
    page worked.

Diff Revision 1

This is not the most recent revision of the diff. The latest diff is revision 2. See what's changed.

orig
1
2

Commits

First Last Summary ID Author
Fix scrolling to anchor when loading the diff viewer.
We have a variety of ways that we link to specific places in the diff viewer, and one of the most common and useful is clicking on the link from a comment to jump straight to the comment in context. This was broken a while back. The fundamental cause here is that the `selectAnchor()` method is checking whether the comment flag's element is shown, but `TextBasedCommentBlock` defers its own show operation in order to let layout settle. We therefore were always bailing early. This change makes two fixes: - We now defer the whole operation for the `#startAtAnchorName` thing. This lets it run after comment flags have shown themselves. - We check the return value of `selectAnchor()` and verify that we actually did something before clearing out the saved anchor name. Testing Done: - Ran js-tests - Clicked on a bunch of links from diff comments and saw that they now properly scrolled the page. - Checked explicitly that a link to a comment on the last file on the page worked.
0db913e17386355678d519d3edbff39460122784 David Trowbridge
reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts
Loading...