Open impl blocks by default #84552

GuillaumeGomez · 2021-04-25T12:54:39Z

As you can see on https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html, impl blocks are currently not open by default whereas they should.

I also realized that a test was outdated so I removed it and opened #84550 because it seems like the rustdoc-gui test suite isn't run on CI...

cc @jyn514
r? @jsha

rust-highfive · 2021-04-25T12:54:42Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

jyn514 · 2021-04-25T13:48:39Z

src/test/rustdoc-gui/impl-default-expansion.goml

+// This test ensures that the impl blocks are open by default.
+goto: file://|DOC_PATH|/index.html
+wait-for: 5000
+assert: ("#main > details.implementors-toggle", "open", "")


Can you add a test that intentionally fails to make sure this runs at all?

They are not run, that's for sure. Otherwise, it'd have failed a long time ago because of nojs-attr-pos.goml.

Ok. In the meantime, can you run it locally and make sure it passes?

This version didn't because I forgot to commit the one with the correct URL (struct.Foo.html and not index.html). But otherwise it works as expected.

Can you fix it to have the correct URL? I don't see the point of adding a test we know will fail.

While you're at it, can you update the other test to pass instead of removing it altogether?

Look at my comment below.

While you're at it, can you update the other test to pass instead of removing it altogether?

The test which fails checked for attributes toggles. Since they don't have toggles anymore, there is no point of keeping the test.

The test which fails checked for attributes toggles. Since they don't have toggles anymore, there is no point of keeping the test.

Can you instead test that it doesn't have toggles?

Would it be useful to check that something was removed? For example, if we put back toggles on attributes with a different DOM, the test would still be happy. Which is why it's simpler to enforce what exists over what doesn't. (Not sure if that makes sense said like this ><)

I agree with @GuillaumeGomez. When removing "code that does X," it's often tempting to add a test for "doesn't do X anymore." But the problem is there are a zillion ways it could wind up doing X again, and it's impossible to cover all of them (or even a reasonable fraction). It winds up adding to the test running time, and the burden of maintaining tests, without a corresponding benefit.

GuillaumeGomez · 2021-04-25T13:51:03Z

Also, I need to make some improvements on the test I added, so please don't r+ right now. :)

…panded"

GuillaumeGomez · 2021-04-25T14:07:31Z

I also added a fix for the expansion of the "blocks" based on the URL hash.

jsha · 2021-04-25T16:20:45Z

src/librustdoc/html/static/main.js

