GeezerVison

It appears this was also discussed in #40215 , but that PR hasn't yet been merged and included a bunch of other changes.

It was also discussed in #40099

It seems there's potentially some reworking or removal that might eventually be done in one or the other of those PRs.

But in the meantime, this one-liner auto addition would be a surgical incremental-improvement which makes the page usable in Firefox Nightly, and also improves the experience in Chrome. (In Chrome, it makes it so scrolling gets less jumpy once you've scrolled over a particular section, or the whole page, and given the browser a chance to compute the actual height of the various section elements). And this has no effect in Safari, since they don't support content-visibility yet based on https://caniuse.com/css-content-visibility

Fast-track has been requested by @aduh95. Please 👍 to approve.

Tested; this is definitely a partial improvement, but IMO #41869 is a better interim fix. @ovflowd said that PR causes some page width issues, but I don't see them.

Thanks for the quick feedback/approval, folks!

Tested; this is definitely a partial improvement, but IMO #41869 is a better interim fix. @ovflowd said that PR causes some page width issues, but I don't see them.

To be clear, this doesn't have to be a situation of "which interim fix is better".

This patch here is just a minor improvement to the approach that the site is already using. It's entirely possible that #41869 will make things better-still, and that's great; but until that patch is ready, this is a trivial step in the direction of usability.

The issues vary by browser and page. I haven't tested this PR yet, I understand that fast-track got requested, but since we do not have interaction testing for docs I ask to collaborators to not merge this before someone in particular is able to check if this doesn't break common docs pages in different browsers.

Thank you for pinging me by the way.

@nodejs/tsc can we make doc tagged PRs to mention the website team? Thank you!

Also @dholbert could you fix your commit message to adhere to the commit guidelines? Ty! ✨

@nodejs/tsc can we make doc tagged PRs to mention the website team? Thank you!

@ovflowd Open a PR to update CODEOWNERS. Our GitHub bot will automatically ping the teams based on the files changed in the pull request.

Also @dholbert could you fix your commit message to adhere to the commit guidelines? Ty!

Sure. Looks like I was missing the doc: subsystem prefix, based on https://github.com/nodejs/node/actions/runs/5093534334/jobs/9156659856 . Fixed in 23bfc09 , I think (hopefully/maybe the linter will re-run and let me know if I missed something else.)

I ask to collaborators to not merge this before someone in particular is able to check if this doesn't break common docs pages in different browsers.

I don't understand what you mean, there are 2 approvals plus one external contributor (plus the PR author) who have validated the change, what do you mean by "someone in particular"? AFAICT this change is an improvement, I don't see a reason to hold off.

I would say that on this case, testing this change needs to be done manually. If you have tested it in different docs pages and browsers and the other approvals also did the same, then sure, feel free to ship it.

I'm not trying to hold this change, I'm just saying to hold it until somebody actually writes down that this change is working as expected.

Since there are no visual tests and interaction tests in place for the docs.

I did test it on a Chromium browser (it's better for sure), on Firefox (no changes with content-visibility disabled in about:config as expected, and slightly better with it enabled although still feels quite broken), no changes on Safari as expected.

Landed in 06136be

Thanks, everyone!

I have two questions, to help with validating things on the Mozilla bug-report side (at https://bugzilla.mozilla.org/show_bug.cgi?id=1832675 ):
(1) do you know how soon the change would be reflected on the public website (I imagine in the stylesheet at https://nodejs.org/docs/latest/api/assets/style.css )?

(2) will this change automagically make it into the version-specific snapshots of the docs (e.g. the stylesheet at https://nodejs.org/docs/latest-v16.x/api/assets/style.css )? or does that need separate changes?

(We actually received the bug report on the Mozilla side for a version-specific link, so ideally I'm hoping those can benefit from this fix as well.)

Hey @dholbert, sorry for being late to come back to you!

(1) It will be released once a new release of Node.js is published. Right now, docs changes are only reflected together with Node.js releases.

(2) Yes, it only goes to snapshots. /latest/ is a symlink to the latest built version of the docs.

cc @nodejs/releasers, we might want to backport the api/assets/style.css, which is located on doc/api_assets/style.css, to the Active LTS, Current, and Maintenance LTS versions (20, 18, 16), respectively I guess.

@ovflowd please consider not pinging team unless necessary, if folks start pinging team for each PR that can be backported (which is all the PRs, except the semver-major ones and the ones with dont-land-on-* labels), it's going to be overwhelming for the release team. In this case, you can add the lts-watch-* labels yourself, you don't need to ping a team for permission. (FYI it's @nodejs/lts whose in charge of choosing which commits land on LTS lines, not @nodejs/releasers according to https://github.com/nodejs/Release#mandate).

Thank you, @aduh95, I'm sadly unaware of the intrinsics of releasing Node.js (yet). Is there any other documentation out there for these labels? (I also pinged the team as I do no know whom to ask this question).

FYI is adding these labels enough to get them backported or what?

Please consider reading the collaborator guide which should have answers to all your questions, if it doesn't, let's open a PR to clarify where it's needed.

That's useful; I appreciate that! I was aware of the collaborator guide but wanted to know if there any other docs you'd suggest I should give read. I am going to keep an eye on the collaborator guide and the releases README. I apologise for any unnecessary ping caused here :)

Hi folks! Just a minor update, following up on this part from my initial comment here:

(In fact the spec was recently updated to require browsers to behave as if auto were specified like this; but this fix hasn't shipped in Firefox yet as of the time I'm writing this patch, so in the meantime it would be nice to avoid the issue directly in the Node.js styles.)

Good news - as of today's Firefox Nightly (v116, datestamp 2023-06-06), we've fixed that^ and now handle this automatically, i.e. today's Firefox Nightly treats nodejs.org's content-visibility:auto; contain-intrinsic-size:1px 5000px; styles as if the 1px 5000px value were instead 1px auto 5000px. The fix there was https://hg.mozilla.org/mozilla-central/rev/af2192d1c537

So that mitigates this for Firefox users & makes the nodeJS docs commit here irrelevant for Firefox Nightly users. Having said that, this commit here is still worth merging around, for reduced-magic & for the benefit of Chrome users (who don't yet have this spec-change implemented -- their bug on the spec change is https://bugs.chromium.org/p/chromium/issues/detail?id=1418453 ). Chrome users do still get a better experience with an explicit auto value, based on my observations in my comment above at least.

Hey @dholbert this already got merged and will be shipped with next Node.js version and then backported :)

Thanks! That's what I thought, yeah. My update was just meant to:
(a) add context & prevent-confusion for folks who might re-test this issue in Firefox Nightly (or other versions with the Firefox patch) when validating this doc-stylesheet-fix when it ships or is backported, who might be wondering why they're not seeing any behavioral difference from this fix.
(b) reassure folks that, despite (a), this isn't/wasn't pointless, since this is still a quality-of-life improvement for Chrome users.