Fix incorrect positioning of clickable area for navigation links on Safari

[mattxwang]

Closes #1392.

Unfortunately, this PR has not actually diagnosed the root problem with the scrollBy calculation/method and Safari. However, by using the 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, 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

@pdmosses 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, 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.

[pdmosses]

why is there a magic 3 multiplier

The current (v0.7.0) code is scrollBy(0, rect.top - 3*rect.height). I think scrollBy(0, rect.top) would move the baseline of targetLink to the top of the navigation panel. Usually, targetLink is a single line, so reducing the scroll distance by 3 lines leaves two lines visible above the targetLink line. This ensures that the two previous (sibling or ancestor) links remain visible.

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

I fear it would significantly undermine the benefit of the automatic scrolling. I much prefer to see at least a bit of the context of the selected link (e.g., so as to be able to select a close sibling).

but happy to fiddle around (e.g. we could vertically align to the center via scrollIntoViewOptions

I think the {block: "center"} option is ideal: not only does it leave visible as many nearby links as possible, but also the position of the selected link is as close as possible to the middle of the navigation panel.¹

See also the detailed motivation for the center option.

though I'm not sure if this causes compatability problems).

The MDN browser compatibility overview indicates that the center option has been supported by almost all major browsers for at least 3 years. It doesn't mention IE 11.

The Can I use summary doesn't detail the degree of browser support for the center option, but indicates that IE 11 still has 0.36% global usage. If the center option would give a JS error on IE 11, it might be worthwhile to detect IE users and use the plain scrollIntoView() method for them.

Footnotes

1. When using nearest instead of center, the selected link is sometimes scrolled right to the bottom of the navigation panel, which might be annoying. ↩

[mattxwang]

I think the {block: "center"} option is ideal: not only does it leave visible as many nearby links as possible, but also the position of the selected link is as close as possible to the middle of the navigation panel.1

Great point! I've changed it in 3d6e39f (so it's live on the deploy preview); what are your thoughts? I agree that after some local testing, this behaviour is better (and closer in intention to the original code here).

If the center option would give a JS error on IE 11, it might be worthwhile to detect IE users and use the plain scrollIntoView() method for them.

I'm not too concerned about this:

1. I don't (personally) think we have an obligation to support IE 11 anymore (given the current state of the web, and the fact that MS itself has stopped supporting it)
2. one of the benefits of this keyword-argument-like browser API is that "random" keys are backwards-compatible by default, e.g.

targetLink.scrollIntoView({ block: "center", peter: "mosses" });

doesn't throw any errors in any browser, since the peter key is never checked. Similarly, I would imagine (though have not tested myself) that IE11 would just skip the block key if it didn't support block, and would give us the default behaviour.

[pdmosses]

I've tested the preview in Safari, Firefox, and Chrome, and the centred navigation scroll works as expected. The incorrect positioning in Safari has disappeared.

The JS update LGTM, and I don't get any JS errors in Safari or Chrome.

Firefox (121.0) reports the following error, but it appears also when browsing v0.7.0 so it isn't related to this PR.

Error in parsing value for ‘-webkit-text-size-adjust’.  Declaration dropped. 
[normalize.scss:13:2](https://deploy-preview-1403--just-the-docs.netlify.app/_sass/vendor/normalize.scss/normalize.scss)

[mattxwang]

Thanks for the review!

Firefox (121.0) reports the following error, but it appears also when browsing v0.7.0 so it isn't related to this PR.

Error in parsing value for ‘-webkit-text-size-adjust’.  Declaration dropped. 
[normalize.scss:13:2](https://deploy-preview-1403--just-the-docs.netlify.app/_sass/vendor/normalize.scss/normalize.scss)

Oh interesting, I hadn't noticed this before. Doing a quick look at text-size-adjust (MDN ref, caniuse seems to indicate that:

1. text-size-adjust itself is still in draft (though it's been around for a while!)
2. it is not supported by Firefox

However, Chromium (on desktop & android) seems to have supported it for a while, as does mobile safari; the latter does it with the vendor prefix -webkit. Since the property is typically only used on phones and that you can't run Firefox Desktop on most consumer phones, I'm inclined to say that this isn't too big of a problem; the property also fails gracefully.

(I found this article by Kilian Valhof to be a good starting place; it seems like most major "resets" include this)

What are your thoughts? If you think it's worthwhile, I'm happy to poke at it for a bit.

[pdmosses]

What are your thoughts? If you think it's worthwhile, I'm happy to poke at it for a bit.

Unless our users complain about it, let's ignore this discrepancy between the current versions of Firefox and normalize.scss, assuming that it will eventually be resolved.