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

Simplify foreign type rendering. #56874

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
7 participants
@JohnHeitmann
Copy link
Contributor

JohnHeitmann commented Dec 16, 2018

Simplified foreign type rendering by switching from tables to flexbox. Also, removed some seemingly extraneous elements like “ghost” spans.

Reduces element count on the std::iter::Iterator page by 30%. On my laptop it drops Iterator page load time from ~15s to ~10s. Frame times during scrolling are a hair lower too.

Known visual changes (happy to tweak based on feedback):

  • The main impl ... headers are now getting the default, larger, h3 font size. This was an accident, but I liked how it turned out so I didn't fix it.
  • There's a hair less vertical spacing between the end of a where block and the start of the next fn. Now, all spacing is consistent. I think this looks a bit worse. I may tweak vertical spacing more here or in a follow-up that cleans up vertical spacing more broadly.
  • "[src]" links are all sized at 17px. A few were 19px in the original.

I haven't yet done heavy cross-browser or cross-crate testing. I was hoping to get a quick thumbs up or thumbs down here at this first draft, then if this is on the right track I'll spend some time on that testing.

TODO:

  • Test on Chrome
  • Test on Firefox
  • Test on UC Android
  • Test on Edge
  • Test on iOS safari
  • Test on desktop safari
  • Update automated tests
  • Increase vertical margin
  • Fix "Important traits for" hover overlap
  • Wait for #55798 to land & merge it
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 16, 2018

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 16, 2018

r? @QuietMisdreavus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 16, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0d001268:start=1544961011779114112,finish=1544961067050664337,duration=55271550225
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:51:17] 
[00:51:17] running 121 tests
[00:51:20] i..ii...iii..iiii.....i...i..........i..iii.............i.....i......ii...i..i.ii..............i...i 100/121
[00:51:20] i..ii..i....iiii.....
[00:51:20] 
[00:51:20]  finished in 3.384
[00:51:20] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:51:34] 
[00:51:34] running 119 tests
[00:51:56] .iiiii...i.....i..i...i..i.i..i.ii..i.....i..i....i..........iiii.........i.i...ii...i.......ii.i.i. 100/119
[00:52:00] i......iii.i.....ii
[00:52:00] 
[00:52:00]  finished in 25.812
[00:52:00] travis_fold:end:test_debuginfo

---
travis_time:start:test_rustdoc
Check compiletest suite=rustdoc mode=rustdoc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:56:50] 
[00:56:50] running 281 tests
[00:57:57] .F..............F......i............................................................................ 100/281
[00:58:52] .................................i.F........................................................F....F.F 200/281
[00:59:37] stdout:
[00:59:37] ------------------------------------------
[00:59:37] 
[00:59:37] ------------------------------------------
[00:59:37] ------------------------------------------
[00:59:37] stderr:
[00:59:37] ------------------------------------------
[00:59:37] 21: @has check failed
[00:59:37]  `XPATH PATTERN` did not match
[00:59:37]  // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]//*/code' "impl<'c, K> Send for Foo<'c, K> where K: for<'b> Fn(&'b bool) -> &'c u8, 'c: 'static"
[00:59:37] 24: @has check failed
[00:59:37]  `XPATH PATTERN` did not match
[00:59:37]  // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]//*/code' "impl<'c, K> Sync for Foo<'c, K> where K: Sync"
[00:59:37] Encountered 2 errors
[00:59:37] 
[00:59:37] ------------------------------------------
[00:59:37] 
[00:59:37] 
[00:59:37] thread '[rustdoc] rustdoc/synthetic_auto/lifetimes.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3252:9
[00:59:37] 
[00:59:37] ---- [rustdoc] rustdoc/synthetic_auto/manual.rs stdout ----
[00:59:37] 
[00:59:37] error: htmldocck failed!
[00:59:37] status: exit code: 1
[00:59:37] command: "/usr/bin/python2.7" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/synthetic_auto/manual" "/checkout/src/test/rustdoc/synthetic_auto/manual.rs"
[00:59:37] ------------------------------------------
[00:59:37] 
[00:59:37] ------------------------------------------
[00:59:37] stderr:
[00:59:37] stderr:
[00:59:37] ------------------------------------------
[00:59:37] 12: @has check failed
[00:59:37]  `XPATH PATTERN` did noout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:59:37] 
[00:59:37] 
[00:59:37] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:59:37] Build completed unsuccessfully in 0:18:29
[00:59:37] Build completed unsuccessfully in 0:18:29
[00:59:37] make: *** [check] Error 1
[00:59:37] Makefile:58: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0010b3a6
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Dec 16 12:50:53 UTC 2018

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 16, 2018

Reducing DOM size seems like a good idea. Please fix the tests too before we start the review. :)

@JohnHeitmann JohnHeitmann force-pushed the JohnHeitmann:docs-spacing branch from fd377b4 to 0384f58 Dec 17, 2018

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 17, 2018