+            } else if (e.tagName === "DETAILS") {
+                e.open = true;


As I understand it, we hide things during load because we're going to manipulate their DOM, and DOM manipulation on hidden things goes faster. The main DOM manipulation is adding the [+] toggles. When we fully switch over to <details> we won't need to hide portions of the DOM during load anymore.

I'd like to start doing that as we go. Rather than opening the <details> tags on load, we should set them to the global default in our generated HTML, and on load we should hide or show them according to settings.

Not all details should be opened by details. For example the traits implementations blocks.

Or did you mean that we should generate the <details> with "open" directly? Still: we will need this JS in any case for when we press the global toggle.

Yes, I mean that for default-shown sections we should generate <details open>.

I agree we need JS to make <details> respond to the global toggle, but I believe that's already in place. And it wouldn't be in this chunk of code, which executes on load rather than in response to the global toggle.

You're right, I'll modify it then.

camelid · 2021-04-25T18:45:01Z

I just opened an issue about this (#84558), but it looks like this should fix it! Do you want to mark this PR as fixing that issue?

GuillaumeGomez · 2021-04-25T20:39:08Z

@camelid I updated the PR message. :)

…t instead of opening them with JS

GuillaumeGomez · 2021-04-26T11:45:32Z

@jsha Updated!

jsha

@bors r+

GuillaumeGomez · 2021-04-26T15:22:58Z

Not sure it works as review comment.

@bors: r=jsha

bors · 2021-04-26T15:22:59Z

📌 Commit 6c8969c has been approved by jsha

bors · 2021-04-27T01:58:33Z

⌛ Testing commit 6c8969c with merge 6eb956f...

bors · 2021-04-27T04:46:40Z

☀️ Test successful - checks-actions
Approved by: jsha
Pushing 6eb956f to master...

lf- · 2021-05-03T02:23:21Z

For anyone wondering what's going on with the nightly preventing getting this patch, that's tracked here:
#84538

GuillaumeGomez · 2021-05-03T08:26:39Z

You can get the last rustdoc version if you remove rustfmt from your components (not ideal but it works).

Open all impl blocks by default

33a7098

GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: rustdoc UI (generated HTML) A-rustdoc-js Area: Rustdoc's front-end labels Apr 25, 2021

rust-highfive assigned jsha Apr 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2021

jyn514 reviewed Apr 25, 2021

View reviewed changes

GuillaumeGomez added 4 commits April 25, 2021 15:54

Add test to ensure that impl blocks are open by default

bcad1ec

Remove unneeded rustdoc-gui test because DOM changed

175cd9b

Fix expansion for item pointed to by the URL hash

1afea79

Add test to ensure that the element pointed to by the URL hash is "ex…

9b44c4b

…panded"

GuillaumeGomez force-pushed the open-impl-blocks-by-default branch from 49ba0ff to 9b44c4b Compare April 25, 2021 14:06

jsha requested changes Apr 25, 2021

View reviewed changes

Add open attribute on details which are supposed to be open by defaul…

6c8969c

…t instead of opening them with JS

GuillaumeGomez mentioned this pull request Apr 26, 2021

Enforce rustdoc-gui test-suite run #84586

Merged

jsha approved these changes Apr 26, 2021

View reviewed changes

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 Apr 26, 2021

ehuss mentioned this pull request Apr 26, 2021

nightly docs missing methods #84596

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 27, 2021

bors merged commit 6eb956f into rust-lang:master Apr 27, 2021

rustbot added this to the 1.53.0 milestone Apr 27, 2021

bors mentioned this pull request Apr 27, 2021

Add new tool to check HTML #84480

Closed

GuillaumeGomez deleted the open-impl-blocks-by-default branch April 27, 2021 08:30

This was referenced Apr 30, 2021

Sidebar links don't open collapsed sections rust-lang/docs.rs#1385

Closed

rustdoc: implementations are starting contracted #84798

Closed

lf- mentioned this pull request May 11, 2021

Crates with docs built by the broken 2021-04-24 nightly should be rebuilt rust-lang/docs.rs#1399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open impl blocks by default #84552

Open impl blocks by default #84552

GuillaumeGomez commented Apr 25, 2021 •

edited

rust-highfive commented Apr 25, 2021

jyn514 Apr 25, 2021

GuillaumeGomez Apr 25, 2021

jyn514 Apr 25, 2021

GuillaumeGomez Apr 25, 2021

jyn514 Apr 25, 2021 •

edited

GuillaumeGomez Apr 25, 2021

GuillaumeGomez Apr 25, 2021

jyn514 Apr 25, 2021

GuillaumeGomez Apr 25, 2021

jsha Apr 25, 2021

GuillaumeGomez commented Apr 25, 2021

GuillaumeGomez commented Apr 25, 2021

jsha Apr 25, 2021

GuillaumeGomez Apr 25, 2021

GuillaumeGomez Apr 25, 2021

jsha Apr 25, 2021

GuillaumeGomez Apr 26, 2021

camelid commented Apr 25, 2021

GuillaumeGomez commented Apr 25, 2021

GuillaumeGomez commented Apr 26, 2021

jsha left a comment

GuillaumeGomez commented Apr 26, 2021

bors commented Apr 26, 2021

bors commented Apr 27, 2021

bors commented Apr 27, 2021

lf- commented May 3, 2021

GuillaumeGomez commented May 3, 2021

Open impl blocks by default #84552

Open impl blocks by default #84552

Conversation

GuillaumeGomez commented Apr 25, 2021 • edited

rust-highfive commented Apr 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Apr 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 25, 2021

GuillaumeGomez commented Apr 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Apr 25, 2021

GuillaumeGomez commented Apr 25, 2021

GuillaumeGomez commented Apr 26, 2021

jsha left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 26, 2021

bors commented Apr 26, 2021

bors commented Apr 27, 2021

bors commented Apr 27, 2021

lf- commented May 3, 2021

GuillaumeGomez commented May 3, 2021

GuillaumeGomez commented Apr 25, 2021 •

edited

jyn514 Apr 25, 2021 •

edited