rustdoc: support separate dark theme logo in html_logo_url

[im-lunex]

View all comments

Currently html_logo_url only accepts a single logo, which is shown in all
themes. This adds a new list form so crates can use a different logo for the
dark (and ayu) themes:

#![doc(html_logo_url(light = "logo-light.png", dark = "logo-dark.png"))]

The old form still works and is unchanged.

The light value is required, dark is optional and falls back to light.
A lint is emitted if the list form is missing light.

Changes:

• parser accepts the new form (new LogoUrls struct)
• rustdoc emits a has-dark-logo class and two imgs, CSS swaps by theme
• noscript.css covers the no-JS prefers-color-scheme fallback
• tests: rustdoc-html (both forms), rustdoc-gui (theme switching),
  rustdoc-ui (missing light error)

issue: #160316

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @lolbinarycat

Some changes occurred in GUI tests.

cc @GuillaumeGomez

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @JonathanBrouwer (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 18 candidates

[im-lunex]

I used a has-dark-logo class on the logo anchor rather than CSS :has(), since rustdoc's CSS doesnt use it yet... happy to switch if preferred

[JonathanBrouwer]

r? rustdoc

[GuillaumeGomez]

You can remove this file.

View changes since the review

[im-lunex]

oh sure.

[im-lunex]

removing this file make it fail the tests.. soo keeping it for now.

[GuillaumeGomez]

Overall, not too sure what to think about this feature... You can tweak the logo display with CSS so not sure having an attribute option for it is a good idea.

Anyway, what do you think @rust-lang/rustdoc-frontend ?

[im-lunex]

Overall, not too sure what to think about this feature... You can tweak the logo display with CSS so not sure having an attribute option for it is a good idea.

Anyway, what do you think @rust-lang/rustdoc-frontend ?

i think having an attribute will be helpful cause here everything lives in one single file. still will love you hear what do you think ?

[notriddle]

The ideal way to do this is SVG logo using the prefers-color-scheme query. That way, you only have to download one image file.

Rustdoc doesn't currently support this, because it doesn't work in all of our supported browsers. It works now, in Chrome and Firefox, but SVG's don't inherit their preferred color scheme from the containing page's used color scheme in Safari, so our JS-based theme switcher can't control the color scheme of an SVG logo.

Sigh.

You can test this out in your own browser by visiting https://notriddle.com/test-color-scheme-code/. The actual code is at https://github.com/notriddle/test-color-scheme-code/.

[im-lunex]

Thanks for the context. but the problem with using a single SVG plus prefers-color-scheme for rustdocs theme switcher is bigger than just Safari It also affects Firefox and Chrome Rustdoc changes only the data-theme attribute and CSS variables but not the actual color-scheme property

Because embedded SVGs with prefers-color-scheme follow only the OS or browser setting unless the page sets color-scheme explicitly the logo does not switch based on the user’s rustdoc theme choice For example the kube-rs docs logo stays the same no matter which rustdoc theme you pick

So beyond Safari’s bug relying on one SVG with prefers-color-scheme won’t work well.. right now Using an explicit attribute controlled fix would be better than waiting for browser issues to get fixed.

so what do you think should we keep moving forward with this?

[notriddle]

unless the page sets color-scheme explicitly

The fix for that would be three lines of code, not including tests.

so what do you think should we keep moving forward with this?

We should move forward with this CSS-based fix.

[im-lunex]

that color-scheme fix makes sense and it covers crates with SVG logos that already handle prefers-color-scheme on chrome and firefox. before we settle on it though, a couple quick things to point out:

1. it leaves out raster logos completely. plenty of crates like kube-rs use PNG or JPEG logos, which can’t switch modes using media queries. this fix doesn’t help them at all, whereas the attribute proposal was the only option that actually worked for raster images.

2. safari is still broken. looking at issue Adaptive SVG logos look terrible on dark themes #91653, safari doesn’t pass the color-scheme down to embedded SVGs, so safari users will still see the wrong logo.

basically, this fix only solves SVG logos on chrome and firefox, not logos overall. are we okay with leaving raster logos without a fix, i think for that the attribute fix could be better.

[rust-rfcbot]

@notriddle has proposed to merge this. The next step is review by the rest of the tagged team members:

• @GuillaumeGomez
• @camelid
• @lolbinarycat
• @notriddle

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[camelid]

I won't raise a formal concern since I don't feel strongly enough to block, but as mentioned above, I'm opposed to having theme-related settings (like light and dark variants) embedded into source code via attributes. It just feels like a mixing of concerns.

@notriddle I'm also confused since you said we should go ahead with the CSS-based approach but then also started FCP for this attribute approach. Could you explain a bit more?

[notriddle]

This attribute-based approach is CSS-based. Sorry if I was confusing everyone. I am in favor of the approach taken by this PR, because it works in Safari, unlike the SVG-based approach.

[GuillaumeGomez]

A full CSS approach is also possible, without requiring a change in the attribute. You use background-image and make the icon change based on the theme. I think this approach is better (and it also doesn't need to update the attribute, yeay).

[im-lunex]

@GuillaumeGomez good call on the pure css swap. that definitely solves the safari and raster image issues.

but there are the gaps:

1. where does rustdoc get the dark url from if we dont add an attribute? would they have to pass it via custom-css?
2. if we drop the <img> tag for a background-image, we lose alt text for screen readers and people can't right click to open/save the logo anymore

how do you wanna handle the url passing and a11y stuff here?

[GuillaumeGomez]

1. where does rustdoc get the dark url from if we dont add an attribute? would they have to pass it via custom-css?

Just like how we handle the theme:

:root[data-theme="dark"] .sidebar .logo-container {
    background-image: ...;
}

:root[data-theme="dark"] .sidebar .logo-container > img {
    /* Hide the original/light theme image eventually so we can put the background-image instead?
         If you do that, don't forget to give a height and width to the logo-container element. */
    display: none;
}

You can add custom CSS with --extend-css.

2. if we drop the tag for a background-image, we lose alt text for screen readers and people can't right click to open/save the logo anymore

Fair. We could move/duplicate the alt attribute to the parent <a> element where the background-image would be.

[im-lunex]

@GuillaumeGomez the <a> will just give it the half of a11y and also it limits the Open Image in a New Tab things.. but the attribute will keep up the natural <img> tag (and was kinda perfect & natural but its just a bit long). and also it make developers write extended-CSS that kinda feels like a step back..

• But yeah respect your fix nice one if we like okay with all those small gaps then we can go along and will love to hear you..

[GuillaumeGomez]

Oh apparently you can use content with img:

:root[data-theme="dark"] .sidebar .logo-container > img {
    content: url("<path to image>");
}

Wouldn't that solve all listed issues?

[GuillaumeGomez]

There are a list of alternatives listed here.

I like the one about the border too:

.myClass {
    border: solid;
    border-width: 0 480px 320px 0;
    border-image: url("http://i.imgur.com/uKnMvyp.jpg") 0 100% 100% 0;
}

[im-lunex]

@GuillaumeGomez Thanks for looking into it

Oh apparently you can use content with img:

:root[data-theme="dark"] .sidebar .logo-container > img {
    content: url("<path to image>");
}

Wouldn't that solve all listed issues?

this one is nice and apparently fixes the issue but im not that sure of this one cause this dosnt yet fix the a11y issue. also this content: url most of the time broken in browsers like Safari soo would be better if we tested this one in safari first.

i dont know if im being annoying or not but (https://developer.mozilla.org/en-US/docs/Web/Security/Defenses/Mixed_content) this might help see the chart given at the bottom.

[GuillaumeGomez]

Well, the border trick should work for all cases, no?

[im-lunex]

Well, the border trick should work for all cases, no?

not sure, we should test it out in safari(cause according to online things safari always had issues with border-images, border-radius this may cause pixelation, blurring, or misaligned edges) and this will not exactly fix the UX issue that remains the same(or worse cuz the border image dosnt actually treat logos like images they are kind of like CSS style prints) with those CSS configs... i dont see any perfect and natural fix for this issue with just CSS

border-image: url("http://i.imgur.com/uKnMvyp.jpg") 0 100% 100% 0;

plus its a lots of extra works for the authors to write those rare CSS properties... cuz i dont think they will love using those & tuning the size things for ratio conflicts?

for context:

• https://bugs.webkit.org/show_bug.cgi?id=23198
• https://www2.webkit.org/show_bug.cgi?id=99922
• https://bugs.webkit.org/show_bug.cgi?id=40509

some of those maybe fixed not sure

[GuillaumeGomez]

this will not exactly fix the a11y issue that remains the same(or worse cuz the border image dosnt actually treat logos like images they are kind of like CSS style prints) with those CSS configs... i dont see any perfect and natural fix for this issue with just CSS

That's where I'm confused. What a11y issue(s) there would be? The img and its attribute would still be there after all, nothing changes there.

[im-lunex]

Also, beyond the potential Safari quirks with border-image, forcing crate authors into CSS workarounds means they'd have to write custom --extend-css and tweak aspect ratios manually—plus we lose proper <img> semantics for accessibility.

(Side note: never mind and let me know if Im missing something obvious or getting details wrong here still learning and not much experienced about it and just trying to make sure the outcome is perfect. and thanks for putting a lot of time on this PR appreciate it)

[im-lunex]

That's where I'm confused. What a11y issue(s) there would be? The img and its attribute would still be there after all, nothing changes there.

soo the thing is that we dont get the

• save image
• copy image address
• open img in new tab things

when we use the border-image approach cuz it will work in light mode but in dark mode when we interact with it it will result in wrong file also high contrast & reader modes will cause issues for the border being overridden also rendering things with WebKit/Blink renderers treat it as a sliced bitmap canvas, also we lost the drag and drop thing...

[GuillaumeGomez]

That's not a11y though, that's UX.

[im-lunex]

That's not a11y though, that's UX.

sorry my bad...

[GuillaumeGomez]

So yeah, the usual issue: a11y vs UX vs UI. If CSS was more powerful, we would live in a perfect world I guess. :')

[im-lunex]

So yeah, the usual issue: a11y vs UX vs UI. If CSS was more powerful, we would live in a perfect world I guess. :')

yeah fr.. hahhh what if we could say 'less rewrite CSS in rust'. dream world

[GuillaumeGomez]

It's done already, called stylo. Give it a look. ;)

One of the very first "real world" Rust projects.

[im-lunex]

It's done already, called stylo. Give it a look. ;)

One of the very first "real world" Rust projects.

in this cursed world a thing like this is soo dymn underrated when i searched stylo holy god it resulted a random Pakistani best ladies shoes brand... 🙏🏻

[GuillaumeGomez]

"stylo" means pencil in french. So now guess why this project name was picked. ;)

[im-lunex]

"stylo" means pencil in french. So now guess why this project name was picked. ;)

the guy who named it knew some random Pakistani best ladies shoes brand gonna replace it.. great thing btw but this one lost to fate..

[notriddle]

The HTML5 <picture> element would be a better way to do this, but making it pick the correct image seems like it would require PreferenceManager, which is still behind a feature flag.

[im-lunex]

The HTML5 <picture> element would be a better way to do this, but making it pick the correct image seems like it would require PreferenceManager, which is still behind a feature flag.

exactly. could be a good idea, but PreferenceManager is experimental/behind-a-flag, so its not something rustdoc can ship on today across Chrome/Firefox/Safari. also this one will still keep the same fundamental issue that was with the previous SVG approch...

[im-lunex]

@GuillaumeGomez @notriddle soo any decision ? what we gonna do about it. the attribute approach gives the best outcome till now...

[GuillaumeGomez]

I personally don't think it's worth extending the attribute for that. It's now up to the rest of the team. Also, this kind of decision happens over weeks, not days. So no need to ping this often.