rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a` #103650

notriddle · 2022-10-27T18:25:56Z

Example: https://notriddle.com/notriddle-rustdoc-demos/line-anchors/test_dingus/fn.test.html

This allows people to treat them like real links, such as right-click to copy URL, and makes the line numbers in a scraped example work at all, when before this commit was added, they had the clickable pointer cursor but did not actually do anything when clicked.

rustbot · 2022-10-27T18:26:00Z

A change occurred in the Ayu theme.

cc @Cldfire

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

GuillaumeGomez · 2022-10-28T09:03:24Z

With this change, when another line is selected, the original one keeps the right border:

Just to explain a bit more what I did:

Click on a example line number.
When on the source code page, click on another line.
The border of the "original" line is still orange.

When switched back to <span>, it doesn't keep it, so I suppose it's a new bug. Once fixed, can you add a test to ensure that it doesn't happen please?

GuillaumeGomez · 2022-10-28T17:56:11Z

src/librustdoc/html/static/js/source-script.js

@@ -211,6 +216,12 @@ const handleSourceHighlight = (function() {
    };
 }());

+const addSourceLineAnchorsHref = () => {
+    onEachLazy(document.querySelectorAll(".src-line-numbers a"), e => {


This is unneeded. If there is an ID, it automatically links to itself if there is no href.

Is it? When I try running with this function patched out, clicking the links still works, but right-clicking them doesn't.

What is the right clicking supposed to do?

It's supposed to let me copy a link to the clipboard, or open it in a new tab.

Oh indeed. It doesn't propose to copy a link, what a shame... Then I guess it comes to this: is it better to generate href through JS or in the HTML directly?

I guess it really depends on how much bigger it makes the doc bundle. If it's only 1% or so, it's probably fine to just use HTML. If it's more like 10%, it should probably be JavaScript, or skip this PR entirely.

Let's see once we have results to think about. ;)

GuillaumeGomez · 2022-10-28T17:58:57Z

src/librustdoc/html/static/js/scrape-examples.js

+
+        const addSourceLineAnchorsHref = () => {
+            onEachLazy(document.querySelectorAll(".src-line-numbers a"), e => {
+                e.href = linkPage + "#" + e.innerText;


On small pages, it's fine. However, it's a very bad ideas as soon as you start to have more than 5 functions with examples which comes pretty quickly. Either generate directly the URLs (which would increase the size) or don't change it at all.

That makes sense.

Okay, I've changed it to just embed the links directly in the outputted HTML.

Big question: is it worth it? It'll increase the page size quite a lot.

notriddle · 2022-10-29T13:40:22Z

Let’s see how much, I suppose.

@bors try @rust-timer queue

rust-timer · 2022-10-29T13:40:24Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-29T13:40:31Z

⌛ Trying commit ec5181653e19477b01f2f1eb60db969c3598b36d with merge c02fcd308362e1b1a08a8c1ba40eaffb84e1d23f...

bors · 2022-10-29T15:59:08Z

☀️ Try build successful - checks-actions
Build commit: c02fcd308362e1b1a08a8c1ba40eaffb84e1d23f (c02fcd308362e1b1a08a8c1ba40eaffb84e1d23f)

rust-timer · 2022-10-29T15:59:10Z

Queued c02fcd308362e1b1a08a8c1ba40eaffb84e1d23f with parent 33b530e, future comparison URL.

rust-timer · 2022-10-29T17:19:12Z

Finished benchmarking commit (c02fcd308362e1b1a08a8c1ba40eaffb84e1d23f): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.4%, 1.4%]	3
Regressions ❌ (secondary)	1.8%	[0.4%, 4.1%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[0.4%, 1.4%]	3

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.1%, 2.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

notriddle · 2022-10-29T17:26:30Z

doc-bytes results: https://perf.rust-lang.org/compare.html?start=33b530e04099465a8029ef581202d52f4075558e&end=c02fcd308362e1b1a08a8c1ba40eaffb84e1d23f&stat=size%3Adoc_bytes

	mean	range	count
Regressions ❌ (primary)	1.66%	[0.23%, 4.14%]	17
Regressions ❌ (secondary)	1.05%	[0.01%, 6.75%]	22
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.66%	[0.23%, 4.14%]	17

…e-css, r=notriddle Fix z-indexes of code example feature and cleanup its CSS When reviewing rust-lang#103650, I realized that the `z-index`es of this feature were completely broken: ![Screenshot from 2022-10-28 10-55-27](https://user-images.githubusercontent.com/3050060/198826360-0c5cbe5a-ea8e-452a-9504-38d3da3615e6.png) This PR fixes it by reducing the value of value under the one used for `.popover` (it could be completely removed but then it wouldn't be displayed as nicely). There was also a lot of duplicated CSS so I merged the rules. r? `@notriddle`

GuillaumeGomez · 2022-10-30T10:53:03Z

I think it's acceptable. r=me once you have fixed the merge conflict.

notriddle · 2022-10-31T20:30:13Z

@GuillaumeGomez Okay, it's rebased.

GuillaumeGomez · 2022-10-31T20:41:09Z

src/librustdoc/html/sources.rs

-                writeln!(line_numbers, "<span>{0}</span>", line + offset)
+            for line_number in 1..=lines {
+                let line = line_number + offset;
+                writeln!(line_numbers, "<a href=\"{root_path}{url}#{line}\">{line}</a>")


This one is still bugging me: as soon as this feature is enabled by default, the size will increase like crazy... I'm really not sure it's worth it to generate links here.

Would generating it with JavaScript be better?

No because then it would become a performance problem on the front-end side. Maybe I'm just overthinking things in here... The problem is that this feature has basically no test or anything, so it's really not great.

Maybe let's ask what the @rust-lang/rustdoc team thinks about it?

I assume the doc_bytes benchmarks are raw file sizes? Seems like this should compress really well and not affect the transferred size much.

The problem isn't the network part of the GUI rendering.

This allows people to treat them like real links, such as right-click to copy URL, and makes the line numbers in a scraped example work at all, when before this commit was added, they had the clickable pointer cursor but did not actually do anything when clicked.

notriddle · 2022-11-11T22:00:43Z

I can't think of any way to really mitigate the HTML overhead. Is this worth rebasing? Or just call it a dud and submit the :target fix as a separate PR?

GuillaumeGomez · 2022-11-12T10:26:56Z

Then I suggest this: let's only keep this change for the source code pages and remove it for the examples as it is where it will crazily increase.

GuillaumeGomez · 2022-11-12T18:26:31Z

Thanks!

@bors r+ rollup

bors · 2022-11-12T18:26:33Z

📌 Commit cb3a04b has been approved by GuillaumeGomez

It is now in the queue for this repository.

…r=GuillaumeGomez rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a` Example: https://notriddle.com/notriddle-rustdoc-demos/line-anchors/test_dingus/fn.test.html This allows people to treat them like real links, such as right-click to copy URL, and makes the line numbers in a scraped example work at all, when before this commit was added, they had the clickable pointer cursor but did not actually do anything when clicked.

