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

rustdoc: sidebar usability improvements #98772

Closed
wants to merge 5 commits into from

Conversation

notriddle
Copy link
Contributor

  • Fix a bug related to scroll locking the main page and resizing the browser window.
  • Improve the mobile behavior of the source sidebar. Technically, this makes the mobile sidebar less consistent with desktop, but it seems better to me.
  • Use the <details> tag for the source sidebar directory tree.
  • Make the source sidebar toggle a real button with a tab index.

https://notriddle.com/notriddle-rustdoc-test/sidebar-fixes/std/

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 1, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2022

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

A change occurred in the Ayu theme.

cc @Cldfire

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@rust-highfive
Copy link
Collaborator

r? @jsha

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2022
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.
On desktop, if you open the source code sidebar, it stays open even when you
move from page to page. It used to do the same thing on mobile, but I think
that's stupid. Since the file list fills the entire screen on mobile, and you
can't really do anything with the currently selected file other than dismiss
the "sidebar" to look at it, it's safe to assume that anybody who clicks a
file in that list probably wants the list to go away so they can see it.
This fixes the extremely poor accessibility of the old system, making it
possible to navigate the sidebar by keyboard, and also implicitly gives the
sidebar items the correct ARIA roles.
This fixes tab focus, so that you can open and close the sidebar
from keyboard.
bottom: 0;
left: 0;
right: 0;
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to set the width here? Also why all these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The essential problem here is that the sidebar toggle element for the source code view is a <div>, rather than a <button>. That's bad, because it won't get reported correctly by accessibility tools, and will also not have a tab position, so you can't get at it with a keyboard.

Fixing that involves making it a <button>, but keeping the appearance the same means having to reset all the default button styles.

@GuillaumeGomez
Copy link
Member

You made a lot of different UI and behaviour changes into one PR, so it's a bit tricky in here. Could you split fixes and improvements please?

Otherwise I took a quite look and noted a few things:

  • There is no more left margin on the source sidebar items.
  • Change of behaviour for the sidebar in the doc pages: it was on purpose that the scroll wasn't locked.
  • The switch to <details>/<summary> for the source sidebar entries is a great idea. One inconvenient though: when you want to copy the text, it triggers an event for the <details>.

@notriddle
Copy link
Contributor Author

@GuillaumeGomez

There is no more left margin on the source sidebar items.

image

On the left is the "before." On the right is the "after."

Could you create a screenshot illustrating the problem? Because, to me, these look indistinguishable.

@notriddle
Copy link
Contributor Author

Change of behaviour for the sidebar in the doc pages: it was on purpose that the scroll wasn't locked.

Do we want that on mobile? The desktop behavior shouldn't be changed by this pull request, but on mobile, the sidebar acts as a modal drawer, which normally prevents the content that it partially covers up from being interacted with.

It's especially annoying because, if I reach the top or bottom of the drawer and try to scroll, the view underneath will scroll instead rather than generating the "bounce" indication that I've reached the end.

@notriddle
Copy link
Contributor Author

notriddle commented Jul 1, 2022

Okay, I split it up into four separate pull requests, one for each feature.

@notriddle notriddle closed this Jul 1, 2022
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 5b9775fe17893cba641a071de7e0a7c8f478c41b and afc8e01cbeb5278f6d41fc4314d0709d318b7735
Rustdoc was updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
.......... (60/66)
.....     (66/66)


/checkout/src/test/rustdoc-gui/sidebar-source-code-display.goml sidebar-source-code-display... FAILED
[ERROR] (line 39) Error: Evaluation failed: The following errors happened (for selector `#source-sidebar details[open] > .files a.selected`): [expected `rgb(255, 255, 255)` for key `background-color`, found `rgb(224, 224, 224)`]: for command `assert-css: (
    "#source-sidebar details[open] > .files a.selected",
    {"color": "rgb(0, 0, 0)", "background-color": "rgb(255, 255, 255)"},
)`
[ERROR] (line 71) Error: Evaluation failed: The following errors happened (for selector `#source-sidebar details[open] > .files > a.selected`): [expected `rgb(51, 51, 51)` for key `background-color`, found `rgb(68, 68, 68)`]: for command `assert-css: (
    "#source-sidebar details[open] > .files > a.selected",
    {"color": "rgb(221, 221, 221)", "background-color": "rgb(51, 51, 51)"},

Build completed unsuccessfully in 0:00:20

@GuillaumeGomez
Copy link
Member

Could you create a screenshot illustrating the problem? Because, to me, these look indistinguishable.

Look at the small arrows, on their left to be more precise 😉

Do we want that on mobile? The desktop behavior shouldn't be changed by this pull request, but on mobile, the sidebar acts as a modal drawer, which normally prevents the content that it partially covers up from being interacted with.

It's especially annoying because, if I reach the top or bottom of the drawer and try to scroll, the view underneath will scroll instead rather than generating the "bounce" indication that I've reached the end.

Hum, that is a good point. I think you're right and we should indeed lock it then.

@notriddle notriddle deleted the notriddle/sidebar-fixes branch July 1, 2022 22:14
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 3, 2022
…details, r=GuillaumeGomez

rustdoc: use <details> tag for the source code sidebar

This fixes the extremely poor accessibility of the old system, making it possible to navigate the sidebar by keyboard, and also implicitly gives the sidebar items the correct ARIA roles.

Split out separately from rust-lang#98772
RalfJung added a commit to RalfJung/rust that referenced this pull request Jul 3, 2022
…details, r=GuillaumeGomez

rustdoc: use <details> tag for the source code sidebar

This fixes the extremely poor accessibility of the old system, making it possible to navigate the sidebar by keyboard, and also implicitly gives the sidebar items the correct ARIA roles.

Split out separately from rust-lang#98772
RalfJung added a commit to RalfJung/rust that referenced this pull request Jul 3, 2022
…details, r=GuillaumeGomez

rustdoc: use <details> tag for the source code sidebar

This fixes the extremely poor accessibility of the old system, making it possible to navigate the sidebar by keyboard, and also implicitly gives the sidebar items the correct ARIA roles.

Split out separately from rust-lang#98772
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 4, 2022
…ebar-button, r=GuillaumeGomez

rustdoc: make source sidebar toggle a real button

This fixes tab focus, so that you can open and close the sidebar from keyboard.

This should cause no visible change in appearance at all. The only way you'd know anything different is if you tried to use keyboard controls to open the source code file navigation sidebar.

Separated out from rust-lang#98772
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 5, 2022
…auto-close, r=GuillaumeGomez

rustdoc: improve click behavior of the source code mobile full-screen "sidebar"

On desktop, if you open the source code sidebar, it stays open even when you move from page to page. It used to do the same thing on mobile, but I think that's stupid. Since the file list fills the entire screen on mobile, and you can't really do anything with the currently selected file other than dismiss the "sidebar" to look at it, it's safe to assume that anybody who clicks a file in that list probably wants the list to go away so they can see it.

Split out separately from rust-lang#98772
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 9, 2022
…scroll-lock, r=jsha

rustdoc: improve scroll locking in the rustdoc mobile sidebars

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 rust-lang#98772
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants