Don't munge window.location.hash to fix scroll positions #873

antifuchs · 2020-07-01T21:16:03Z

This fixes #764 and #510 by changing rustdoc pages' CSS layout to work together with the position:fixed nav bar such that keyboard focus and anchor-ID jumping work.
(Edited a great deal from when I first submitted this)

The technique used here relies on scroll-padding-top in the CSS, which is supported everywhere except Safari (neither iOS nor macOS). To fix Safari, we use a JS script to adjust the scroll offset on initial visits to the page and on internal jumping.

With this, the following things work (tested on Firefox, Chrome, mobile&desktop Safari):

Back/forward navigation only requires one single button press on non-firefox browsers
links to jump anchors show the whole item in the viewport automatically
input focus is set correctly on JS sessions, so that keyboard scrolling keeps working.

antifuchs · 2020-07-02T04:56:40Z

OK, so we had a good time on discord figuring out how to solve this for the position: relative main content container.

This weird javascript I'd touched earlier was necessary because of the fixed-position navbar; since this causes #595 and the solution in #510 looked much better, we decided to go with @Koxiaet's solution from #510 after all, and try fitting elements into the viewport despite the overlapping top navbar. The solution I pushed here uses javascript on page load (and on anchor jump links) to scroll just the right amount; unfortunately, no pure-CSS solution seemed to work:

The :target::before solution from this SO answer doesn't function correctly in rustdoc pages because the jump targets are display: flex & that messes up the alignment of elements in the [+] pub fn foo() [src] line.
Using scroll-padding-top on the body doesn't have any effect, even though it's advertised as being useful for exactly this use case... I don't understand why; plus, it doesn't work in IE at all /:

So I went with the solution from #510 (comment), adjusting it so that back/forward buttons don't jump to where a user might have left off.

This solution isn't ideal either:

Requires JS to be on, otherwise people see the crummy scroll positions
If people visit intra-page links by means other than clicking them or hitting "Enter" on them, they get crummy scroll positions.
Does not work in the "source code" view, as it's rustdoc's JS that scrolls to the first marked line... and that scrolls it out of the viewport. I don't think we can patch that.
Generally seems displeasing, being a JS-based solution.