Rollup of 5 pull requests Successful merges: - rust-lang#103650 (rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a`) - rust-lang#104177 (rustdoc: use consistent "popover" styling for notable traits) - rust-lang#104318 (Move tests) - rust-lang#104323 (rustdoc: remove no-op CSS `.scrape-help { background: transparent }`) - rust-lang#104345 (Fix up a Fluent message) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

notriddle assigned GuillaumeGomez Oct 27, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 27, 2022

notriddle force-pushed the notriddle/line-anchors branch 4 times, most recently from 5f38053 to 563986e Compare October 27, 2022 19:07

notriddle force-pushed the notriddle/line-anchors branch 2 times, most recently from d9b9630 to c9e7c56 Compare October 28, 2022 17:24

GuillaumeGomez reviewed Oct 28, 2022

View reviewed changes

notriddle force-pushed the notriddle/line-anchors branch from 78a7205 to ec51816 Compare October 28, 2022 20:30

GuillaumeGomez mentioned this pull request Oct 29, 2022

Fix z-indexes of code example feature and cleanup its CSS #103722

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 29, 2022

GuillaumeGomez approved these changes Oct 30, 2022

View reviewed changes

notriddle force-pushed the notriddle/line-anchors branch from ec51816 to 5ec54cc Compare October 31, 2022 20:27

notriddle force-pushed the notriddle/line-anchors branch from 5ec54cc to 5421a84 Compare October 31, 2022 20:29

GuillaumeGomez reviewed Oct 31, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

notriddle added 2 commits October 31, 2022 13:59

rustdoc: remove left border from .src-line-numbers > a

ba4ae13

notriddle force-pushed the notriddle/line-anchors branch from 5421a84 to ba4ae13 Compare October 31, 2022 20:59

GuillaumeGomez approved these changes Nov 12, 2022

View reviewed changes

rustdoc: avoid excessive HTML generated in example sources

cb3a04b

notriddle force-pushed the notriddle/line-anchors branch from 93594a6 to cb3a04b Compare November 12, 2022 16:23

GuillaumeGomez approved these changes Nov 12, 2022

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 Nov 12, 2022

Manishearth mentioned this pull request Nov 12, 2022

Rollup of 6 pull requests #104340

Closed

JohnTitor mentioned this pull request Nov 13, 2022

Rollup of 5 pull requests #104351

Merged

bors merged commit 96753eb into rust-lang:master Nov 13, 2022

rustbot added this to the 1.67.0 milestone Nov 13, 2022

notriddle deleted the notriddle/line-anchors branch November 13, 2022 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a` #103650

rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a` #103650

notriddle commented Oct 27, 2022

rustbot commented Oct 27, 2022

GuillaumeGomez commented Oct 28, 2022

GuillaumeGomez Oct 28, 2022

notriddle Oct 28, 2022

GuillaumeGomez Oct 28, 2022

notriddle Oct 28, 2022

GuillaumeGomez Oct 29, 2022

notriddle Oct 29, 2022

GuillaumeGomez Oct 29, 2022

GuillaumeGomez Oct 28, 2022

notriddle Oct 28, 2022

GuillaumeGomez Oct 29, 2022

notriddle commented Oct 29, 2022

rust-timer commented Oct 29, 2022

bors commented Oct 29, 2022

bors commented Oct 29, 2022

rust-timer commented Oct 29, 2022

rust-timer commented Oct 29, 2022

notriddle commented Oct 29, 2022

GuillaumeGomez commented Oct 30, 2022

notriddle commented Oct 31, 2022

GuillaumeGomez Oct 31, 2022

notriddle Oct 31, 2022

GuillaumeGomez Oct 31, 2022

Nemo157 Oct 31, 2022

GuillaumeGomez Nov 1, 2022

This comment has been minimized.

notriddle commented Nov 11, 2022

GuillaumeGomez commented Nov 12, 2022

GuillaumeGomez commented Nov 12, 2022

bors commented Nov 12, 2022

rustdoc: change .src-line-numbers > span to .src-line-numbers > a #103650

rustdoc: change .src-line-numbers > span to .src-line-numbers > a #103650

Conversation

notriddle commented Oct 27, 2022

rustbot commented Oct 27, 2022

GuillaumeGomez commented Oct 28, 2022

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

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

notriddle commented Oct 29, 2022

rust-timer commented Oct 29, 2022

bors commented Oct 29, 2022

bors commented Oct 29, 2022

rust-timer commented Oct 29, 2022

rust-timer commented Oct 29, 2022

Overall result: ❌ regressions - ACTION NEEDED

notriddle commented Oct 29, 2022

GuillaumeGomez commented Oct 30, 2022

notriddle commented Oct 31, 2022

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

This comment has been minimized.

notriddle commented Nov 11, 2022

GuillaumeGomez commented Nov 12, 2022

GuillaumeGomez commented Nov 12, 2022

bors commented Nov 12, 2022

rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a` #103650

rustdoc: change `.src-line-numbers > span` to `.src-line-numbers > a` #103650