Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove content-visibility hack from the docs #41869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CyberAP
Copy link

@CyberAP CyberAP commented Feb 6, 2022

Current implementation of content-visibility hack to achieve better performance completely breaks scrolling on the site. This PR suggests removing this hack until a better solution is found.

Fixes #40099

Screen.Recording.2022-02-06.at.04.30.24.mov

Current implementation of `content-visibility` hack to achieve better performance completely breaks scrolling on the site. This PR suggests removing this hack until a better solution is found.
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 6, 2022
@Trott
Copy link
Member

Trott commented Feb 6, 2022

@nodejs/website

@tniessen
Copy link
Member

tniessen commented Feb 6, 2022

Potential issues were mentioned in the original PR about a year ago: #37301 (comment) (cc @aduh95)

@Trott
Copy link
Member

Trott commented Feb 6, 2022

Just in case anyone wants confirmation, I can confirm that this fixes the scrollbar issue.

@aduh95
Copy link
Contributor

aduh95 commented Feb 6, 2022

For a little more context, this is a Blink-specific issue: they are the only one who have implemented content-visibility, and their performance at rendering large HTML documents is not great (compared to say Firefox).

Personally, I never interact with the scrollbar, I have no use for it, so I would take better performance over scrollbar funkiness any day.
With this PR being open, I understand that my use case is not universal and there is also of subset of users who find usefulness in the scrollbar, which is why this PR and #40215 have been opened.

We can't please both types of users, whatever we decide to go with, it will be the equivalent of telling one group or the other "sorry you can't use our docs with your current browser, please Firefox instead (or fight with with Google and Microsoft so they fix the underlining issue in Blink)".

Removing this hack makes rendering the docs with a Chromium-based browser much slower, whether or not you are using the scrollbar, so I'm not convinced it would be beneficial overall.

@silverwind
Copy link
Contributor

silverwind commented Feb 6, 2022

I generally agree that the hack needs to be removed if it interferes with scrolling like shown.

I do wonder if there are better solutions. I may be wrong, but I think PR diffs on GitHub use some kind of lazy-loading that does not interfere with scrolling while still rendering pretty fast. Maybe it's some JS-based approach they are using. Another benefit of JS-based would be that it will work in all browsers.

@zbjornson
Copy link
Contributor

+1 for landing this and backporting to previous versions. Firefox is progressing toward shipping CSS containment (enabled in Nightly since March 2nd), and in that browser the docs page TOC links jump to random locations with these two CSS rules, making the API docs infuriating to use.

I've verified that removing these rules fixes both the scrollbar jittering (#40099) and the TOC links (#47106).

@vweevers
Copy link
Contributor

The fact that this fixes the TOC links should be the focus (and motivation for landing it) because IMO that's much more frustrating than the scrollbar jittering, enough so that it trumps the performance concern.

@ovflowd
Copy link
Member

ovflowd commented May 4, 2023

I believe entirely this should land ASAP, and we can create a backport compatibility plan afterwards. The more we take time for landing this the more we postpone on improving the user experience.

@ovflowd
Copy link
Member

ovflowd commented May 4, 2023

Note that this cannot land as it is because the width of the page is not respected and gets broken, so a few tweaks need to be done, such as CSS grids. I can amend changes to this branch as also the initial commit does not adhere to guidelines.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to land, but, removing content-visibility introduces a bug on content-shift and the content not respecting the page's max-width.

So we need to amend some changes before this can get merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: weird scrolling/anchor behavior in Chrome and Firefox
9 participants