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

More natural rustdoc typography styling #54772

Closed
wants to merge 6 commits into from

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Oct 3, 2018

This patch is similar in spirit to rust-lang/blog.rust-lang.org#263 and onur/docs.rs#240; it removes the specific font overrides in rustdoc.css so that the browser will use the user's configured fonts. This reduces the weight of all rustdoc pages (yay for mobile!) while also letting users use whatever fonts they prefer.

The net result of the change is that the page will look less "uniform" across different browsers, OSes, and devices, but the overall aesthetic should be preserved. It also means that user preferences are respected, and that legibility is improved for users with particular font needs, or on platforms that sometimes gets confused about how to render fonts nicely (ehrm, Linux).

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(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 Oct 3, 2018
This patch is similar in spirit to rust-lang/blog.rust-lang.org#263 and
onur/docs.rs#240; it removes the specific font overrides in rustdoc.css
so that the browser will use the user's configured fonts. This reduces
the weight of all rustdoc pages (yay for mobile!) while also letting
users use whatever fonts they prefer.

The net result of the change is that the page will look less "uniform"
across different browsers, OSes, and devices, but the overall aesthetic
should be preserved. It also means that user preferences are respected,
and that legibility is improved for users with particular font needs, or
on platforms that sometimes gets confused about how to render fonts
nicely (ehrm, Linux).
@jonhoo jonhoo changed the title Don't override the user's fonts [WIP] More natural rustdoc typography styling Oct 3, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 3, 2018

2018-10-02-223949_1920x1080_scrot
(left: before, right: after)

Note that this is with my font settings, so it may look different for different users.

@jonhoo jonhoo changed the title [WIP] More natural rustdoc typography styling More natural rustdoc typography styling Oct 3, 2018
@GuillaumeGomez
Copy link
Member

It seems like a good idea since it reduces the whole pages size but the rendering isn't unified anymore. Not sure what to think about this last part...

cc @rust-lang/rustdoc

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 3, 2018

@GuillaumeGomez Why is the rendering being unified a plus?

@GuillaumeGomez
Copy link
Member

It's like a signature. Also, I just thought about it but it'll likely break the UI tests.

@QuietMisdreavus
Copy link
Member

I'm skeptical about this. It puts aspects of the pages' appearance out of our control, and can lead to situations that are hard to verify, because people's system fonts are going to be different. For example, inline code spans look awkward in my browser at work (Chrome, Windows 7) because its choice of monospace font uses a shorter line-height than its sans-serif font:

image

I agree that this will allow well-configured systems to have clearer fonts available, but i can't shake the feeling that it will look worse on some systems. We'll also need to be more careful about using text symbols as labels - right now, different elements use a circle-i character, a warning symbol, and a microscope emoji to flag certain sections. For example, Windows 7 (and many Linux systems) may not have an emoji font loaded, leaving our "unstable" banner with a little box: (On the other hand, the microscope emoji doesn't load for me with our current docs, so maybe this concern is moot.)

image

(See again how the monospace font is way smaller than the sans-serif font in that banner.)

Related to the text icons, the baseline for some of them seems to be different in certain situations. For example, in your own screenshot, the "important traits" icon is no longer inline with the "fold docs" icon. On my own system, the smaller monospace font also causes the "fold docs" icon to get out of line with the function signature:

image

(Humorously enough, this problem doesn't occur in Internet Explorer because it uses Courier New for its monospace font instead of Consolas, which Chrome uses.)


These may each be small things, but they're things we didn't need to worry about before, because we shipped a font of our choosing, giving rust documentation a uniform look and feel above and beyond the page layout. This PR has merit, but i also feel that it will introduce problems that are hard to reproduce, and possibly create complaints from people whose docs are now using a potentially-worse font.

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 4, 2018

@QuietMisdreavus Yeah, you're definitely right that this will make the page read worse for those with unfortunate font combinations, which I agree is unfortunate. The issue with monospaced fonts generally being smaller is also a significant one. I don't actually know how to resolve those, but also feel like it's sad to have to pull in all these fonts and not allow the user to configure their own fonts.

Some of the alignment issues (like the i icon) can be solved automatically by placing things in the same container and using CSS flexbox or grid with proper alignment, but that would be a more significant change. For icons, I think unicode characters (at least some of them) have pretty decent font coverage at this point, so as long as we stick to pretty basic ones, we should be fine.

Maybe the path forward would be a deeper re-write that uses more modern CSS tools like flexbox and grid to set up the page such that these issues go away. The one that we probably can't get away from easily is inline monospace. There is some discussion here and here that may be relevant though, and may solve our woes.

@QuietMisdreavus
Copy link
Member

I suppose a way to figure it out is to decide how many people actually configure their own fonts, how many people even know that's a thing, and how many people would be worse off if we stopped shipping our own font. I doubt it will render anyone's docs illegible. Is there prior discussion about turning off font overrides? Did anyone request this (here or for any of the linked PRs in the OP)? Is this a "best practice" thing? I'm trying to find the motivation.

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 5, 2018

I suspect the number of users who do so is relatively small. It's probably mainly users with special needs, or people like me who care about typography :) No-one requested this beyond me as far as I'm aware, so it is probably a niche case, though there is an argument for this reducing the page size by a decent amount. The current page pulls down eight-or-so fonts, which is not great for loading speed, especially on mobile. I also think this there is a "best practice" argument to be made that you shouldn't pick specific fonts unless you have a good reason to (like you need particular glyphs). Letting the browser choose gives you whatever font is standard for the user's platform, which leads to greater uniformity across the user's experiences, even if the page looks different for different users.

@GuillaumeGomez
Copy link
Member

"Relatively small" is still worse than "none".

@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2018

Ping from triage @steveklabnik / @rust-lang/rustdoc: I assume this requires a team decision.

@TimNN TimNN added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2018
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 17, 2018

I'll close it for now. Too many potential issues for a small doesn't seem to be worth it. A system to import could be fun/useful though, but it'd be on users' side.

Thanks anyway @jonhoo!

@QuietMisdreavus
Copy link
Member

...did you mean to leave the PR open? >_>

@GuillaumeGomez
Copy link
Member

No, I just forgot to close it. 🤣

jonhoo added a commit to jonhoo/rust that referenced this pull request Nov 7, 2018
This is a (much) more constrained version of rust-lang#54772 that also aims at
improving the situation in rust-lang#34681. It removes any font specifications
that are not the "official" rustdoc font, and instead relies on the
browser to provide the fallback font if the official on is not
available. On Linux systems, this is particularly important, as fonts
like Helvetica, Arial, and Times often look pretty bad since they're
pulled from extracted MS fonts. A specification like `serif` or
`sans-serif` lets the browser instead choose a good font.
kennytm added a commit to kennytm/rust that referenced this pull request Nov 8, 2018
…llaumeGomez

Remove intermediate font specs

This is a (much) more constrained version of rust-lang#54772 that also aims at improving the situation in rust-lang#34681. It removes any font specifications that are not the "official" rustdoc font, and instead relies on the browser to provide the fallback font if the official on is not available. On Linux systems, this is particularly important, as fonts like Helvetica, Arial, and Times often look pretty bad since they're pulled from extracted MS fonts. A specification like `serif` or `sans-serif` lets the browser instead choose a good font.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Nov 9, 2018
…llaumeGomez

Remove intermediate font specs

This is a (much) more constrained version of rust-lang#54772 that also aims at improving the situation in rust-lang#34681. It removes any font specifications that are not the "official" rustdoc font, and instead relies on the browser to provide the fallback font if the official on is not available. On Linux systems, this is particularly important, as fonts like Helvetica, Arial, and Times often look pretty bad since they're pulled from extracted MS fonts. A specification like `serif` or `sans-serif` lets the browser instead choose a good font.
@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants