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 #87663

Merged
merged 4 commits into from
Aug 4, 2021

Conversation

GuillaumeGomez
Copy link
Member

This is a reopening of #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 #87059

r? @notriddle

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: rustdoc UI (generated HTML) labels Jul 31, 2021
Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Wait, actually, there's one thing that I'd like to try, but I'd like a few other people to check this out on their configuration, to see if they also prefer it this way.

The thing that I don't like about the current version of this is that it makes the icons look blurrier on hidpi screens. I'm not quite sure how to fix that, but I think adding these CSS rules might help:

image-rendering: crisp-edges;
image-rendering: -moz-crisp-edges; /* Firefox before version 65 */
image-rendering: -webkit-optimize-contrast; /* Safari */
-ms-interpolation-mode: nearest-neighbor; /* EdgeHTML Edge and Trident */
image-rendering: pixelated; /* Chrome */

@notriddle
Copy link
Contributor

notriddle commented Jul 31, 2021

The one on the left is with the image-rendering CSS things, while the one on the right doesn't have them:

image

Here's a web upload showing it, so that we can test this on as many browsers as we can get. Because the caniuse site has quite a few caveats with getting good pixel art scaling in browsers.

https://notriddle.com/notriddle-rustdoc-test/pixelated-plus-minus/std/index.html

I seem to have a version that works in Chromium and Firefox on Linux, and Mobile Safari on an iPad, but I'm not terribly confident in it. We really want this icon to snap to the pixel grid. Hard. And it's not as easy to get that as I would like.

@Urgau
Copy link
Member

Urgau commented Jul 31, 2021

@notriddle Maybe with them in SVG it will help.

Toggle minus:

<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" stroke="#000" fill="none"><path d="M4.5 2H2v12h2.5m7-12H14v12h-2.5"/><path d="M4.5 8h7"/></svg>

Toggle plus:

<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" stroke="#000" fill="none"><path d="M4.5 2H2v12h2.5m7-12H14v12h-2.5"/><path d="M4.5 8h7"/><path d="M8 11.5V8.125h0V4.5"/></svg>

@GuillaumeGomez
Copy link
Member Author

@Urgau: Let's try svg! Thanks for the conversion. ;)

@GuillaumeGomez
Copy link
Member Author

The SVG you provided is all blurry on my computer, it gets better when I zoom in, but it's not great... I've read around that using number without decimal makes it better, maybe it's worth a try? I tried to "fix" the SVG but it was a complete failure. ^^'

@Urgau
Copy link
Member

Urgau commented Aug 2, 2021

The SVG you provided is all blurry on my computer, it gets better when I zoom in, but it's not great... I've read around that using number without decimal makes it better, maybe it's worth a try? I tried to "fix" the SVG but it was a complete failure. ^^'

My SVGs have a size of 16x16 like the original PNG files, this size is very small and when a software tries to display an image with such a tiny size, it renders it in this original size and then adapts it to the target size (which is much bigger) but some software "forget" that an SVG can be rendered at any size (which avoids any scaling and is the reason for SVGs) and so they render it and adapt it instead of rendering it at the target size, that maybe be why it can appear blurry on your computer.

You can see on this jsfiddle my SVGs in their original sizes but also in 350x350, I don't see any blur, do you ?

Do you want me to increase their default size ? and by curiosity, what software are you using to render the SVGs ?

I never heard that number without decimal makes svg better, but I'm not an expert either.

@notriddle
Copy link
Contributor

The SVG you provided is all blurry on my computer, it gets better when I zoom in, but it's not great...

If it gets better when you zoom in, then it's not being caused by the browser rendering it at the original size and scaling. It's caused by the lines not being placed exactly on pixel boundaries.

This is why the original PNG files are not 16x16. They're 17x17. By using an odd number of pixels, you can draw a 1px line in the exact center of the image without aliasing.

@GuillaumeGomez
Copy link
Member Author

@Urgau I meant what @notriddle said: the pixel boundaries are not integers but decimals, which is what I think is the issue.

@Urgau
Copy link
Member

Urgau commented Aug 3, 2021

This is why the original PNG files are not 16x16. They're 17x17. By using an odd number of pixels, you can draw a 1px line in the exact center of the image without aliasing.

Heu... I don't know what happen, I'm pretty sure I have enter 17x17 for the size; I don't know what to say, I should have checked, sorry.

@Urgau I meant what @notriddle said: the pixel boundaries are not integers but decimals, which is what I think is the issue.

If that the case I don't know how to resolve that because Inkscape is generating this decimal values.


In any case I have redone and attached the SVGs with their correct size 17x17 (I checked this time). Let me know if it's still not good, maybe I can try something else.

Toggle minus (17x17)

<svg width="17" height="17" stroke="#000" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7"/></svg>

Toggle plus (17x17)

<svg width="17" height="17" stroke="#000" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7M8.5 12V8.625v0V5"/></svg>

PS: By redoing them I even manage to shrink them down a little more

@GuillaumeGomez
Copy link
Member Author

Ah nice, thanks! Let's try with these new images then.

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
@GuillaumeGomez
Copy link
Member Author

@Urgau Not blurry anymore, thanks a lot!

@notriddle I think the PR is ready. :)

@notriddle
Copy link
Contributor

Can you please publish a built copy of the standard library docs, like I’ve been doing? That way, we can all easily test the results.

@GuillaumeGomez
Copy link
Member Author

Sure, here you go: https://guillaume-gomez.fr/rustdoc-test/foo/struct.Foo.html (not std, simply a small crate).

Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Add the attribute shape-rendering="crispEdges" to each of the <svg> elements, and then I'll approve it.

Here's a side-by-side comparison of the SVG, on the left is without crispEdges, and the right is with crispEdges. Both are zoomed in at 133%, so that the line would need to be aliased, and magnified with no interpolation after taking a screenshot.

image

@@ -0,0 +1 @@
<svg width="17" height="17" stroke="#000" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7"/></svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<svg width="17" height="17" stroke="#000" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7"/></svg>
<svg width="17" height="17" stroke="#000" fill="none" shape-rendering="crispEdges" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7"/></svg>

@@ -0,0 +1 @@
<svg width="17" height="17" stroke="#000" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7M8.5 12V8.625v0V5"/></svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<svg width="17" height="17" stroke="#000" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7M8.5 12V8.625v0V5"/></svg>
<svg width="17" height="17" stroke="#000" fill="none" shape-rendering="crispEdges" xmlns="http://www.w3.org/2000/svg"><path d="M5 2.5H2.5v12H5m7-12h2.5v12H12M5 8.5h7M8.5 12V8.625v0V5"/></svg>

@GuillaumeGomez
Copy link
Member Author

That looks better, indeed! Thanks to both of you for the help!

@bors: r=notriddle

@bors
Copy link
Contributor

bors commented Aug 3, 2021

📌 Commit 6fe0972 has been approved by notriddle

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 3, 2021
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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81797 (Add `core::stream::from_iter`)
 - rust-lang#87267 (Remove space after negative sign in Literal to_string)
 - rust-lang#87663 (Rustdoc accessibility: use an icon for the [-]/[+] controls)
 - rust-lang#87720 (don't use .into() to convert types to identical types (clippy::useless_conversion))
 - rust-lang#87723 (Use .contains instead of manual reimplementation.)
 - rust-lang#87729 (Remove the aarch64 `crypto` target_feature)
 - rust-lang#87731 (Update cargo)
 - rust-lang#87734 (Test dropping union fields more)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 75e1487 into rust-lang:master Aug 4, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 4, 2021
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-brace-minus-brace branch August 4, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: rustdoc UI (generated HTML) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants