From da38718d7acc994dc6298c3309b084c90b00bc4c Mon Sep 17 00:00:00 2001 From: Matt Wang Date: Mon, 1 Jan 2024 14:19:24 -0800 Subject: [PATCH] Fix incorrect positioning of clickable area for navigation links on Safari (#1403) Closes #1392. Unfortunately, this PR has not actually diagnosed the root problem with the `scrollBy` calculation/method and Safari. However, by using the [`scrollIntoView`](https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView) function (which essentially does what the calculation was meant to do), this problem is "magically" solved! As a side effect, I think this makes the code easier to maintain (I myself was thinking: why is there a magic `3` multiplier?). ~~I will point out that this does change *how much is scrolled*; following the spec for the method, the sidebar is now scrolled so that the active navigation link is top-aligned with the scroll container (which in this case, is the navigation sidebar's "cutoff"). I personally am fine with this change, but happy to fiddle around (e.g. we could vertically align to the `center` via [`scrollIntoViewOptions`](https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView#parameters), though I'm not sure if this causes compatability problems).~~ I will point out that this does change *how much is scrolled*; we are now using the `center` option to `scrollBy`, which centers the target link. As Peter has commented in the PR thread, this seems to be the best compromise for maintaining the spirit of the previous calculation. ### testing Peter did a great job writing a reproducible bug report in #1392. To test this, 1. first, follow the instructions verbatim on `main`. observe that the bug appears on Safari on macOS, but not Firefox and Chrome 2. then, apply this change (e.g. check out the branch) 3. next, replay the instructions - observing that 1. the bug is fixed on Safari 2. there is no change to the behaviour on Firefox and Chrome (other than the "start" of the scroll") ### compatability On [Can I use](https://caniuse.com/scrollintoview), `scrollIntoView` has 98.15% adoption (the only "major" holdout being Opera Mini); and, the partial support from IE is about an option that we don't use. So, I'm pretty confident that we should be able to roll out this change without our users being locked out by a new-ish method. --- CHANGELOG.md | 3 ++- assets/js/just-the-docs.js | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e78b988..227e2d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ This website is built from the `HEAD` of the `main` branch of the theme reposito Code changes to `main` that are *not* in the latest release: -- N/A +- Fixed: incorrect positioning of clickable area for navigation links on Safari by [@mattxwang] in [#1403] Docs changes made since the latest release: @@ -30,6 +30,7 @@ Docs changes made since the latest release: [@mitchnemirov]: https://github.com/mitchnemirov [#1390]: https://github.com/just-the-docs/just-the-docs/pull/1390 +[#1403]: https://github.com/just-the-docs/just-the-docs/pull/1403 ## Release v0.7.0 diff --git a/assets/js/just-the-docs.js b/assets/js/just-the-docs.js index b7e3655..4f0296d 100644 --- a/assets/js/just-the-docs.js +++ b/assets/js/just-the-docs.js @@ -499,8 +499,7 @@ function navLink() { function scrollNav() { const targetLink = navLink(); if (targetLink) { - const rect = targetLink.getBoundingClientRect(); - document.getElementById('site-nav').scrollBy(0, rect.top - 3*rect.height); + targetLink.scrollIntoView({ block: "center" }); targetLink.removeAttribute('href'); } }