-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Move scripts
on the rustdoc template into head
and apply the defer
attribute
#90872
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
Move scripts
on the rustdoc template into head
and apply the defer
attribute
#90872
Conversation
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
r? @jsha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, thanks for picking this up! One small change to request.
@@ -13,13 +13,23 @@ | |||
href="{{static_root_path | safe}}rustdoc{{page.resource_suffix}}.css" {# -#} | |||
id="mainThemeStyle"> {#- -#} | |||
{{- style_files | safe -}} | |||
<script id="default-settings" {# -#} | |||
<script defer id="default-settings" {# -#} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's omit the defer
here. This doesn't actually load any external JS, and is just a convenient place to hang some data- attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and there's even a citation from MDN: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-defer
defer
Warning: This attribute must not be used if the src attribute is absent (i.e. for inline scripts), in this case it would have no effect.
<script src="{{static_root_path | safe}}storage{{page.resource_suffix}}.js"></script> {#- -#} | ||
<script src="{{page.root_path | safe}}crates{{page.resource_suffix}}.js"></script> {#- -#} | ||
<script defer src="{{static_root_path | safe}}storage{{page.resource_suffix}}.js"></script> {#- -#} | ||
<script defer src="{{page.root_path | safe}}crates{{page.resource_suffix}}.js"></script> {#- -#} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, storage and crates JS should not get the defer attribute. They are both on the critical path for rendering the page, so we actually want them to block page render, rather than cause a re-style later.
As an example of why this is important, try changing to a dark theme, and then reloading the page. If storage has the defer attribute, you'll see the page first load with light styles and then with dark (assuming your system default theme is light).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsha
Thank you so much for your extremely clear review! I've made changes as you show :)
@bors r+ Thanks @ken-matsui ! |
📌 Commit 88787a3 has been approved by |
…plate, r=jsha Move `scripts` on the rustdoc template into `head` and apply the `defer` attribute Closes rust-lang#90719
…plate, r=jsha Move `scripts` on the rustdoc template into `head` and apply the `defer` attribute Closes rust-lang#90719
☀️ Test successful - checks-actions |
Finished benchmarking commit (80f5f60): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Closes #90719