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

Add auto to docs' contain-intrinsic-size decl, to prevent jitter/flicker #48195

Merged
merged 1 commit into from
May 27, 2023
Merged

Conversation

dholbert
Copy link
Contributor

This addresses a scrolling/flickering issue in the node.js docs, as viewed by Firefox versions that have 'content-visibility' support (on by default in Firefox Nightly for several releases, opt-in in about:config in release versions).

See https://bugzilla.mozilla.org/show_bug.cgi?id=1832675 for details. Before this patch, the content-visibility:auto and contain-intrinsic-size:1px 5000px styles are telling the browser to suppress rendering of offscreen sections and behave as if they're 5000px tall, though they in fact are much shorter. This causes scrolling to behave quite bizarrely and can cause troublesome oscillating issues.

By adding auto to the contain-intrinsic-size CSS, we're asking the browser to remember the last-rendered size for the element (once it's been rendered) instead of forcing the browser to treat it as being 1px by 5000px when it goes offscreen.

(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.)

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 26, 2023
@dholbert
Copy link
Contributor Author

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

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 26, 2023
This commit addresses a scrolling/flickering issue in the HTML version
of the docs. By adding `auto` to the `contain-intrinsic-size` CSS
property, we're asking the browser to remember the last-rendered size
for the element (once it's been rendered) instead of forcing the browser
to treat it as being 1px by 5000px when it goes offscreen.
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2023
@aduh95 aduh95 added fast-track PRs that do not need to wait for 48 hours to land. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 26, 2023
@github-actions
Copy link
Contributor

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

@zbjornson
Copy link
Contributor

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.

@dholbert
Copy link
Contributor Author

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.

@ovflowd
Copy link
Member

ovflowd commented May 26, 2023

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.

@ovflowd
Copy link
Member

ovflowd commented May 26, 2023

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

@ovflowd
Copy link
Member

ovflowd commented May 26, 2023

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

@richardlau
Copy link
Member

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

@dholbert
Copy link
Contributor Author

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

@aduh95
Copy link
Contributor

aduh95 commented May 27, 2023

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.

@ovflowd
Copy link
Member

ovflowd commented May 27, 2023

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.

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 27, 2023
@aduh95
Copy link
Contributor

aduh95 commented May 27, 2023

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.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit 06136be into nodejs:main May 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 06136be

@dholbert dholbert deleted the patch-1 branch May 27, 2023 16:27
@dholbert
Copy link
Contributor Author

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

@ovflowd
Copy link
Member

ovflowd commented May 29, 2023

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.

@aduh95
Copy link
Contributor

aduh95 commented May 30, 2023

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

@aduh95 aduh95 added the lts-watch-v18.x PRs that may need to be released in v18.x. label May 30, 2023
@ovflowd
Copy link
Member

ovflowd commented May 30, 2023

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?

@aduh95
Copy link
Contributor

aduh95 commented May 30, 2023

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.

* `lts-watch-` labels are for pull requests to consider for landing in staging
branches. For example, `lts-watch-v10.x` would be for a change
to consider for the `v10.x-staging` branch.

Any collaborator can attach these labels to any pull request/issue. As commits
land on the staging branches, the backporter removes the `lts-watch-` label.
Likewise, as commits land in an LTS release, the releaser removes the `land-on-`
label.

@ovflowd
Copy link
Member

ovflowd commented May 30, 2023

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

targos pushed a commit that referenced this pull request May 30, 2023
This commit addresses a scrolling/flickering issue in the HTML version
of the docs. By adding `auto` to the `contain-intrinsic-size` CSS
property, we're asking the browser to remember the last-rendered size
for the element (once it's been rendered) instead of forcing the browser
to treat it as being 1px by 5000px when it goes offscreen.

PR-URL: #48195
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos mentioned this pull request Jun 4, 2023
@dholbert
Copy link
Contributor Author

dholbert commented Jun 6, 2023

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.

@ovflowd
Copy link
Member

ovflowd commented Jun 6, 2023

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

@dholbert
Copy link
Contributor Author

dholbert commented Jun 6, 2023

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.

danielleadams pushed a commit that referenced this pull request Jul 6, 2023
This commit addresses a scrolling/flickering issue in the HTML version
of the docs. By adding `auto` to the `contain-intrinsic-size` CSS
property, we're asking the browser to remember the last-rendered size
for the element (once it's been rendered) instead of forcing the browser
to treat it as being 1px by 5000px when it goes offscreen.

PR-URL: #48195
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
This commit addresses a scrolling/flickering issue in the HTML version
of the docs. By adding `auto` to the `contain-intrinsic-size` CSS
property, we're asking the browser to remember the last-rendered size
for the element (once it's been rendered) instead of forcing the browser
to treat it as being 1px by 5000px when it goes offscreen.

PR-URL: nodejs#48195
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This commit addresses a scrolling/flickering issue in the HTML version
of the docs. By adding `auto` to the `contain-intrinsic-size` CSS
property, we're asking the browser to remember the last-rendered size
for the element (once it's been rendered) instead of forcing the browser
to treat it as being 1px by 5000px when it goes offscreen.

PR-URL: nodejs#48195
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This commit addresses a scrolling/flickering issue in the HTML version
of the docs. By adding `auto` to the `contain-intrinsic-size` CSS
property, we're asking the browser to remember the last-rendered size
for the element (once it's been rendered) instead of forcing the browser
to treat it as being 1px by 5000px when it goes offscreen.

PR-URL: nodejs#48195
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed lts-watch-v18.x PRs that may need to be released in v18.x. labels Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants