Handle notable trait popup differently

[GuillaumeGomez]

POC for #91290.

So this is what I had in mind: having a notable-traits.js file in each crate and load it only when needed (I replicated what @jsha did for the search).

Currently, it's only on click but we can also add "mouseover", I didn't do it in this POC for simplicity. To handle the click, I simply changed the current code. When clicking on "i", we move and update the popup to display the notable trait information.

The advantage of this method is that we have element to care about and that's it and our HTML is finally valid based on the HTML spec.

You can try it out here.

Ah also, size of the rendering before/after:

$ cd build/x86_64-unknown-linux-gnu/doc/std
$ du -sh .
106M	.
$ du -sh .
100M	.

So not much difference.

cc @jsha @Manishearth

[rust-highfive]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

[rust-highfive]

r? @ollie27

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

[Manishearth]

I'd really like to avoid having a second mega JS file that contains stuff for all the crates combined: Can we include the info on the generated file itself? Either as data attributes on the notable popup button, or as display:none HTML elements as siblings.

Yes, the current way we generate HTML is nonconformant, but the fix to that is to make it a sibling not a child.

[jsha]

I'd really like to avoid having a second mega JS file that contains stuff for all the crates combined: Can we include the info on the generated file itself? Either as data attributes on the notable popup button, or as display:none HTML elements as siblings.

I'm in favor of the sibling approach too. To be clear: a sibling of the <summary> tag. My reasoning is that it's easier to inspect the HTML and reason about it when it's part of the DOM tree.

My proposed optimization on the issue was that we'd have a pile of such sibling at the end of each generated HTML page, and select the appropriate one. But that's probably over-optimizing - it's not clear how many pages (other than Vec) have large numbers of duplicated Notable Traits.

[GuillaumeGomez]

I'd really like to avoid having a second mega JS file that contains stuff for all the crates combined

Sorry if I wasn't clear: each file is crate specific. It's not in the root but in the crate (contrary to the search index).

Can we include the info on the generated file itself? Either as data attributes on the notable popup button, or as display:none HTML elements as siblings.

I'm not a big fan of the sibling approach because in the case it's in a <summary>, it's going to be tricky to put it into the right location (ie: not in the <summary>).

Either as data attributes on the notable popup button,

As data attribute could work. It'll just be a very big one haha.

I'm in favor of the sibling approach too. To be clear: a sibling of the

tag.

The issue with this is that it might "conflict" with the docblock element (which is the one following the item), to be confirmed. Another issue is for the implementation in rustdoc, I think it'll make a lot of code to handle it correctly.

What about simply putting it at the end of the DOM?

My proposed optimization on the issue was that we'd have a pile of such sibling at the end of each generated HTML page, and select the appropriate one. But that's probably over-optimizing - it's not clear how many pages (other than Vec) have large numbers of duplicated Notable Traits.

I think it's the best approach because it'd be much simpler to handle. Sibling with <summary> context doesn't seem like a good idea to me.

[Manishearth]

Thinking about it more, sibling of the summary tag won't work well if we have multiple of these. It's kinda unfortunate that we're putting the entire signature inside the summary tag but it makes some sense.

A way to still have HTML work would be to generate them all at the end and give them ids. Not great. I'm still not a fan of generating these at runtime since it moves more generation logic into JS, but I'm not sure if this is any better.

[GuillaumeGomez]

It can be pure HTML at the end of the current file too, doesn't have to be pure JS. But in any case, the display will need to be handled by JS unfortunately.

[Manishearth]

But in any case, the display will need to be handled by JS unfortunately.

There's a crucial distinction: If it's HTML at the end of the file, JS never needs to construct HTML, it simply needs to toggle some on-element CSS properties. It's the constructing-HTML-in-JS that I wish to avoid.

[GuillaumeGomez]

I'm fine with both approach. What do you think @jsha?

Just to sum it up:

• Storing as JS allows to save a bit of space but we then have to build some HTML.
• Storing as HTML only requires to change a CSS property to make it work (meaning much less JS).

[Manishearth]

Yep, that's an accurate summary. I'm not a fan of either approach but I have a slight preference to the HTML one from a code architecture point of view.

[camelid]

Are you absolutely sure this needs a RefCell?

[GuillaumeGomez]

For now at least it does unfortunately...

[camelid]

Ah, I didn't notice this is in the SharedContext.

[GuillaumeGomez]

I moved the DOM directly into the item's page. So now the JS only display it (in the correct location).

[rust-log-analyzer]

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

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

failures:

---- [rustdoc] rustdoc/spotlight-from-dependency.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/spotlight-from-dependency" "/checkout/src/test/rustdoc/spotlight-from-dependency.rs"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
6: @has check failed
 `XPATH PATTERN` did not match
 // @has - '//div[@id="method.new"]//span[@class="notable-traits"]//code/span' 'impl Iterator for Odd'
Encountered 1 errors

------------------------------------------

---
test result: FAILED. 485 passed; 1 failed; 4 ignored; 0 measured; 0 filtered out; finished in 51.82s



command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--rustdoc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" "--src-base" "/checkout/src/test/rustdoc" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "rustdoc" "--mode" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-12/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "12.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:17:52

[bors]

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

[Dylan-DPC]

Closing this after a discussion with the author