Other than UC Android testing (I'm still working on getting a working emulator), I'm now happy with this. Tests pass, there's a bit more vertical whitespace, and I caught a bug with the z index of tooltips.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 17, 2018

The big test is wether it works on iOS considering it's the worst supported browser.

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 17, 2018

Yup, works as well on iOS as the old.

I'm giving up on UC Browser unless someone can toss me a guide on how to get it going in an emulator. I've tried loading it in many flavors of Andriod with no luck. The closest I got was Kit Kat, where it was able to install, but crash on launch.

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 17, 2018

Argh, I'm sorry, I just saw "[src]" is wrapping incorrectly on the std/iter/struct.Chain.html page. Let me track that down...

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 17, 2018

Those glitches are now fixed.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 18, 2018

Ok, code seems good. Just one thing though: do you mind waiting for #55798 to be merged first please? It's been around a long time and it'll affect the same code. Very sorry about that... :-/

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 18, 2018

No problem!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 18, 2018

Thanks!

@QuietMisdreavus
Copy link
Member

QuietMisdreavus left a comment

Thanks so much for this! I've got just one comment of my own, and like @GuillaumeGomez, we should wait for #55798 to merge (it's currently being tested in the build queue, shouldn't take long now) to land first. Thanks for being so thorough with testing, too!

By chance, do you have access to a copy of Internet Explorer? I know we don't officially support it, but i'd like to see whether this breaks that too much.

}
clean::AssociatedTypeItem(ref bounds, ref default) => {
let id = cx.derive_id(format!("{}.{}", item_type, name));
let ns_id = cx.derive_id(format!("{}.{}", name, item_type.name_space()));
write!(w, "<h4 id='{}' class=\"{}{}\">", id, item_type, extra_class)?;
write!(w, "<span id='{}' class='invisible'><code>", ns_id)?;
write!(w, "<code> id='{}'", ns_id)?;

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Dec 20, 2018

Member

This seems to have the > for the <code> tag in the wrong spot?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 20, 2018

@QuietMisdreavus Once my PR is merged, I'll check how this one goes on iOS too (I think safari might allow that...).

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2018

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

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 22, 2018

Update: finished the main part of the merge plus some follow-up bug hunting. The biggest change is that I found js was changing some display to block that should have been flex. Switching the js to change all display to flex had two minor knock-on effects: 1) Flex doesn't collapse margins, so I had to ease in a margin or two. 2) Added flex-basis: 100% in a few spots to get full-width drawing back.

I made enough changes that I want to re-test on various browsers again. I'll have that done in ~24 hours.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2018

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

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 26, 2018

Quick update: iOS-- and perhaps mobile Chrome-- are having troubles with my latest changes after the merge. I'm working through those issues.

@JohnHeitmann JohnHeitmann force-pushed the JohnHeitmann:docs-spacing branch from 0bc7f10 to f292e0d Dec 28, 2018

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Dec 28, 2018

Merged associated const changes, fixed some quirks, and cleaned up the commits (next time I'll keep my powder dry longer).

Aside from the associated const merge, the other change here is the main.js update to change some display to "block" and some to "flex" once loading is complete. The better way to do this in general would be to remove a css class, but my first naive pass at that broke in noscript. I'll try to clean that up in a follow-up. I didn't want to drag this out longer.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 28, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0f6de56a:start=1545960519757192942,finish=1545960599037553785,duration=79280360843
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:09:31] 
[01:09:31] running 118 tests
[01:09:55] .iiiii...i.....i..i...i..i.i..i.ii..i.....i..i....i..........iiii..........i...ii...i.......ii.i.i.i 100/118
[01:09:59] ......iii.i.....ii
[01:09:59] 
[01:09:59]  finished in 29.037
[01:09:59] travis_fold:end:test_debuginfo

---
[01:15:20] 
[01:15:20] running 287 tests
[01:16:28] ..........................i......................................................................... 100/287
[01:17:26] .....................................i.............................................................. 200/287
16] 30: @has check failed
[01:18:16]  `XPATH PATTERN` did not match
[01:18:16]  // @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]//*/code' "impl<'a> Send for Parser<'a>"
[01:18:16] Encountered 1 errors
[01:18:16] 
[01:18:16] ------------------------------------------
[01:18:16] 
---
[01:18:16] 
[01:18:16] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:495:22
[01:18:16] 
[01:18:16] 
[01:18:16] 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" "--mode" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/ch

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Simplify foreign type rendering.
Simplified foreign type rendering by switching from tables to flexbox. Also, removed some seemingly extraneous elements like “ghost” spans.

Reduces element count on std::iter::Iterator by 30%.

@JohnHeitmann JohnHeitmann force-pushed the JohnHeitmann:docs-spacing branch from f292e0d to 34bd2b8 Dec 28, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 3, 2019

Seems really cool! I'll give it a try locally in the next day before r+ing it. Thanks a lot!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 10, 2019

Damn, didn't take a look at it yet. >< I'll try to do it tomorrow.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 10, 2019

Ok checked. The output is good, therefore we can move forward. Thanks!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

📌 Commit 34bd2b8 has been approved by GuillaumeGomez

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

⌛️ Testing commit 34bd2b8 with merge b1e6fa6...

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #56874 - JohnHeitmann:docs-spacing, r=GuillaumeGomez
Simplify foreign type rendering.

