-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix font size for [src] links in headers #92404
Conversation
Some changes occurred in HTML/CSS/JS. |
Sorry, I don't have enough familiarity with the CSS to review. r? @jsha |
Thanks for fixing this! This is also mentioned in some of the readability / accessibility threads around styling - that the I think the actual fix is not quite right - it's providing overrides on top of overrides on top of overrides. What we want is to set the font size at the correct level and then avoid overriding it. Also, our heading sizes are 1em, 1.1em, 1.15em, 1.3em, 1.4em, 1.5em. 1.1em corresponds to 17.6px, so setting srclink to 17px is still not quite exactly right. One of the problems here: Typically we'd like to set the font size on the That means we should set the font size on the parent element. So for instance the outer div here: <div id="associatedconstant.MAX" class="associatedconstant has-srclink">
<div class="rightside"><span class="since" title="Stable since Rust version 1.43.0">1.43.0</span>
<a class="srclink" href="../src/core/num/f32.rs.html#404" title="goto source code">[src]</a>
</div>
<a href="#associatedconstant.MAX" class="anchor"></a>
<h4 class="code-header">pub const <a href="#associatedconstant.MAX" class="constant">MAX</a>:
<a class="primitive" href="primitive.f32.html">f32</a>
</h4>
</div> And the outer div here: <div id="impl-1" class="impl has-srclink">
<div class="rightside">
<a class="srclink" href="../src/core/num/f32.rs.html#375-1119" title="goto source code">[src]</a>
</div>
<a href="#impl-1" class="anchor"></a>
<h3 class="code-header in-band">impl <a class="primitive" href="primitive.f32.html">f32</a></h3>
</div> Then we can remove the overrides of font-size for h3.code-header and h4.code-header:
Replacing them with (I think) Alternately, maybe we shouldn't set a "global" h3 / h4 font size, but should have a set of font sizes for prose headings, and a separate set of font sizes for code headers, so they don't conflict. |
Also part of the problem is that the rule we use to make the target highlight is:
That We need to reduce this selector to just |
Doing some experimentation, I realized our current approach of setting font-size in terms of em is probably a bad fit, because if we apply such a rule multiple times we will get compounding results. This is explained here: https://developer.mozilla.org/en-US/docs/Web/CSS/font-size#ems
Edit: actually "rem" fixes this problem: "rem values were invented in order to sidestep the compounding problem. rem values are relative to the root html element, not the parent element." And defining font sizes in px is not accessible. So I think we should use rem. I'm working on a counterproposal branch, which I'll have up in a sec. |
Here's my counterproposal branch: https://github.com/rust-lang/rust/compare/master...jsha:heading-sizes?expand=1. Instead of adding more overrides, I removed some of the already-existing overrides, and made the overall heading rules more specific. The idea here is that we would have specific font-size rules for different headings in difference situations. I think the rules I've set up cover all situations, but to be sure I added (temporarily) default rules for h2-h3 that set a very large font-size. If you spot any inappropriately large headings when browsing around the demo, that's a bug I need to fix. If we go this route I would remove those too-large rules before merge. I also switched the font-size specifications for the rules I touches so they're in rem. There are some remaining font-size specifications elsewhere in px, em, and %. We should follow up in a separate PR to change those all to rem. Demos at: https://rustdoc.crud.net/jsha/heading-sizes/std/string/struct.String.html |
@jsha Your proposal looks great but is incomplete: The background should be on all content like this: But otherwise, I think it's a better take. Do you mind if I cherry-pick your commit and modify it? |
Please go ahead!
…On Thu, Dec 30, 2021, 07:18 Guillaume Gomez ***@***.***> wrote:
@jsha <https://github.com/jsha> Your proposal looks great but is
incomplete:
[image: Screenshot from 2021-12-30 13-15-46]
<https://user-images.githubusercontent.com/3050060/147751292-56449d63-9b49-4231-8fc2-7f2394b8a8fd.png>
The background should be on all content like this:
[image: Screenshot from 2021-12-30 13-17-04]
<https://user-images.githubusercontent.com/3050060/147751308-4ae0c46e-b2c6-477c-81e9-f05613b17fb3.png>
But otherwise, I think it's a better take. Do you mind if I cherry-pick
your commit and modify it?
—
Reply to this email directly, view it on GitHub
<#92404 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABVYLMDOJ2WLOUK5GWQURDUTREZJANCNFSM5K577NRQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…omez Set font size proportional to user's font size According to MDN (https://developer.mozilla.org/en-US/docs/Web/CSS/font-size), > To maximize accessibility, it is generally best to use values that are relative to the user's default font size. > Defining font sizes in px is not accessible, because the user cannot change the font size in some browsers. Note that changing font size (in browser or OS settings) is distinct from the zoom functionality triggered with Ctrl/Cmd-+. Zoom functionality increases the size of everything on the page, effectively applying a multiplier to all pixel sizes. Font size changes apply to just text. For relative font sizes, we could use `em`, as we do in several places already. However that has a problem of "compounding" (see MDN article for details). The compounding problem is nicely solved by `rem`, which make font sizes relative to the root element, not the parent element. Since we were using a hodge-podge of pixel sizes, em, rem, and percentage sizes before, this change switches everything to rem, while keeping the same size relative to our old default of 16px. 16px is still the default on most browsers, for users that haven't set a larger or smaller font size. Part of rust-lang#59845. Note: this will conflict with rust-lang#92404. We should merge that first (once it's done) and I'll resolve the merge conflicts. r? `@GuillaumeGomez` Demo: https://rustdoc.crud.net/jsha/font-size-access/std/string/struct.String.html
☔ The latest upstream changes (presumably #92580) made this pull request unmergeable. Please resolve the merge conflicts. |
360859c
to
a0085f2
Compare
@jsha It would be actually more complicated to do what you suggested from what I tried: it requires a lot more of code to achieve the same result. :-/ |
Which part? Fixing the target selector, or fixing the heading selectors? I'm okay not making the suggested fixes to heading selectors, but I think the fix to the target selector is needed if you want this to be reliably correct. |
I was talking about the heading selectors. I will send the target selector change in a few seconds. ;) |
Done! |
@bors r+ rollup |
📌 Commit 3b70c6e has been approved by |
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#92058 (Make Run button visible on hover) - rust-lang#92288 (Fix a pair of mistyped test cases in `std::net::ip`) - rust-lang#92349 (Fix rustdoc::private_doc_tests lint for public re-exported items) - rust-lang#92360 (Some cleanups around check_argument_types) - rust-lang#92389 (Regression test for borrowck ICE rust-lang#92015) - rust-lang#92404 (Fix font size for [src] links in headers) - rust-lang#92443 (Rustdoc: resolve associated traits for non-generic primitive types) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…-inline-style, r=jsha Create CSS class instead of using inline style for search results I saw this change in the update you proposed in rust-lang#92404. :) r? `@jsha`
…-inline-style, r=jsha Create CSS class instead of using inline style for search results I saw this change in the update you proposed in rust-lang#92404. :) r? ``@jsha``
Fixes #90384.
cc @jsha
r? @camelid