rustdoc: improve scroll locking in the rustdoc mobile sidebars #98775
Conversation
|
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @Folyd, @jsha |
rustbot
added
the
T-rustdoc
|
r? @jsha (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Since the code is almost to lock the scroll when the sidebar is open, we should maybe merge both codes. What do you think? So for this, I'm a bit shared: it makes it somewhat coherent with the source sidebar but I don't think it's needed in this case. So better ask others what they think. |
|
So after reading notriddle's comment, I think it's actually a good idea. Still double-checking there is no unforeseen UX issue but otherwise I agree with it. |
|
I'm kinda +1 on this, but I defer to jsha |
|
☔ The latest upstream changes (presumably #98925) made this pull request unmergeable. Please resolve the merge conflicts. |
notriddle
force-pushed
the
notriddle/mobile-sidebar-scroll-lock
branch
from
9621c35
to
5e91804
Compare
This comment has been minimized.
This comment has been minimized.
This commit ports the scroll locking behavior from the source sidebar to the regular sidebar, and also fixes some bad behavior where opening a "mobile" sidebar, and growing the viewport so that the "desktop" mode without scroll locking is activated, could potentially leave the page stuck. This does not affect the behavior on larger screens. Only small ones, where the sidebar covers up the main content.
notriddle
force-pushed
the
notriddle/mobile-sidebar-scroll-lock
branch
from
5e91804
to
6b60bc6
Compare
|
I'm +1 on the idea - scrolling the sidebar should never scroll the main content, and it shouldn't be possible to get "stuck" in a situation where you can't scroll the main content. I'm having a little trouble understanding the PR description:
Can you link to the PR that modified the scroll locking behavior from the source sidebar? I went looking and couldn't find it. Alternately/additionally, it would be good to explain the scroll locking behavior in this PR's description. I think this problem is not unique to the mobile sidebars. On Chrome latest (Linux), with a desktop-sized window, when I scroll to the bottom of a sidebar (docs or source) and keep scrolling down, the main content starts scrolling down. If I then start scrolling up again, the main content scrolls up; but I expected the sidebar to scroll up. Only after wiggling my mouse can I get the sidebar to scroll up again. Does that match the behavior you're working on fixing for the mobile sidebars? |
You're right. It's done.
That's the same bug, yes. The trouble is that the desktop sidebar and content area are both usable at the same time, making the solution space different. When using the lock-scroll trick from these pull requests, the scroll bar goes away. On desktop platforms with scrollbars that affect the size of the viewport, this causes the layout to shift. This is acceptable when you're explicitly clicking an Open Sidebar button, since the layout is changing anyway, but when the sidebar and the main content area are both supposed to be usable at once, there's no good opportunity to squeeze in a layout shift like this. There are alternative solutions that might work better, but they're different solutions than the one we're using on the mobile layout, so that can probably be a separate PR. Like intercepting mouse wheel events, or canceling scroll events based on where the mouse is, or any of the other ways we might go about it. |
|
The scroll-lock trick uses a lot of JavaScript and seems complex. Also, I'm nervous about trying to avoid layout shifts by guessing the width of the scrollbar, which can vary based on OS. I tried another approach, which rearranges which elements have a scroll bar. I think it works pretty well: https://github.com/rust-lang/rust/compare/master...jsha:rust:scroll-fix?expand=1
https://rustdoc.crud.net/jsha/scroll-fix/std/string/struct.String.html To make this work right, I had to make keyboard focus begins in the main content area rather than the sidebar, which seems more useful to me. This also affects how the mobile topbar works, in a positive way. Instead of having it overlap the body and sidebar (in the z axis) with a corresponding offset in both, it is above the body and sidebar in the y axis. If we go this route we will probably need some slight tweaks in the docs.rs CSS to accomodate the topbar over there. |
Won't this cause rust-lang/docs.rs#595 to come back? |
Oh, interesting. I wasn't aware of that issue. Seems like yes, which would be a blocker. |
|
r? @jsha Are there any further blockers on this change? |
|
@bors r+ rollup Thanks for driving this forward! |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 14 pull requests Successful merges: - rust-lang#98775 (rustdoc: improve scroll locking in the rustdoc mobile sidebars) - rust-lang#99479 (rustdoc-json: Remove doc FIXME for Import::id and explain) - rust-lang#100040 (Error on broken pipe but do not backtrace or ICE) - rust-lang#100072 (linker-plugin-lto.md: Correct the name of example c file) - rust-lang#100098 (Some "this expression has a field"-related fixes) - rust-lang#100226 (Do not manually craft a span pointing inside a multibyte character.) - rust-lang#100240 (Fail gracefully when const pattern is not structural match.) - rust-lang#100256 (Add some high-level docs to `FnCtxt` and `ItemCtxt`) - rust-lang#100261 (Set tainted errors bit before emitting coerce suggestions.) - rust-lang#100275 (also update anyhow in codegen_cranelift) - rust-lang#100281 (Remove more Clean trait implementations) - rust-lang#100314 (Mention `unit-test` in MIR opt test README) - rust-lang#100319 (Remove more Clean trait implementations) - rust-lang#100323 ([rustdoc] Don't render impl blocks with doc comment if they only contain private items by default) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…ll, r=GuillaumeGomez rustdoc: factor JS mobile scroll lock into its own function rust-lang#98775 (comment)
Fixes the desktop scrolling weirdness mentioned in rust-lang#98775 (comment) As described in the MDN page for this property: * The current Firefox ESR is 102, and the first Firefox version to support this feature is 59. * The current Chrome version 112, and the first version to support this is 63. * Edge is described as having a minor bug in `none` mode, but we use `contain` mode anyway, so it doesn't matter. * Safari 16, released September 2022, is the last browser to add this feature, and is also the oldest version we officially support.
…avior, r=GuillaumeGomez rustdoc: use CSS `overscroll-behavior` instead of JavaScript Fixes the desktop scrolling weirdness mentioned in rust-lang#98775 (comment) Preview: https://notriddle.com/rustdoc-demo-html-3/overscroll-behavior/issue_107918/index.html As described in the [MDN overscroll-behavior] page: * The current Firefox ESR is 102, and the first Firefox version to support this feature is 59. * The current Chrome version 112, and the first version to support this is 63. * Edge is described as having a minor bug in `none` mode, but we use `contain` mode anyway, so it doesn't matter. * Safari 16, released September 2022, is the last browser to add this feature, and is also the oldest version we officially support. [MDN overscroll-behavior]: https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior
Fixes the desktop scrolling weirdness mentioned in rust-lang#98775 (comment) As described in the MDN page for this property: * The current Firefox ESR is 102, and the first Firefox version to support this feature is 59. * The current Chrome version 112, and the first version to support this is 63. * Edge is described as having a minor bug in `none` mode, but we use `contain` mode anyway, so it doesn't matter. * Safari 16, released September 2022, is the last browser to add this feature, and is also the oldest version we officially support.
…avior, r=GuillaumeGomez rustdoc: use CSS `overscroll-behavior` instead of JavaScript Fixes the desktop scrolling weirdness mentioned in rust-lang#98775 (comment) Preview: https://notriddle.com/rustdoc-demo-html-3/overscroll-behavior/issue_107918/index.html As described in the [MDN overscroll-behavior] page: * The current Firefox ESR is 102, and the first Firefox version to support this feature is 59. * The current Chrome version 112, and the first version to support this is 63. * Edge is described as having a minor bug in `none` mode, but we use `contain` mode anyway, so it doesn't matter. * Safari 16, released September 2022, is the last browser to add this feature, and is also the oldest version we officially support. [MDN overscroll-behavior]: https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior
This PR prevents the main content area from scrolling while the mobile sidebar is open on documentation pages (porting the scroll locking behavior from the source sidebar to the regular sidebar), and also fixes some bad behavior where opening a "mobile" sidebar, and growing the viewport so that the "desktop" mode without scroll locking is activated, could potentially leave the page stuck.
This does not affect the behavior on larger screens. Only small ones, where the sidebar covers up the main content.
Split out from #98772