OTOH, My back/forward button work quite well now, and the highlighted elements go in the right places (:

antifuchs · 2020-07-02T17:08:05Z

OK - pushed up yet another massive rework for different/maybe-better tradeoffs:

Use a flexbox-based navigation system (see here for an example and the technique)
Since the scrolling container is now a sub-element and not <body>, browsers doesn't retain scroll positions as well; to fix that, save scroll positions in per-tab sessionStorage and restore upon visiting the page again (via reload or back/forward buttons).
The JS also focuses the "main content" body, so scrolling with the keyboard keeps working.

I tested the following things, with JS turned on, in chrome/safari/firefox & for good measure, on an iOS device:

scrolling the body with a keyboard works.
back/forward buttons jump directly to the previous page and scroll position
reloading the page stays where you were previously
jumping to a target (on a rustdoc page and on the source code view) has the whole target in the viewport.

jyn514

I left a few superficial comments but I think @GuillaumeGomez should do a more thorough review - Guillaume, do you have the time?

In the meantime I'd love to get feedback from the people having issues in #595. I'm currently working on getting a staging server set up and I'll ping people once I have a link available.

templates/rustdoc.hbs

templates/style.scss

jyn514 · 2020-07-02T17:24:24Z

Also I love that this is mostly CSS and the JavaScript is only for the scroll position :D

GuillaumeGomez

A lot of small things, and I'm not really convinced by this solution but since we don't have something better... Thanks for the PR in any case!

GuillaumeGomez · 2020-07-02T18:16:30Z

templates/style.scss

-    padding-top: $top-navbar-height;
+    height: 100%;
+    overflow: hidden;
+    margin: 0px;


Suggested change

margin: 0px;

margin: 0;

templates/style.scss

templates/rustdoc.hbs

jyn514 · 2020-07-02T22:32:34Z

I tested on android chrome

Back button keeps the scroll position (if there's a forward button on mobile, I don't know about it)
Reloading the page keeps the scroll position
Clicking on a hash link to a function keeps full function in view and the sidebar above the function

jyn514 · 2020-07-03T20:46:16Z

I have a staging server with these changes running at http://50.116.47.176:3000/. @Koxiaet , @cormacrelf, @imjasonmiller, @Nemo157, @rudedogg could you check if this fixes the issues you were having with slow scrolling and keyboard shortcuts?

This exact PR won't be merged (it needs changes once we finally finish up #786) but it will end up looking very similar to what I have on the staging server.

imjasonmiller · 2020-07-03T21:12:08Z

@jyn514 it seems to be fixed for me on the main page, but not on the actual documentation pages themselves. The sidebar with Methods, Trait implementations and so on also doesn't appear to support smooth scrolling yet, but I think that might be fixed with -webkit-overflow-scrolling: touch on the scrolling container?

I could also send you screen recording, if that's okay?

jyn514 · 2020-07-03T21:30:19Z

A screen recording sounds great!

it seems to be fixed for me on the main page, but not on the actual documentation pages themselves

That's a shame ... by main page you mean the /crate URLs? Those are using a different stylesheet than the rustdoc pages which might be why.

I'm really confused now why this doesn't work, the header is no longer position: fixed at all ...

antifuchs · 2020-07-03T21:33:17Z

I'm really confused now why this doesn't work, the header is no longer position: fixed at all ...

I'm guessing the way that the flex box top navbar fixation works causes behavior similar to the way a position: fixed container would? I'm not sure why only the left bar would be affected for you, though.

@imjasonmiller - can you tell me what exact configuration you're running (what browser, OS version, device type)? I'd love a screen recording, too.

imjasonmiller · 2020-07-03T22:06:48Z

That's a shame ... by main page you mean the /crate URLs? Those are using a different stylesheet than the rustdoc pages which might be why.

@jyn514, I think this might be an error on my part. The actual documentation page (and sidebar) for a crate itself (e.g. http://50.116.47.176:3000/bat/0.15.4/bat/) seems to not support momentum scrolling, but the other pages seem to work fine. Are those pages using the separate style sheet you mentioned?

... can you tell me what exact configuration you're running (what browser, OS version, device type)? I'd love a screen recording, too.

@antifuchs, if the other tagged users aren't experiencing any issues, it might be considered legacy? It's an old iPhone 5S on iOS Safari and I've switched to Android quite some time ago.

It's currently past midnight, so I'll get to the screen recording this weekend, if you'd still like to?

antifuchs · 2020-07-03T23:51:55Z

@imjasonmiller so the 5S is running iOS 12, I suppose? My 13.5 iPhone 11 does do momentum scrolling - I wonder if that's why we see different things. Either way, if you could get a screen recording, that'd be marvelous! (No rush, this PR won't be "ripe" until the template refactor is done) (:

Kestrer · 2020-07-04T12:47:29Z

It still doesn't work for me, but it's probably more the the fault of my browser which is unable to scroll anything other than the body itself.

antifuchs · 2020-07-04T14:05:21Z

Well - I don't think solutions that please everyone exist.

Let me eat my words, 30 minutes after posting this :D :D :D

Turns out there might be a solution that pleases everyone!! scroll-padding-top is a CSS attribute you can attach to the html (and in chrome, must also attach to the body), which tells the browser where to move the viewport.

I'd been trying this before and discarded it because I hadn't attached the attribute to the right elements (been trying it with the headers that get jumped to). This article finally set me straight.

The solution works (as in keyboard scrolling, jump-target positioning, back/forward DTRT) in Chrome and Firefox, requires zero javascript and no page structure modification. I think we have a winner.

Will commit what I have and push it up asap.

GuillaumeGomez · 2020-07-04T14:09:25Z

Nice finding! If this is the miracle solution we've been looking for so long, I'll be really happy to finally close this chapter! :D

antifuchs · 2020-07-04T14:26:58Z

Welp. Testing this in some more browsers, it looks like mobile and desktop Safari does not support the attribute for anchor elements: https://bugs.webkit.org/show_bug.cgi?id=179379

I guess we'll have to put a little bit of javascript back to fix safari positioning support, but it's better-enough of a solution that it'd probably be worth it.

antifuchs · 2020-07-04T15:22:52Z

I rebased away the (confusing) flex layout changes) & pushed the scroll-padding-top based solution up now. The JS does look a little intimidating, but I hope it's not too bad. The only place where it really has any effect is in Safari.

templates/rustdoc.hbs

antifuchs · 2020-07-04T18:21:48Z

Thanks for the comments, @Koxiaet - that definitely makes the code easier to read; I've converted all the assignments to const (same scoping rules as let, signals they're convenience value holders) and shortened the expression as you suggested.

jyn514

I redeployed the changes to http://50.116.47.176:3000/. This is really exciting!

templates/rustdoc.hbs

Kestrer · 2020-07-05T05:52:13Z

I redeployed the changes to http://50.116.47.176:3000/. This is really exciting!

I don't think you have. I checked it and it's still the old code, and my cache is off.

jyn514 · 2020-07-05T11:50:54Z

Oops, fixed. Good catch!

Kestrer · 2020-07-05T12:54:22Z

At least for me, it works perfectly!

antifuchs · 2020-07-06T16:41:00Z

I asked my colleagues to try the staging server on Android devices, and they confirmed that it seems to work there in both Chrome and Firefox \o/

jyn514 · 2020-07-06T17:48:48Z

Ok great! Then I think this is only waiting on #879, then a rebase and we'll be good to go :) I'll see if I can review Tera this afternoon.

Kixiron · 2020-07-06T19:04:07Z

I’ll have time later today/tonight to work on it, if you’re extra-free it’d help tons to investigate the "funky JS" issues on the templates I made but if not then no sweat

This (fairly new) CSS attribute gets browsers that support it perfectly set up to skip the top nav bar. Unfortunately, Safari (both macOS and iOS) don't support it for this purpose yet, so we work around them in JS.

antifuchs · 2020-07-13T22:02:59Z

OK, I rebased this onto master & ported the scroll-position fixing script to rustdoc/page.html. This should be the only place it's really needed for Safaris, but lmk and we can move it to another page template too.

I tried the change on Safari (mobile and desktop), and it works as it did on the old master branch.

antifuchs force-pushed the url-hash-modification branch from 26450ce to 37d1a56 Compare July 1, 2020 21:19

This comment has been minimized.

Sign in to view

antifuchs force-pushed the url-hash-modification branch from 37d1a56 to 9e4aefd Compare July 1, 2020 22:19

This comment has been minimized.

Sign in to view

antifuchs force-pushed the url-hash-modification branch from 9e4aefd to 9744d58 Compare July 1, 2020 22:34

This comment has been minimized.

Sign in to view

antifuchs force-pushed the url-hash-modification branch from 5b9bfc7 to b71d417 Compare July 2, 2020 04:46

jyn514 reviewed Jul 2, 2020

View reviewed changes

templates/rustdoc.hbs Outdated Show resolved Hide resolved

templates/rustdoc.hbs Outdated Show resolved Hide resolved

templates/style.scss Outdated Show resolved Hide resolved

GuillaumeGomez requested changes Jul 2, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

jyn514 added P-medium Medium priority S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jul 3, 2020

antifuchs changed the title ~~Don't munge window.location.hash~~ Don't munge window.location.hash; use flex layout for rustdoc pages Jul 3, 2020

antifuchs force-pushed the url-hash-modification branch from 4b2d24b to 104bad9 Compare July 3, 2020 19:08

jyn514 added A-frontend Area: Web frontend S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jul 3, 2020

antifuchs force-pushed the url-hash-modification branch from 104bad9 to fbad575 Compare July 4, 2020 15:19

antifuchs changed the title ~~Don't munge window.location.hash; use flex layout for rustdoc pages~~ Don't munge window.location.hash to fix scroll positions Jul 4, 2020

Kestrer reviewed Jul 4, 2020

View reviewed changes

templates/rustdoc.hbs Outdated Show resolved Hide resolved

Kestrer reviewed Jul 4, 2020

View reviewed changes

templates/rustdoc.hbs Outdated Show resolved Hide resolved

jyn514 reviewed Jul 4, 2020

View reviewed changes

templates/rustdoc.hbs Outdated Show resolved Hide resolved

templates/rustdoc.hbs Outdated Show resolved Hide resolved

jyn514 mentioned this pull request Jul 12, 2020

Migrate to Tera #887

Merged

jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 13, 2020

Kestrer and others added 2 commits July 13, 2020 17:53

removed fixed

78d62bb

Use scroll-padding-top to position anchors correctly in the viewport

d2080e8

This (fairly new) CSS attribute gets browsers that support it perfectly set up to skip the top nav bar. Unfortunately, Safari (both macOS and iOS) don't support it for this purpose yet, so we work around them in JS.

antifuchs force-pushed the url-hash-modification branch from cccdc7f to d2080e8 Compare July 13, 2020 22:00

jyn514 added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jul 13, 2020

jyn514 merged commit 731d031 into rust-lang:master Jul 13, 2020

jyn514 mentioned this pull request Jul 13, 2020

iPhone scrolling is painful #595

Closed

jyn514 mentioned this pull request Mar 26, 2021

maybeFixupViewportPosition: Uncaught TypeError: document.getElementsByClassName(...)[0] is undefined #1334

Closed

This was referenced Jan 16, 2022

Switch to position: sticky and rely on scroll-padding-top #1595

Open

Remove JS workaround for scroll-padding-top #1596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't munge window.location.hash to fix scroll positions #873

Don't munge window.location.hash to fix scroll positions #873

antifuchs commented Jul 1, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antifuchs commented Jul 2, 2020 •

edited

Loading

antifuchs commented Jul 2, 2020

jyn514 left a comment •

edited

Loading

jyn514 commented Jul 2, 2020

GuillaumeGomez left a comment •

edited

Loading

GuillaumeGomez Jul 2, 2020

jyn514 commented Jul 2, 2020 •

edited

Loading

This comment has been minimized.

jyn514 commented Jul 3, 2020

imjasonmiller commented Jul 3, 2020

jyn514 commented Jul 3, 2020

antifuchs commented Jul 3, 2020

imjasonmiller commented Jul 3, 2020 •

edited

Loading

antifuchs commented Jul 3, 2020

Kestrer commented Jul 4, 2020

antifuchs commented Jul 4, 2020

GuillaumeGomez commented Jul 4, 2020

antifuchs commented Jul 4, 2020

antifuchs commented Jul 4, 2020

antifuchs commented Jul 4, 2020

jyn514 left a comment

Kestrer commented Jul 5, 2020

jyn514 commented Jul 5, 2020

Kestrer commented Jul 5, 2020

antifuchs commented Jul 6, 2020

jyn514 commented Jul 6, 2020

Kixiron commented Jul 6, 2020

antifuchs commented Jul 13, 2020

Don't munge window.location.hash to fix scroll positions #873

Don't munge window.location.hash to fix scroll positions #873

Conversation

antifuchs commented Jul 1, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antifuchs commented Jul 2, 2020 • edited Loading

antifuchs commented Jul 2, 2020

jyn514 left a comment • edited Loading

Choose a reason for hiding this comment

jyn514 commented Jul 2, 2020

GuillaumeGomez left a comment • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez Jul 2, 2020

Choose a reason for hiding this comment

jyn514 commented Jul 2, 2020 • edited Loading

This comment has been minimized.

jyn514 commented Jul 3, 2020

imjasonmiller commented Jul 3, 2020

jyn514 commented Jul 3, 2020

antifuchs commented Jul 3, 2020

imjasonmiller commented Jul 3, 2020 • edited Loading

antifuchs commented Jul 3, 2020

Kestrer commented Jul 4, 2020

antifuchs commented Jul 4, 2020

GuillaumeGomez commented Jul 4, 2020

antifuchs commented Jul 4, 2020

antifuchs commented Jul 4, 2020

antifuchs commented Jul 4, 2020

jyn514 left a comment

Choose a reason for hiding this comment

Kestrer commented Jul 5, 2020

jyn514 commented Jul 5, 2020

Kestrer commented Jul 5, 2020

antifuchs commented Jul 6, 2020

jyn514 commented Jul 6, 2020

Kixiron commented Jul 6, 2020

antifuchs commented Jul 13, 2020

antifuchs commented Jul 1, 2020 •

edited

Loading

antifuchs commented Jul 2, 2020 •

edited

Loading

jyn514 left a comment •

edited

Loading

GuillaumeGomez left a comment •

edited

Loading

jyn514 commented Jul 2, 2020 •

edited

Loading

imjasonmiller commented Jul 3, 2020 •

edited

Loading