Disable stylesheet objects rather than link elements #75063

Cldfire · 2020-08-02T17:24:49Z

Resolves #75011.

I changed the way themes are switched in rustdoc to more closely resemble that of mdbook. This method uses StyleSheet.disabled which has much better browser support than the usage of disabled on link that I introduced in a previous PR.

As added bonuses, we no longer need to use the IDs mainThemeStyle or themeStyle, usage of switchTheme is simpler, and we no longer need the hack that changes the href on the light.css stylesheet.

I've tested this in Firefox, Chromium, and Waterfox Classic, and it works well across all those browsers. @Nemo157 would perhaps be able to test this in Safari as well?

r? @GuillaumeGomez

rust-highfive · 2020-08-02T17:24:52Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

GuillaumeGomez

I'm worried about this change: until the storage.js file is executed, all themes are enabled at the same time, right? This is a huge step backward. :-/

src/librustdoc/html/static/storage.js

Cldfire · 2020-08-02T19:24:09Z

I'm worried about this change: until the storage.js file is executed, all themes are enabled at the same time, right? This is a huge step backward. :-/

Correct, do you think that will be a problem?

GuillaumeGomez · 2020-08-02T19:44:19Z

On slowest computers, it's gonna be an issue for sure: the browser will have to parse and render multiple styles at once. Definitely not optimal...

Cldfire · 2020-08-02T20:12:40Z

On slowest computers, it's gonna be an issue for sure: the browser will have to parse and render multiple styles at once. Definitely not optimal...

True, although this was the behavior of rustdoc before 03f565c. As far as I'm aware this wasn't causing issues before with 2 theme stylesheets, do we believe it would cause an issue with 3?

(EDIT: thinking about this some more we likely have a similar problem in the code's current state anyway? The non-disabled stylesheet (light.css) will load initially, and then after storage.js runs the stylesheet href could get changed to, say, dark.css, and now the effort the browser has spent loading and applying light.css is wasted anyhow. The fact that we don't see content flashing changing themes on page load leads me to believe that storage.js runs before light.css gets applied, though.)

The disabled attribute method on link elements is definitely better in this regard, though. If you don't see it becoming a compatibility issue it might be best to leave things as-is.

GuillaumeGomez · 2020-08-02T20:27:12Z

This is a good point. Only way would be to just compare I guess. Also, what do you @rust-lang/rustdoc ?

jyn514 · 2020-08-02T20:57:44Z

Personally I don't think the themes are going to be affecting render time the most, it's the gobs of generated HTML. If you're worried though, maybe we could load the theme preference from local storage before loading the CSS?

GuillaumeGomez · 2020-08-02T21:02:03Z

maybe we could load the theme preference from local storage before loading the CSS?

Isn't it already what we do?

Manishearth · 2020-08-03T04:29:00Z

On slowest computers, it's gonna be an issue for sure: the browser will have to parse and render multiple styles at once. Definitely not optimal...

No, <style disabled> does not change parsing. What it does change is whether or not the style is applied, and flipping that attribute will cause a relayout to occur. If you set it in the <head> tag this will occur before any layout has happened so there should be no observable change.

GuillaumeGomez · 2020-08-03T07:51:10Z

@Manishearth This is what we do currently but this PR, unless I'm missing something, is removing this mechanism and the disable is added by the JS, not by rustdoc.

Manishearth · 2020-08-03T07:52:25Z

@GuillaumeGomez right, as long as the JS occurs in the head tag it doesn't matter

The parsing is inevitable. It's the relayout you want to avoid

GuillaumeGomez · 2020-08-03T07:54:30Z

Yes, on that point we agree. Thanks for the precision!

JohnCSimon · 2020-08-18T05:19:35Z

ping from triage:
@Cldfire - can you address the change request and/or mark it as completed? Thanks.
#75063 (review)

jyn514 · 2020-08-18T12:19:32Z

This isn't waiting on anything AFAIK - @GuillaumeGomez are you happy with it?

Cldfire · 2020-08-18T12:20:55Z

Yep, this one is good to go if everyone else is happy :)

GuillaumeGomez · 2020-08-18T12:25:28Z

I guess we are. Moving forward then, thanks!

@bors: r+

bors · 2020-08-18T12:25:29Z

