Skip to content
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

Use details tag for trait implementors. #84320

Merged
merged 1 commit into from
Apr 24, 2021

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Apr 18, 2021

Part of #83332 and following on from #83337 and #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.

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2021
@jyn514 jyn514 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 Apr 18, 2021
@jyn514 jyn514 assigned GuillaumeGomez and unassigned jyn514 Apr 18, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

How does this interact with #82805 ? Should we merge that first?

@jsha
Copy link
Contributor Author

jsha commented Apr 18, 2021

This is independent of #82805, since we use a class name (.rustdoc-toggle) to distinguish <details> generated for rustdoc-internal reasons from those generated by the documentation author.

BTW I'm seeing errors in the rustdoc tests about xpaths not matching. Is there a command I can use to run just the rustdoc tests?

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

Is there a command I can use to run just the rustdoc tests?

Yes, x.py test src/test/rustdoc (or if you want to run all tests, not just HTML tests, x.py test src/test/rustdoc*).

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

@jsha also consider setting download-rustc = true, that cuts down on compile times quite a lot.

@rust-log-analyzer

This comment has been minimized.

@Xanewok
Copy link
Member

Xanewok commented Apr 18, 2021

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.

Would it be possible to somehow bring back the "clickable" mouse icon state when hovering, at least over the toggle, or maybe show a tiny grey highlight over the entire "clickable" line to hint the user that it's interactive?
I personally know that the line is clickable but it might not be obvious for others, especially since the § sign does show show the clickable mouse icon hint, further increasing the confusion somewhat.

(Thanks for all the work on simplifying HTML/JS parts of rustdoc!)

@jsha
Copy link
Contributor Author

jsha commented Apr 18, 2021

Would it be possible to somehow bring back the "clickable" mouse icon state when hovering

Yep! Thanks for noticing. I thought I had done that already, but you're right that it's not in this PR.

@jsha
Copy link
Contributor Author

jsha commented Apr 18, 2021

Alright, pushed a new version with tests fixed and the cursor made into a pointer.

@Nemo157
Copy link
Member

Nemo157 commented Apr 19, 2021

The toggle button is now going off the left hand side of the page on the mobile view

image

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, we should make sure the rendered version works on other platforms too

@GuillaumeGomez
Copy link
Member

Just remains to fix @Nemo157's issue and I guess we're good to go?

This switches from JS-generated toggles to using the HTML <details> tag
for expanding and collapsing entries in the "Implementors" section.
@jyn514
Copy link
Member

jyn514 commented Apr 20, 2021

r? @GuillaumeGomez

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to r=me this if Nemo's issue has been fixed

@GuillaumeGomez
Copy link
Member

Waiting for @Nemo157's confirmation then. :)

@Nemo157
Copy link
Member

Nemo157 commented Apr 20, 2021

Yep, it's fixed in the current version.

@bors
Copy link
Contributor

bors commented Apr 21, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 21, 2021
@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-apple-alt failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMTdPjYKNVxK

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536
---
The following warnings were emitted during compilation:

warning: error: Connection to server timed out

