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] Add sans-serif font setting #133636

Merged
merged 6 commits into from
Jan 30, 2025
Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 29, 2024

Fixes #52449.

This PR adds a new setting introducing the possibility to switch to a sans-serif font (Fira Sans) for the text.

Can be tested here.

cc @rust-lang/rustdoc-frontend
r? @notriddle

@rustbot rustbot added 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. labels Nov 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@notriddle
Copy link
Contributor

If you want Fira Sans for the main body type, then Fira Mono should be used for code. That way they have consistent point sizes, x-height, and other features.

@GuillaumeGomez
Copy link
Member Author

Sounds good to me!

@notriddle
Copy link
Contributor

notriddle commented Nov 29, 2024

The other problem I have with this change is that it was discussed before and rejected. I'm not sure why, though, so I'm not sure how to avoid hitting the same problems as back then:

#16207 #16173

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Checking what studies say about it.

So seems like there is no difference or it's a bit better in favour of sans serif. In any case I want to ensure the team is on board with this as it's a big UI change. Better take our time to ensure that we're not missing something before approving the change.

@notriddle
Copy link
Contributor

I realized I was kind of terse. Here's a slightly more detailed opinion, along with the reason why I hold it:

  1. If you choose a body font, you need to choose a code font that goes with it, because Rustdoc mixes them freely in layouts like this one:

    before after
    image image

    If you're not sure why I think "before" looks better, focus on the phrase "or target".

  2. If we change something, we need to address the reasons why it was done that way originally. It's fine if the reason seems to be "nobody thought of it," but the choice of a serif font here was definitely deliberate.

    As a user with bad enough vision that they won't let me drive a car, Fira Sans doesn't seem significantly more readable than Source Serif. This adds up, since, even though it's a serif font, Source Serif is supposed to be usable as a screen font. On the other hand, cataracts, astigmatism, and amblyopia are all "bad eyesight," but they have totally different effects. Just because it WorksForMe(tm) doesn't mean it works for anybody else.

I think this is why even relatively un-customizable apps like Apple Books have multiple font packs built in. It's few enough that they can make sure every one of them looks good, but diverse enough to satisfy the most common problems someone might have with extended reading.

I think we should add an option to the Settings panel to change the font, just like Pocket and Firefox's Reader Mode. This is because, when you consider the "bikeshedding effect," there really aren't that many people complaining about Source Serif.

Screenshot_2024-11-29_10-57-04

Screenshot_2024-11-29_11-14-05

Screenshot_2024-11-29_11-37-49

Now that CSS Variables are being used, implementing this shouldn't actually be that hard.

@GuillaumeGomez
Copy link
Member Author

I realized I was kind of terse. Here's a slightly more detailed opinion, along with the reason why I hold it:

No I actually agreed with your point. I should have looked into it beforehand.

If you choose a body font, you need to choose a code font that goes with it, because Rustdoc mixes them freely in layouts like this one:

Starting the moment you mentioned it, I was planning to change it. I didn't because I don't want to invest more time into it unless I'm sure the team is onboard.

I think we should add an option to the Settings panel to change the font, just like Pocket and Firefox's Reader Mode.

I think as a first step (and eventually as final step too) it might be better. Like that people can pick serif or sans serif and everyone is happy.

@bors
Copy link
Contributor

bors commented Nov 29, 2024

☔ The latest upstream changes (presumably #133634) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

I added a new setting to use "sans serif font":

image

I updated online demo as well.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 3, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Rebased just in case but again, when running ./x.py test collect-license-metadata, no changes...

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Yeah ok, I officially have no idea what's wrong with this script.

],
"spdx": "Apache-2.0 OR CC-BY-SA-4.0 OR MIT"
},
"name": "src/doc/embedded-book",
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it's because you've got your config.toml set up to avoid building the embedded book and gcc, so they don't wind up included in this file even though they should be.

To fix this, would either do a truly complete rust build, or I would just hand-patch in the few lines that are added in your PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that makes more sense now. Thanks a lot!

@GuillaumeGomez GuillaumeGomez force-pushed the sans-serif branch 2 times, most recently from 762eec8 to 58090ad Compare December 4, 2024 16:15
@notriddle
Copy link
Contributor

The PR description and title don't describe what it does any more. That should probably be changed, then it can be more easily FCP-ed.

@rfcbot
Copy link

rfcbot commented Jan 19, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@rust-log-analyzer

This comment has been minimized.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 29, 2025
@rfcbot
Copy link

rfcbot commented Jan 29, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@GuillaumeGomez
Copy link
Member Author

CI fixed, let's go. :)

@bors r=rustdoc

@bors
Copy link
Contributor

bors commented Jan 29, 2025

📌 Commit 2511faf has been approved by rustdoc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 29, 2025
[rustdoc] Add sans-serif font setting

Fixes rust-lang#52449.

This PR adds a new setting introducing the possibility to switch to a sans-serif font (`Fira Sans`) for the text.

Can be tested [here](https://rustdoc.crud.net/imperio/sans-serif/std/index.html).

cc `@rust-lang/rustdoc-frontend`
r? `@notriddle`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 30, 2025
[rustdoc] Add sans-serif font setting

Fixes rust-lang#52449.

This PR adds a new setting introducing the possibility to switch to a sans-serif font (`Fira Sans`) for the text.

Can be tested [here](https://rustdoc.crud.net/imperio/sans-serif/std/index.html).

cc ``@rust-lang/rustdoc-frontend``
r? ``@notriddle``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#133636 ([rustdoc] Add sans-serif font setting)
 - rust-lang#135434 (Match Ergonomics 2024: update edition 2024 behavior of feature gates)
 - rust-lang#135739 (Clean up uses of the unstable `dwarf_version` option)
 - rust-lang#135882 (simplify `similar_tokens` from `Option<Vec<_>>` to `&[_]`)
 - rust-lang#136179 (Allow transmuting generic pattern types to and from their base)
 - rust-lang#136199 (Fix a couple Emscripten tests)
 - rust-lang#136238 (ci: refactor how directories are removed in free-disk-space disk)
 - rust-lang#136251 (use impl Into<String> instead of explicit type args with bounds)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133636 ([rustdoc] Add sans-serif font setting)
 - rust-lang#135434 (Match Ergonomics 2024: update edition 2024 behavior of feature gates)
 - rust-lang#135739 (Clean up uses of the unstable `dwarf_version` option)
 - rust-lang#135882 (simplify `similar_tokens` from `Option<Vec<_>>` to `&[_]`)
 - rust-lang#136179 (Allow transmuting generic pattern types to and from their base)
 - rust-lang#136199 (Fix a couple Emscripten tests)
 - rust-lang#136251 (use impl Into<String> instead of explicit type args with bounds)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 282d866 into rust-lang:master Jan 30, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 30, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2025
Rollup merge of rust-lang#133636 - GuillaumeGomez:sans-serif, r=rustdoc

[rustdoc] Add sans-serif font setting

Fixes rust-lang#52449.

This PR adds a new setting introducing the possibility to switch to a sans-serif font (`Fira Sans`) for the text.

Can be tested [here](https://rustdoc.crud.net/imperio/sans-serif/std/index.html).

cc ```@rust-lang/rustdoc-frontend```
r? ```@notriddle```
@GuillaumeGomez GuillaumeGomez deleted the sans-serif branch January 30, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust docs should at least have the option of using sans-serif fonts
8 participants