📌 Commit c4d0a1b has been approved by GuillaumeGomez

ollie27 · 2020-08-18T14:07:57Z

With JS disabled the ayu theme will now be used instead of the light theme. Is that change intentional?

GuillaumeGomez · 2020-08-18T14:10:08Z

Holding off until @ollie27's concern is resolved.

@bors: r-

Cldfire · 2020-08-19T01:48:22Z

With JS disabled the ayu theme will now be used instead of the light theme. Is that change intentional?

With JS disabled the themes will now conflict with one another (they will all be enabled at the same time), so regardless of which stylesheet comes first in the HTML things could look funky.

Do we need to support no-JS usage? Doesn't that break search?

jyn514 · 2020-08-19T01:53:02Z

Well, I think there's a difference between a feature that's JS-only and completely breaking the page if JS is turned off. I see the way this is implemented is themeSheet.disabled = themeName !== newTheme; - is there a CSS equivalent of that? Or is that the disabled="" that isn't supported in WaterFox?

jyn514 · 2020-08-19T01:53:48Z

Maybe we could have light.css for <noscript> and turn it on with JS only otherwise?

GuillaumeGomez · 2020-08-19T11:07:18Z

Do we need to support no-JS usage? Doesn't that break search?

Yes and yes. We don't consider the search breakage when JS is disabled as an issue though, but the rendering has to be "clean", or at least readable.

Manishearth · 2020-08-19T12:54:36Z

Note: if this is broken under no-JS then it will also be broken if the relevant JS is in a separate file and needs to load, on bad networks. I think the default theme code is inline but if so we need a comment there that makes sure it's never moved out, and if not we need to make this work if that JS file fails to load

Cldfire · 2020-08-26T03:17:34Z

I investigated using rel="stylesheet alternate", and while that does fix the no-JS side of things, it feels very laggy while switching, introduces a color flash while changing themes, and doesn't appear to have good browser support.

I came up with what appears to be a nice compromise here that still uses the disabled attribute on link. I changed it so that the disabled link attribute is used initially on page load to disable everything but the light theme:

                r#"<link rel="stylesheet" type="text/css" href="{}.css"{}>"#,
                Escape(&format!("{}{}{}", static_root_path, t, page.resource_suffix)),
                if t == "light" { "" } else { " disabled" }

And then in the theme switcher we simply remove the disabled attribute from the element before setting the disabled stylesheet property (confusing naming, right?):

            themeSheet.removeAttribute('disabled');
            themeSheet.disabled = themeName !== newTheme;

This works well across most things I tested:

Firefox with JS enabled (everything works as expected)
Firefox with JS disabled (light theme is the only active stylesheet)
Waterfox classic with JS enabled (everything works as expected)

Unfortunately Waterfox Classic with JS disabled is still broken because, well, it doesn't support the disabled attribute on links. Which is exactly what this PR was trying to remove.

I would guess that the set of people using Waterfox Classic is going to happen to intersect with the set of people that want to be able to use rustdoc with JS disabled, so this doesn't really accomplish anything.

The other idea I had was to only load the dark / ayu themes if JS is enabled (by adding them dynamically on page load). Should I try that out or nah?

Maybe we could have light.css for and turn it on with JS only otherwise?

@jyn514 good thought, but unfortunately I don't think <noscript> can help us out here since there's no way for us to prevent the other theme stylesheets from getting loaded and applied to the page when JS is off.

jyn514 · 2020-08-26T03:25:00Z

I don't think can help us out here since there's no way for us to prevent the other theme stylesheets from getting loaded and applied to the page when JS is off.

Can we insert the other stylesheets with JS? So they don't show up at all in the HTML and only appear once the JS executes?

jyn514 · 2020-08-26T03:25:31Z

and if that doesn't work I really think we should close this as wontfix, it's not worth the effort for fixing such a small minority of browsers.

jyn514 · 2020-08-26T03:29:34Z

I don't think can help us out here since there's no way for us to prevent the other theme stylesheets from getting loaded and applied to the page when JS is off.

Can we insert the other stylesheets with JS? So they don't show up at all in the HTML and only appear once the JS executes?

https://stackoverflow.com/a/577002/7669110 might possibly be what we want

Cldfire · 2020-08-26T03:46:33Z