error: failed to run custom build command for `psm v0.1.11`
Caused by:
Caused by:
  process didn't exit successfully: `/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-rustc/release/build/psm-d1f698f3a1071e45/build-script-build` (exit code: 1)
  --- stdout
  cargo:rustc-cfg=asm
  cargo:rustc-cfg=switchable_stack
  OPT_LEVEL = Some("3")
  TARGET = Some("x86_64-apple-darwin")
  HOST = Some("x86_64-apple-darwin")
  CC_x86_64-apple-darwin = Some("sccache /Users/runner/work/rust/rust/clang+llvm-10.0.0-x86_64-apple-darwin/bin/clang")
  CFLAGS_x86_64-apple-darwin = Some("-ffunction-sections -fdata-sections -fPIC --target=x86_64-apple-darwin -stdlib=libc++ -fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/756679f9b2a7ac45714ffa94b48985f6e569aa7b")
  CRATE_CC_NO_DEFAULTS = None
  DEBUG = Some("false")
  running: "sccache" "/Users/runner/work/rust/rust/clang+llvm-10.0.0-x86_64-apple-darwin/bin/clang" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-stdlib=libc++" "-fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/756679f9b2a7ac45714ffa94b48985f6e569aa7b" "-xassembler-with-cpp" "-DCFG_TARGET_OS_macos" "-DCFG_TARGET_ARCH_x86_64" "-DCFG_TARGET_ENV_" "-o" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-rustc/x86_64-apple-darwin/release/build/psm-210b72712d640ca3/out/src/arch/x86_64.o" "-c" "src/arch/x86_64.s"
  cargo:warning=error: Connection to server timed out

  --- stderr



  error occurred: Command "sccache" "/Users/runner/work/rust/rust/clang+llvm-10.0.0-x86_64-apple-darwin/bin/clang" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-ffunction-sections" "-fdata-sections" "-fPIC" "--target=x86_64-apple-darwin" "-stdlib=libc++" "-fdebug-prefix-map=/Users/runner/work/rust/rust=/rustc/756679f9b2a7ac45714ffa94b48985f6e569aa7b" "-xassembler-with-cpp" "-DCFG_TARGET_OS_macos" "-DCFG_TARGET_ARCH_x86_64" "-DCFG_TARGET_ENV_" "-o" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-rustc/x86_64-apple-darwin/release/build/psm-210b72712d640ca3/out/src/arch/x86_64.o" "-c" "src/arch/x86_64.s" with args "clang" did not execute successfully (status code exit code: 2).

warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] typenum test:false 54.939
error: build failed
error: build failed
command did not execute successfully: "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0/bin/cargo" "build" "--target" "x86_64-apple-darwin" "-Zbinary-dep-depinfo" "-j" "3" "--release" "--locked" "--color" "always" "--features" "jemalloc llvm max_level_info" "--manifest-path" "/Users/runner/work/rust/rust/compiler/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /Users/runner/work/rust/rust/build/bootstrap/debug/bootstrap dist
Build completed unsuccessfully in 3:58:19

@GuillaumeGomez
Copy link
Member

@bors retry

@bors 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 21, 2021
@bors
Copy link
Contributor

bors commented Apr 21, 2021

⌛ Testing commit 569096c with merge 925cab99c4e0eb12370602bb567e962bc58c0895...

@bors
Copy link
Contributor

bors commented Apr 21, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 21, 2021
@GuillaumeGomez GuillaumeGomez removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2021
@GuillaumeGomez
Copy link
Member

Really weird...

@bors retry

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 21, 2021
@Nemo157
Copy link
Member

Nemo157 commented Apr 22, 2021

#82805 does affect this, at least on Firefox, injecting summary { display: list-item; } into the css on the linked example shows

image

but since this is probably merging first, I guess it can be fixed there.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 22, 2021
…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.
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2021
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#83990 (implement `TrustedRandomAccess` for `Take` iterator adapter)
 - rust-lang#84250 (bootstrap: use bash on illumos to run install scripts)
 - rust-lang#84320 (Use details tag for trait implementors.)
 - rust-lang#84436 (Make a few functions private)
 - rust-lang#84453 (Document From implementations for Waker and RawWaker)
 - rust-lang#84458 (Remove unnecessary fields and parameters in rustdoc)
 - rust-lang#84485 (Add some associated type bounds tests)
 - rust-lang#84489 (Mention FusedIterator case in Iterator::fuse doc)
 - rust-lang#84492 (rustdoc: Remove unnecessary dummy span)
 - rust-lang#84496 (Add some specialization tests)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b454469 into rust-lang:master Apr 24, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 24, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 24, 2021
…g, r=jsha

Clean up DOM strings

Follow-up of rust-lang#84320.

r? `@jsha`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 24, 2021
…g, r=jsha

Clean up DOM strings

Follow-up of rust-lang#84320.

r? ``@jsha``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants