[WIP] Start transforming documentation toggles into pure css ones by GuillaumeGomez · Pull Request #83355 · rust-lang/rust

GuillaumeGomez · 2021-03-21T21:41:49Z

This is a first draft of #83332. I precise: this is a work in progress. However, with this we can already see what it'll look like after. I precise: even if incomplete, this is working.

For now, this PR switched the doc blocks toggles for the following parts:

item doc block (the one at the top)
methods doc blocks

Some explanations on how it works: I used a trick based on checkbox. I hide the checkbox and only print its label, so when you click on the label, you check/uncheck the checkbox. Now, that also brings some downsides too:

Positioning the label would be tricky without a parent so I had to add a div to wrap the function and its documentation. However, I see that as a positive point because we're finally "merging" the items and their documentation.
It makes the DOM heavier because we have to add a div, a label and an input for each item with documentation.

The advantages now:

It'll be much simpler to handle showing/hiding documentation since it'll be handled 100% with CSS. No more JS for this (except for toggling all toggles at once, this one will remain).
A lot less of weird JS that we had.
No more DOM manipulation when we arrive on a page to add the toggles (meaning better performance!).
It'll finally be possible to hide/show documentation even without JS.

There are some small display differences too because currently we use width on the -/+ sign to make the whole a big larger.

Before:

After:

If this seems to be the right way to go, I'll clean up this PR (and finish to remove the remaining JS toggles) so we can finally have toggles 100% non-JS. \o/

r? @Manishearth

rust-highfive · 2021-03-21T21:41:51Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

Manishearth · 2021-03-21T21:44:48Z

Note this might conflict with #83337, might want to be careful on timing?

GuillaumeGomez · 2021-03-21T21:49:29Z

I didn't touch anything on the item's display because of your PR (which I still need to review). So we should be fine. ;)

rust-log-analyzer · 2021-03-21T21:53:37Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 435 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/src/librustdoc/html/static/rustdoc.css: missing trailing newline
some tidy checks failed


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/tidy
Build completed unsuccessfully in 0:00:12

camelid · 2021-03-21T22:03:58Z

src/librustdoc/html/format.rs

 crate struct Buffer {
    for_html: bool,
    buffer: String,
+    crate label_id_count: usize,


Buffer seems like the wrong place to put this field. Maybe it should be on Context instead?

Good point.

camelid · 2021-03-21T22:04:49Z

src/librustdoc/html/static/main.js

        }
        if (!otherMessage) {
-            span.innerHTML = "&nbsp;Expand&nbsp;description";
+           return null;


nit: This is 3-space indent, it should be 4.

the8472 · 2021-03-21T22:27:02Z

Have you tried using <summary><details> instead? It semantically matches and also provides interactivity without JS.

Manishearth · 2021-03-22T05:42:33Z

Yeah, note that with details you can also select on details[open] to custom style the expansion, so we could make it display different text for the show/hide summary bits (have two parts of the summary tag)

Manishearth · 2021-03-22T05:43:39Z

I didn't touch anything on the item's display because of your PR (which I still need to review). So we should be fine. ;)

It does conflict a little but but that's fine

GuillaumeGomez · 2021-03-22T10:27:57Z

Oh nice, I didn't know about <summary>! I'm gonna give it a try then.

bors · 2021-03-26T01:18:30Z

☔ The latest upstream changes (presumably #82873) made this pull request unmergeable. Please resolve the merge conflicts.

jsha · 2021-03-26T07:03:39Z

@GuillaumeGomez I actually started on a <details><summary>... approach to this a while ago and got quite far. You can see it at:

master...jsha:details-toggle
https://jacob.hoffman-andrews.com/rust/details-toggle/std/string/struct.String.html

You are very welcome to pick up any of that code if you'd like. I didn't wind up finishing it because my original motivation was to improve performance, and some performance test tools actually showed a regression in render performance. But given that you'd like to clean up the toggle-related JS anyhow, it seems like a good fit.

jsha · 2021-03-26T07:16:59Z

By the way, a nice advantage of the <details><summary> approach is that it fixes a significant source of layout shift during load: #83075

jsha · 2021-03-27T22:17:43Z

I incorporated the <details> <summary> idea into Manishearth#1. Check it out!

Manishearth · 2021-03-28T01:17:56Z

@GuillaumeGomez given that @jsha started a bunch of the work on this in #83337, perhaps we should close this PR for now, wait for that to land, and build improvements on top of that?

GuillaumeGomez · 2021-03-28T10:01:03Z

Sure!

…earth,Nemo157,GuillaumeGomez Use details tag for trait implementors. Part of rust-lang#83332 and following on from rust-lang#83337 and rust-lang#83355. This removes one category of JS-generated toggles (implementors), and replaces them with a `<details>` tag. This simplifies the JS, and fixes some bugs where things that were supposed to be hidden by the toggle were not hidden. Compare https://hoffman-andrews.com/rust/details-implementors/std/io/trait.Read.html#impl-Read vs https://doc.rust-lang.org/nightly/std/io/trait.Read.html#implementors. This introduces a `left: -23px` to put the toggle in the correct place, matching the current style for `.collapse-toggle`. It's worth noting this introduces a slight behavior change: since the entire line is now a `<summary>`, any part of the line is clickable. So for instance, in `impl Read for File`, clicking `impl` or `for` will collapse / expand the docs. Clicking `Read` or `File` still links to the appropriate documentation as before.

Start transforming documentation toggles into pure css ones

3ab7bdb

rust-highfive assigned Manishearth Mar 21, 2021

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

camelid added A-rustdoc-ui Area: Rustdoc UI (generated HTML) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 21, 2021

camelid reviewed Mar 21, 2021

View reviewed changes

jsha mentioned this pull request Mar 27, 2021

rustdoc: Hide item contents, not items #83337

Merged

GuillaumeGomez closed this Mar 28, 2021

GuillaumeGomez deleted the pure-css-toggles branch March 28, 2021 10:01

This was referenced Apr 18, 2021

Use details tag for trait implementors. #84320

Merged

Clean up toggle logic in rustdoc #83332

Closed

Uh oh!

Conversation

GuillaumeGomez commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 21, 2021

Uh oh!

Manishearth commented Mar 21, 2021

Uh oh!

GuillaumeGomez commented Mar 21, 2021

Uh oh!

rust-log-analyzer commented Mar 21, 2021

Uh oh!

camelid Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

camelid Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

the8472 commented Mar 21, 2021

Uh oh!

Manishearth commented Mar 22, 2021

Uh oh!

Manishearth commented Mar 22, 2021

Uh oh!

GuillaumeGomez commented Mar 22, 2021

Uh oh!

bors commented Mar 26, 2021

Uh oh!

jsha commented Mar 26, 2021

Uh oh!

jsha commented Mar 26, 2021

Uh oh!

jsha commented Mar 27, 2021

Uh oh!

Manishearth commented Mar 28, 2021

Uh oh!

GuillaumeGomez commented Mar 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

GuillaumeGomez commented Mar 21, 2021 •

edited

Loading