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 accessibility: use an icon for the [-]/[+] controls #87207

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Jul 17, 2021

This way, we can show the plus and minus buttons on screens, while voice
control will read off actual words "Collapse" and "Expand" instead of reading
"open brace minus close brace" and "open brace plus close brace".

Part of #87059

Preview it at: https://notriddle.com/notriddle-rustdoc-test/rustdoc-brace-minus-brace/std/index.html

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @ollie27

(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 17, 2021
@notriddle notriddle force-pushed the notriddle/rustdoc-brace-minus-brace branch from c296b06 to f4a4db4 Compare July 17, 2021 00:58
@notriddle notriddle marked this pull request as draft July 17, 2021 00:58
@rust-log-analyzer

This comment has been minimized.

This way, we can show the plus and minus buttons on screens, while voice
control will read off actual words "Collapse" and "Expand" instead of reading
"open brace minus close brace" and "open brace plus close brace".

Part of rust-lang#87059
@notriddle notriddle force-pushed the notriddle/rustdoc-brace-minus-brace branch from f4a4db4 to d8282d1 Compare July 17, 2021 01:20
notriddle added a commit to notriddle/notriddle-rustdoc-test that referenced this pull request Jul 17, 2021
@notriddle notriddle marked this pull request as ready for review July 17, 2021 01:25
font-weight: 300;
font-size: 0.8em;
letter-spacing: 1px;
cursor: pointer;
width: 17px;
height: max(17px, 1.1em);
background: data-url(plus-17x17.png) no-repeat top left;
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply using the image directly instead of going through a data-url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, the CSS file never links to images. This is probably because the images have version numbers (plus.png becomes plus1.55.0.png), so it would require patching the style sheet either way.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, my PR doing that hasn't been merged yet. Take a look at how I did: https://github.com/rust-lang/rust/pull/86892/files#diff-39d31d43319757bdd745b6b58f1d8593af4d28454f8ad0259b802bd776898e58R178-R203

That will allow you to simply add the background url from the rust side directly.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jul 17, 2021

That looks quite nice overall. Just one note: the image size isn't matching the <details> font size anymore.

Another question: wouldn't it be possible to create an SVG so we can resize it without problem? Or even simply make a bigger image so we simply scale it down?

And finally: is it looking better to you @ahicks92 and @DataTriny?

@ahicks92
Copy link
Contributor

The thing is that screen readers actually already indicate that those can be expanded and collapsed, at least so long as they're "buttons" (I know they're not in the HTML). So how much this matters really depends on what we do about that, though for the moment it's redundant when taken in isolation.

Also moving this to the "end of the line" would still be something we should do: "collapse pub fn then" etc. is still spammy. This is certainly an improvement, but it's important to me to flag that this doesn't really resolve that part of the issue in the event that you're thinking that it does.

@GuillaumeGomez
Copy link
Member

@ahicks92 This change (moving to <details>) allowed us to remove a lot of JS, improving performance and reducing maintenance cost. For us, it was really a big improvement. The goal now is to try to make it not a loss for you. I'm just not sure how exactly. 😕

@notriddle
Copy link
Contributor Author

notriddle commented Jul 17, 2021

@GuillaumeGomez

That looks quite nice overall. Just one note: the image size isn't matching the <details> font size anymore.

The funny part is that I actually did that on purpose. Take this screenshot of the new version (on the left) vs the old one (on the right):

A8CC22B6-EC3E-465B-9827-1B14D01F704B

In the old version, the button on Expand Description is bigger than the one on impl<T> Vec<T, Global>. It’s sized so that it flows with the text.

In the new version, they’re exactly the same size, so they both look like they occupy a shared “gutter” space on the left. It also, hopefully, makes it look less like source code. They’re not text. They’re icons.

CC #59851

@notriddle
Copy link
Contributor Author

notriddle commented Jul 17, 2021

@ahicks92

I’ve pushed a new patch to put the “Expand”/“Collapse” text on the end of the line. It should now read more like “pub fn bla bla bla collapse”.

Now I just need to figure out a way to do the same thing with the "go to source code" link...

@GuillaumeGomez
Copy link
Member

@notriddle It makes sense. We can always discuss it later if someone complains about it. :)

@ahicks92
Copy link
Contributor

What I'm trying to say is that with the current HTML, NVDA is smart enough to realize that the entire thing is already expanded or collapsed. Other screen readers may or may not be smart enough to do the same and the redundant info is now at the end of the line where it can be ignored, so in so much as my approval is required you have it. I just thought it was worth offering further clarification and specificity as to what it does and doesn't solve.

In the long run it looks like the fact these are buttons is the details element itself, and as regards that specific aspect of this I'm fine if they stay buttons. It's not ideal, but it's also using HTML which is supposed to be accessible out of the box in the way you're supposed to use it, I'm enough of a programmer to admit that makes it not your problem, and there is a next button key in the screen reader. I'm happy to move that discussion to the main issue before anyone does anything taking a lot of man-hours. Suffice it to say that I'm not necessarily advocating for making you give me my h tags back, just for fixing the efficiency of navigation that got broken by removing them.

@GuillaumeGomez
Copy link
Member

Suffice it to say that I'm not necessarily advocating for making you give me my h tags back, just for fixing the efficiency of navigation that got broken by removing them.

You mean (for example) making the function headers back into h4? I thought it was only in the table of content but maybe I misunderstood...

For the rest, maybe we can simply land this improvement and check what remains to be done?

@ahicks92
Copy link
Contributor

As I said, I'm fine with this improvement. I'll provide more clarity on the main issue as to what the problem is as to not have it get lost in closed prs.

@GuillaumeGomez
Copy link
Member

@notriddle: Why did you close it? It seems like you're on the right track though... If you lack time to finish it (for the css declaration as I said), I can do it if you want?

@notriddle
Copy link
Contributor Author

Exactly. I don’t really have time to finish it. It also really isn’t as important, now.

@GuillaumeGomez
Copy link
Member

No problem, I'll finish it in the next days then. Thanks a lot for paving the way. ;)

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2021
…brace, r=notriddle

Rustdoc accessibility: use an icon for the [-]/[+] controls

This is a reopening of rust-lang#87207 with improvement for the way of generating the `background-image` CSS property.

I quote from the original PR:

> This way, we can show the plus and minus buttons on screens, while voice
> control will read off actual words "Collapse" and "Expand" instead of reading
> "open brace minus close brace" and "open brace plus close brace".

Part of rust-lang#87059

r? `@notriddle`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2021
…brace, r=notriddle

Rustdoc accessibility: use an icon for the [-]/[+] controls

This is a reopening of rust-lang#87207 with improvement for the way of generating the `background-image` CSS property.

I quote from the original PR:

> This way, we can show the plus and minus buttons on screens, while voice
> control will read off actual words "Collapse" and "Expand" instead of reading
> "open brace minus close brace" and "open brace plus close brace".

Part of rust-lang#87059

r? ``@notriddle``
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants