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

cloydlau · 2024-07-01T10:13:13Z

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 · 2024-07-01T10:13:19Z

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 · 2024-07-02T02:20:53Z

By the way, this fixes #53594

yume-chan · 2024-07-02T09:28:01Z

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 · 2024-07-02T09:29:07Z

I 2nd what @yume-chan wrote abovd

bukowa · 2024-07-02T10:43:53Z

Link for ref

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

cloydlau · 2024-07-03T07:22:41Z

@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:

Open https://nodejs.org/docs/latest-v22.x/api/assert.html
Open the browser Console

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 · 2024-07-04T14:20:03Z

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 · 2024-07-04T15:18:56Z

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 · 2024-07-06T13:34:32Z

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

ovflowd · 2024-07-06T17:22:16Z

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 · 2024-07-06T21:56:09Z

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

jwueller · 2024-07-07T16:41:10Z

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 · 2024-07-08T18:05:41Z

@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 · 2024-07-09T01:46:39Z

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

RedYetiDev · 2024-07-09T02:13:57Z

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

targos · 2024-07-10T07:25:23Z

@ovflowd Can you please review again? I hit the problem that makes it impossible to select text today and it is really annoying.

ovflowd · 2024-07-10T09:20:30Z

@ovflowd Can you please review again? I hit the problem that makes it impossible to select text today and it is really annoying.

I don't think this PR fixes that, @targos. This PR is about when scrolling to a section, it being off by a bit or by a lot. To be honest I don't know what is the root cause of the issue you are facing, but it is indeed annoying as hell.

ovflowd

LGTM! This should fix the wrong offset.

nodejs-github-bot · 2024-07-10T09:24:45Z

Landed in 661ae62

targos · 2024-07-10T09:31:15Z

I don't think this PR fixes that

I'm not sure, but I downloaded the artifact from https://github.com/nodejs/node/actions/runs/9851882411 and tested locally. The issue is gone.

ovflowd · 2024-07-10T10:00:00Z

I don't think this PR fixes that

I'm not sure, but I downloaded the artifact from nodejs/node/actions/runs/9851882411 and tested locally. The issue is gone.

Interesting. Could be, since it is adding padding non-stop to the scroll itself. Yeah this should fix then. We landed it. I assume it will ship only with the next release?

targos · 2024-07-10T10:05:55Z

Yes. Documentation for a specific release, like other assets, is immutable.

jwueller · 2024-07-10T12:22:24Z

I don't think this PR fixes that, @targos. This PR is about when scrolling to a section, it being off by a bit or by a lot. To be honest I don't know what is the root cause of the issue you are facing, but it is indeed annoying as hell.

It does, because the padding was spanning 50% of the viewport. That caused browser select-auto-scroll thresholds to behave differently, and a bunch of other potential problems like the ones described here and in the linked issues. The scroll padding was (ab)used to achieve an effect that it isn't intended for, thereby changing unrelated semantics.

ovflowd · 2024-07-10T12:47:43Z

I don't think this PR fixes that, @targos. This PR is about when scrolling to a section, it being off by a bit or by a lot. To be honest I don't know what is the root cause of the issue you are facing, but it is indeed annoying as hell.

It does, because the padding was spanning 50% of the viewport. That caused browser select-auto-scroll thresholds to behave differently, and a bunch of other potential problems like the ones described here and in the linked issues. The scroll padding was (ab)used to achieve an effect that it isn't intended for, thereby changing unrelated semantics.

Well, if you read my last comment I actually acknowledge that it does.

PR-URL: #53662 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: James M Snell <jasnell@gmail.com>

jwueller · 2024-07-13T15:02:21Z

Well, if you read my last comment I actually acknowledge that it does.

Yeah, that's my bad. I should have read more thoroughly. I was mostly focused on summarizing the exact mechanism and not specifically trying to single you out.

PR-URL: #53662 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 1, 2024

cloydlau mentioned this pull request Jul 1, 2024

doc: avoid hiding by navigation bar in anchor jumping #45131

Merged

AugustinMauroy approved these changes Jul 1, 2024

View reviewed changes

ovflowd requested changes Jul 1, 2024

View reviewed changes

jasnell approved these changes Jul 1, 2024

View reviewed changes

RedYetiDev approved these changes Jul 2, 2024

View reviewed changes

mikeesto mentioned this pull request Jul 2, 2024

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

Closed

aymen94 approved these changes Jul 3, 2024

View reviewed changes

cloydlau requested a review from ovflowd July 3, 2024 12:01

RedYetiDev mentioned this pull request Jul 3, 2024

scrolls when highlinthing text with mouse. #53712

Closed

mikeesto mentioned this pull request Jul 8, 2024

Page tends to scroll up on "trying" to select nodejs/nodejs.org#6906

