doc: fix that 22.x docs scroll to wrong locations

[cloydlau]

See: #45131 (comment)

That PR was proposed on Octobe 2022, at that time Node 22 had not yet been released. It might be caused by scroll-padding-top: 50vh;. Therefore I submitted a PR to fix that issue.

However, that issue still exists in the Node 17 documentation. I submitted this PR to specifically address the documentation for Node 17.

[nodejs-github-bot]

Review requested:

• @nodejs/nodejs-website

[AugustinMauroy]

LGTM!

[ovflowd]

I dont think your previous changes actually caused any issue, but gotta test by myself.

The comment you received on your merged PR was unrelated to your change and actually a bug caused by recent Chromium changes.

[RedYetiDev]

In favor of #53679, but I think this doesn't have to do with the wrong scrolling, but rather the highlighting scroll speed issue, see the linked PR for more info.

[RedYetiDev]

By the way, this fixes #53594

[yume-chan]

scroll-padding-top: 50vh; causes section titles to scroll to the center of the page when a link is clicked, which is confusing. Also, dragging to scroll triggers across the entire top half of the page, making it very difficult to select text (#53594)

But it shouldn't be removed. It should be set to the real height of the header (plus some reasonable padding) so section titles won't be covered by the floating header when scrolling to them.

[ovflowd]

I 2nd what @yume-chan wrote abovd

[bukowa]

Link for ref

• Navigation Anomalies on Node.js API Documentation Website nodejs.org#6869

[cloydlau]

@yume-chan @ovflowd @RedYetiDev

But it shouldn't be removed. It should be set to the real height of the header (plus some reasonable padding)

For the documentation of Node 17, yes, that is exactly what I did in this PR. However, adding scroll-padding-top to the documentation of Node 22 can instead cause extra padding.

After some testing, for the documentation of Node 22, sometimes it requires scroll-padding-top: 70px; and sometimes scroll-padding-top: 0; is needed. It is not very stable. Strangely, after I removed scroll-behavior: smooth;, everything works fine. Maybe there's a bug with it.

You can test it by following the steps below:

1. Open https://nodejs.org/docs/latest-v22.x/api/assert.html

2. Open the browser Console

3. Run

   document.documentElement.style.scrollPaddingTop = 'initial'
   document.documentElement.style.scrollBehavior = 'initial'
   document.querySelector('#toc > ul > li > ul > li:nth-child(5) > a').click()

   but if you reload the page and then run

   document.documentElement.style.scrollPaddingTop = '70px'
   document.documentElement.style.scrollBehavior = 'initial'
   document.querySelector('#toc > ul > li > ul > li:nth-child(5) > a').click()

   or

   document.documentElement.style.scrollPaddingTop = 'initial'
   document.querySelector('#toc > ul > li > ul > li:nth-child(5) > a').click()

   you don't necessarily go to the desired location.

[jwueller]

It seems like this scroll padding is an attempt at scrolling the clicked link target to the center of the viewport. It has a lot of nasty side-effects though, and breaks user expectation in my opinion. Usually the anchors are expected to scroll to the top.

But it shouldn't be removed. It should be set to the real height of the header (plus some reasonable padding) so section titles won't be covered by the floating header when scrolling to them.

That is, in fact, already being accomplished by this bit in the same file:

node/doc/api_assets/style.css

Lines 43 to 48 in f1ac7df

	h2 :target,
	h3 :target,
	h4 :target,
	h5 :target {
	scroll-margin-top: 55px;
	}

So there is no reason to keep the scroll padding on the entire document in my opinion.

Speaking of the real header size, I suggest changing the snippet above from 55px to something like 4rem, so that it scales more cleanly with font size adjustments. And if you want to make it even prettier you can reduce it to like 1rem when the header is hidden.

Alternatively, replace the scroll-margin-top on the headings with scroll-padding-top: 4rem on the document. But having both makes no sense here, and making it relative to viewport height is definitely a bad idea in all cases.

tl;dr: Please consider merging this, since the header is already accounted for, and the current behavior is broken and also redundant.

[cloydlau]

• Solution 1: scroll-margin-top on the headings, compatible with or without headings, but may not work when used with scroll-behavior: smooth; (I have removed it for now, does anyone know why?).

• Solution 2: scroll-padding-top on the document, not compatible without headings, but works normally when used with scroll-behavior: smooth;.

[RedYetiDev]

Imo Solution 2 makes more sense, as there are cases where the user will want to highlight the headings, WDYT @ovflowd?

[ovflowd]

Imo Solution 2 makes more sense, as there are cases where the user will want to highlight the headings, WDYT @ovflowd?

Is there any situation where a page has no headings?

[RedYetiDev]

I don't think so, but I might be wrong.

[jwueller]

Number 1) requires fewer changes from the current one, but 2) is definitely more universal, since it's not restricted to only aligning the headings correctly. It also works for any other element that might get scrolled into view by any other mechanism (like scrollIntoView()).

I think it probably makes sense to put scroll-padding-top: 4rem on html, and then get rid of the entire heading rule, including the `scroll-margin-top´, since it doesn't do anything useful at that point.

[RedYetiDev]

@cloydlau IMO the commit message should be updated to better explain the change

I suggest:
doc: update `scroll-padding-top` to 4rem

But this is only a suggestion

[bukowa]

Docs are unusable, not sure if this is related but when you select some text it scrolls.=

[RedYetiDev]

@bukowa, this PR intends to fix that issue, once it lands.

---

@cloydlau, could you remove the second clause of your commit, it's too long to pass validation. IMO you really only need the first part to update stand the change.

---

I've removed the squash label, as this PR has been squashed by the author.