https://stackoverflow.com/a/577002/7669110 might possibly be what we want

I tried something similar to this and it looks like it will work. I don't have time to finish it right now though.

Will return and poke at it another night.

Cldfire · 2020-08-26T23:48:12Z

Branch here that implements that idea (add stylesheets to page using JS in the head tag). Unfortunately this causes flashing on page load on my machine.

Unless anyone has any other bright ideas (or can see something wrong in the above branch) we are unfortunately going to have to close this PR 😞

GuillaumeGomez · 2020-08-27T08:52:34Z

Well, at least now we know...

Disable stylesheet objects rather than link elements

38d6899

rust-highfive assigned GuillaumeGomez Aug 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2020

Cldfire mentioned this pull request Aug 2, 2020

rustdoc page styles are broken with Waterfox #75011

Closed

tidy fix

eea0e39

GuillaumeGomez requested changes Aug 2, 2020

View reviewed changes

src/librustdoc/html/static/storage.js Outdated Show resolved Hide resolved

ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 2, 2020

Apply suggestion

c4d0a1b

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2020

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 18, 2020

jyn514 added the A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc label Aug 25, 2020

Cldfire closed this Sep 1, 2020

Cldfire mentioned this pull request Oct 11, 2020

Add settings to rustdoc to use the system theme #77809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable stylesheet objects rather than link elements #75063

Disable stylesheet objects rather than link elements #75063

Cldfire commented Aug 2, 2020

rust-highfive commented Aug 2, 2020

GuillaumeGomez left a comment

Cldfire commented Aug 2, 2020

GuillaumeGomez commented Aug 2, 2020

Cldfire commented Aug 2, 2020 •

edited

Loading

GuillaumeGomez commented Aug 2, 2020

jyn514 commented Aug 2, 2020

GuillaumeGomez commented Aug 2, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

JohnCSimon commented Aug 18, 2020

jyn514 commented Aug 18, 2020

Cldfire commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

bors commented Aug 18, 2020

ollie27 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Cldfire commented Aug 19, 2020 •

edited

Loading

jyn514 commented Aug 19, 2020

jyn514 commented Aug 19, 2020

GuillaumeGomez commented Aug 19, 2020

Manishearth commented Aug 19, 2020

Cldfire commented Aug 26, 2020 •

edited

Loading

jyn514 commented Aug 26, 2020

jyn514 commented Aug 26, 2020

jyn514 commented Aug 26, 2020

Cldfire commented Aug 26, 2020

Cldfire commented Aug 26, 2020 •

edited

Loading

GuillaumeGomez commented Aug 27, 2020

Disable stylesheet objects rather than link elements #75063

Disable stylesheet objects rather than link elements #75063

Conversation

Cldfire commented Aug 2, 2020

rust-highfive commented Aug 2, 2020

GuillaumeGomez left a comment

Choose a reason for hiding this comment

Cldfire commented Aug 2, 2020

GuillaumeGomez commented Aug 2, 2020

Cldfire commented Aug 2, 2020 • edited Loading

GuillaumeGomez commented Aug 2, 2020

jyn514 commented Aug 2, 2020

GuillaumeGomez commented Aug 2, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

JohnCSimon commented Aug 18, 2020

jyn514 commented Aug 18, 2020

Cldfire commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

bors commented Aug 18, 2020

ollie27 commented Aug 18, 2020

GuillaumeGomez commented Aug 18, 2020

Cldfire commented Aug 19, 2020 • edited Loading

jyn514 commented Aug 19, 2020

jyn514 commented Aug 19, 2020

GuillaumeGomez commented Aug 19, 2020

Manishearth commented Aug 19, 2020

Cldfire commented Aug 26, 2020 • edited Loading

jyn514 commented Aug 26, 2020

jyn514 commented Aug 26, 2020

jyn514 commented Aug 26, 2020

Cldfire commented Aug 26, 2020

Cldfire commented Aug 26, 2020 • edited Loading

GuillaumeGomez commented Aug 27, 2020

Cldfire commented Aug 2, 2020 •

edited

Loading

Cldfire commented Aug 19, 2020 •

edited

Loading

Cldfire commented Aug 26, 2020 •

edited

Loading

Cldfire commented Aug 26, 2020 •

edited

Loading