Closed

RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 8, 2024

cloydlau force-pushed the main branch from ef44052 to 07f7c6d Compare July 9, 2024 01:37

RedYetiDev removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 9, 2024

doc: update scroll-padding-top to 4rem

5c55c3e

cloydlau force-pushed the main branch from 07f7c6d to 5c55c3e Compare July 9, 2024 06:08

ovflowd approved these changes Jul 10, 2024

View reviewed changes

ovflowd added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 10, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2024

nodejs-github-bot merged commit 661ae62 into nodejs:main Jul 10, 2024
19 checks passed

aduh95 pushed a commit that referenced this pull request Jul 12, 2024

doc: update scroll-padding-top to 4rem

9fc5da7

PR-URL: #53662 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 mentioned this pull request Jul 12, 2024

V22.5.0 proposal #53826

Merged

aduh95 pushed a commit that referenced this pull request Jul 16, 2024

doc: update scroll-padding-top to 4rem

2d92ec2

PR-URL: #53662 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cloydlau commented Jul 1, 2024 •

edited

Loading

nodejs-github-bot commented Jul 1, 2024

AugustinMauroy left a comment

ovflowd left a comment

RedYetiDev left a comment •

edited

Loading

RedYetiDev commented Jul 2, 2024

yume-chan commented Jul 2, 2024

ovflowd commented Jul 2, 2024

bukowa commented Jul 2, 2024 •

edited

Loading

cloydlau commented Jul 3, 2024 •

edited

Loading

jwueller commented Jul 4, 2024 •

edited

Loading

cloydlau commented Jul 4, 2024

RedYetiDev commented Jul 6, 2024 •

edited

Loading

ovflowd commented Jul 6, 2024

RedYetiDev commented Jul 6, 2024

jwueller commented Jul 7, 2024 •

edited

Loading

RedYetiDev commented Jul 8, 2024

bukowa commented Jul 9, 2024 •

edited

Loading

RedYetiDev commented Jul 9, 2024 •

edited

Loading

targos commented Jul 10, 2024

ovflowd commented Jul 10, 2024 •

edited

Loading

ovflowd left a comment

nodejs-github-bot commented Jul 10, 2024

targos commented Jul 10, 2024

ovflowd commented Jul 10, 2024

targos commented Jul 10, 2024

jwueller commented Jul 10, 2024 •

edited

Loading

ovflowd commented Jul 10, 2024

jwueller commented Jul 13, 2024 •

edited

Loading

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

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

Conversation

cloydlau commented Jul 1, 2024 • edited Loading

nodejs-github-bot commented Jul 1, 2024

AugustinMauroy left a comment

Choose a reason for hiding this comment

ovflowd left a comment

Choose a reason for hiding this comment

RedYetiDev left a comment • edited Loading

Choose a reason for hiding this comment

RedYetiDev commented Jul 2, 2024

yume-chan commented Jul 2, 2024

ovflowd commented Jul 2, 2024

bukowa commented Jul 2, 2024 • edited Loading

cloydlau commented Jul 3, 2024 • edited Loading

jwueller commented Jul 4, 2024 • edited Loading

cloydlau commented Jul 4, 2024

RedYetiDev commented Jul 6, 2024 • edited Loading

ovflowd commented Jul 6, 2024

RedYetiDev commented Jul 6, 2024

jwueller commented Jul 7, 2024 • edited Loading

RedYetiDev commented Jul 8, 2024

bukowa commented Jul 9, 2024 • edited Loading

RedYetiDev commented Jul 9, 2024 • edited Loading

targos commented Jul 10, 2024

ovflowd commented Jul 10, 2024 • edited Loading

ovflowd left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 10, 2024

targos commented Jul 10, 2024

ovflowd commented Jul 10, 2024

targos commented Jul 10, 2024

jwueller commented Jul 10, 2024 • edited Loading

ovflowd commented Jul 10, 2024

jwueller commented Jul 13, 2024 • edited Loading

cloydlau commented Jul 1, 2024 •

edited

Loading

RedYetiDev left a comment •

edited

Loading

bukowa commented Jul 2, 2024 •

edited

Loading

cloydlau commented Jul 3, 2024 •

edited

Loading

jwueller commented Jul 4, 2024 •

edited

Loading

RedYetiDev commented Jul 6, 2024 •

edited

Loading

jwueller commented Jul 7, 2024 •

edited

Loading

bukowa commented Jul 9, 2024 •

edited

Loading

RedYetiDev commented Jul 9, 2024 •

edited

Loading

ovflowd commented Jul 10, 2024 •

edited

Loading

jwueller commented Jul 10, 2024 •

edited

Loading

jwueller commented Jul 13, 2024 •

edited

Loading