Simplified foreign type rendering by switching from tables to flexbox. Also, removed some seemingly extraneous elements like “ghost” spans.

Reduces element count on the `std::iter::Iterator` page by 30%. On my laptop it drops Iterator page load time from ~15s to ~10s. Frame times during scrolling are a hair lower too.

Known visual changes (happy to tweak based on feedback):
* The main `impl ...` headers are now getting the default, larger, h3 font size. This was an accident, but I liked how it turned out so I didn't fix it.
* There's a hair less vertical spacing between the end of a where block and the start of the next fn. Now, all spacing is consistent. I think this looks a bit worse. I may tweak vertical spacing more here or in a follow-up that cleans up vertical spacing more broadly.
* "[src]" links are all sized at 17px. A few were 19px in the original.

I haven't yet done heavy cross-browser or cross-crate testing. I was hoping to get a quick thumbs up or thumbs down here at this first draft, then if this is on the right track I'll spend some time on that testing.

TODO:

- [x] Test on Chrome
- [x] Test on Firefox
- [ ] ~~Test on UC Android~~
- [x] Test on Edge
- [x] Test on iOS safari
- [x] Test on desktop safari
- [x] Update automated tests
- [x] Increase vertical margin
- [x] Fix "Important traits for" hover overlap
- [x] Wait for #55798 to land & merge it
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 12, 2019

The job dist-x86_64-apple of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:03:17]       Memory: 8 GB
[00:03:17]       Boot ROM Version: VMW71.00V.0.B64.1704110547
[00:03:17]       Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
[00:03:17]       SMC Version (system): 2.8f0
[00:03:17]       Serial Number (system): VMGj7F5vtHLz
[00:03:17] 
[00:03:17] hw.ncpu: 4
[00:03:17] hw.byteorder: 1234
[00:03:17] hw.memsize: 8589934592
---
[02:49:11] travis_time:end:stage2-cargo-miri:start=1547323791942555000,finish=1547323792550490000,duration=607935000

[02:49:11] [TIMING] ToolBuild { compiler: Compiler { stage: 2, host: "x86_64-apple-darwin" }, target: "x86_64-apple-darwin", tool: "cargo-miri", path: "src/tools/miri", mode: ToolRustc, is_optional_tool: true, source_type: Submodule, extra_features: [] } -- 0.640
[02:49:12] [TIMING] Miri { stage: 2, target: "x86_64-apple-darwin" } -- 1.478
The job exceeded the maximum time limit for jobs, and has been terminated.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Jan 13, 2019

@bors retry

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#56874 - JohnHeitmann:docs-spacing, r=Guilla…
…umeGomez

Simplify foreign type rendering.

Simplified foreign type rendering by switching from tables to flexbox. Also, removed some seemingly extraneous elements like “ghost” spans.

Reduces element count on the `std::iter::Iterator` page by 30%. On my laptop it drops Iterator page load time from ~15s to ~10s. Frame times during scrolling are a hair lower too.

Known visual changes (happy to tweak based on feedback):
* The main `impl ...` headers are now getting the default, larger, h3 font size. This was an accident, but I liked how it turned out so I didn't fix it.
* There's a hair less vertical spacing between the end of a where block and the start of the next fn. Now, all spacing is consistent. I think this looks a bit worse. I may tweak vertical spacing more here or in a follow-up that cleans up vertical spacing more broadly.
* "[src]" links are all sized at 17px. A few were 19px in the original.

I haven't yet done heavy cross-browser or cross-crate testing. I was hoping to get a quick thumbs up or thumbs down here at this first draft, then if this is on the right track I'll spend some time on that testing.

TODO:

- [x] Test on Chrome
- [x] Test on Firefox
- [ ] ~~Test on UC Android~~
- [x] Test on Edge
- [x] Test on iOS safari
- [x] Test on desktop safari
- [x] Update automated tests
- [x] Increase vertical margin
- [x] Fix "Important traits for" hover overlap
- [x] Wait for rust-lang#55798 to land & merge it

bors added a commit that referenced this pull request Jan 13, 2019

Auto merge of #57580 - Centril:rollup, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #56874 (Simplify foreign type rendering.)
 - #57113 (Move diagnostics out from QueryJob and optimize for the case with no diagnostics)
 - #57366 (Point at match discriminant on type error in match arm pattern)
 - #57538 (librustc_mir: Fix ICE with slice patterns)

Failed merges:

 - #57381 (Tweak output of type mismatch between "then" and `else` `if` arms)

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

⌛️ Testing commit 34bd2b8 with merge 9aee7ed...

@bors bors merged commit 34bd2b8 into rust-lang:master Jan 14, 2019

1 of 2 checks passed

homu Testing commit 34bd2b845b3acd84c5a9bddae3ff8081c19ec5e9 with merge 9aee7ed335cfcaad33a6f6492497588b3d421b2d...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ricochet1k

This comment has been minimized.

Copy link
Contributor

ricochet1k commented on src/librustdoc/html/static/rustdoc.css in 34bd2b8 Jan 18, 2019

This line change breaks links to types in function signatures and makes the function signature disappear when clicking on links on in the left navigation. Does anyone else see this, or is it just me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment