Add auto to docs' contain-intrinsic-size decl, to prevent jitter/flicker #48195
Conversation
|
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 |
aduh95
added
the
author ready
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
added
the
commit-queue
aduh95
added
fast-track
|
Fast-track has been requested by @aduh95. Please 👍 to approve. |
|
Thanks for the quick feedback/approval, folks!
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! ✨ |
@ovflowd Open a PR to update CODEOWNERS. Our GitHub bot will automatically ping the teams based on the files changed in the pull request. |
Sure. Looks like I was missing the |
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. |
aduh95
added
the
commit-queue
|
I did test it on a Chromium browser (it's better for sure), on Firefox (no changes with |
nodejs-github-bot
removed
the
commit-queue
|
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 ): (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 |
|
@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 |
|
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. node/doc/contributing/collaborator-guide.md Lines 805 to 807 in 260092e node/doc/contributing/collaborator-guide.md Lines 813 to 816 in 260092e |
|
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 :) |
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>
|
Hi folks! Just a minor update, following up on this part from my initial comment here:
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 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 |
|
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: |
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>
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>
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>
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
added
backported-to-v18.x
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:autoandcontain-intrinsic-size:1px 5000pxstyles 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
autoto thecontain-intrinsic-sizeCSS, 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
autowere 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.)