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

[rustdoc] Allows links in headings #117662

Merged
merged 7 commits into from
Jan 20, 2024

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 7, 2023

Reopening of #94360.

Explanations

Rustdoc currently doesn't follow the markdown spec on headings: we don't allow links in them. So instead of having headings linking to themselves, this PR generates an anchor on the left side like this:

image

previous version

image

Having the anchor always displayed allows for mobile devices users to be able to have a link to the anchor. The different color used for the anchor itself is the same as links so people notice when looking at it that they can click on it.

You can test it here.

cc @camelid
r? @notriddle

@rustbot 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 Nov 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@notriddle
Copy link
Contributor

This doesn’t seem like a great solution. The section button adds visual clutter after so much other work has been done to reduce it.

Other than conflicting with the [-] button in the top-doc, are there any other problems with having it in the gutter and showing it on mouse-over?

The top-doc problem seems like it could be acceptably solved by hiding the button entirely:

#top-doc > *:first-child > .anchor { display: none }

It is inconsistent, but since the header is at the top of the page, not being able to link to it directly doesn’t seem like that big of a missing feature.

@GuillaumeGomez
Copy link
Member Author

This doesn’t seem like a great solution. The section button adds visual clutter after so much other work has been done to reduce it.

Agreed, hence why we now have hosted documentation so we can suggest alternatives. :)

Other than conflicting with the [-] button in the top-doc, are there any other problems with having it in the gutter and showing it on mouse-over?

Because it brings two issues:

  1. It doesn't work on mobile (that's minor)
  2. It either creates a void (where the symbol is supposed to be) or moves the content when mouse overs the title or needs to be placed at the end of the heading (which brings new issues like "what happens if the symbol can't stand on the same line and then appears on the next one?".

So displaying the symbol all the time seems like the best solution. Other possibilities I see are (they are not mutually exclusive):

  • Change the icon for the anchor
  • Change the color
    • Same color all the time?
    • Dimmed out until heading is hovered?
    • Something else?

The top-doc problem seems like it could be acceptably solved by hiding the button entirely:

#top-doc > *:first-child > .anchor { display: none }

It is inconsistent, but since the header is at the top of the page, not being able to link to it directly doesn’t seem like that big of a missing feature.

As you said, it is inconsistent and I'd prefer to avoid that if possible.

@notriddle
Copy link
Contributor

It either creates a void (where the symbol is supposed to be) or moves the content when mouse overs the title or needs to be placed at the end of the heading (which brings new issues like "what happens if the symbol can't stand on the same line and then appears on the next one?".

The Methods from Deref heading already has a solution to those problems: It displays in the gutter when you mouse over it, where a gap already exists.

@GuillaumeGomez
Copy link
Member Author

Yes but it's okay because in this context, there is no [-] element so there can not be any conflict. In the case of headings, we'd need to have an incoherence in case we remove the anchor if the first element of the top doc block is a heading, which is my main issue.

But if it's only a concern for me, I can make it appear on hover except on the first heading if the first element of the top doc block and end the debate. :)

I suppose the color should be the one from the text as well?

(Confirming before making changes 😉 )

@notriddle
Copy link
Contributor

Yeah, that's the idea. Same way that Methods from Deref header works.

@GuillaumeGomez
Copy link
Member Author

Ok! I'm a bit sad for the mobile devices but I suppose it's a minor concern. Thanks for the feedback, doing the update as soon as possible!

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

So it now looks like this:

image

I also updated the link to the hosted docs in the first comment.

@notriddle
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 14, 2023

Team member @notriddle has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 14, 2023
@fmease
Copy link
Member

fmease commented Nov 15, 2023

Could we address the issue with mobile devices (which inherently don't support hovers), by utilizing @media (pointer:none), (pointer:coarse) { /* … */ } to always show § on mobile devices (or is that considered to be too cluttered)? I haven't checked browser support on https://caniuse.com yet. Rn, there's no way to “obtain” a section's link anchor on mobile, did I read that right? Ah, you need to click on the header first to display the section sign.

@GuillaumeGomez
Copy link
Member Author

So you suggest to display the anchor all the time on mobile devices? No need to use such CSS features, we could simply use a media query to make the anchors displayed all the time in this case. However, it would mean more content displayed on devices that don't have that much screen space

I'm personally fine with that (ie, displaying anchors all the time on mobile devices) but we currently don't display existing anchors already, so I think the best would be to remain coherent with that (at least for the moment, we can discuss about that in a separate issue).

@GuillaumeGomez
Copy link
Member Author

From last rustdoc meeting:

seems like we have a consensus to make all headings except code headers to make the anchor appear on hover and not be a link anymore

I'll update the PR in the next days following this input.

@GuillaumeGomez
Copy link
Member Author

I unified the headings and the anchors and used this opportunity to move the creation of such headings into one place. With this, I think the concern was resolved?

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 18, 2024
@rfcbot
Copy link

rfcbot commented Jan 18, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@notriddle
Copy link
Contributor

@bors r

@bors
Copy link
Contributor

bors commented Jan 18, 2024

📌 Commit bf4a20c has been approved by notriddle

It is now in the queue for this repository.

@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 Jan 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 19, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117561 (Stabilize `slice_first_last_chunk`)
 - rust-lang#117662 ([rustdoc] Allows links in headings)
 - rust-lang#119815 (Format sources into the error message when loading codegen backends)
 - rust-lang#119835 (Exhaustiveness: simplify empty pattern logic)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120009 (never_patterns: typecheck never patterns)
 - rust-lang#120122 (Don't add needs-triage to A-diagnostics)
 - rust-lang#120126 (Suggest `.swap()` when encountering conflicting borrows from `mem::swap` on a slice)
 - rust-lang#120134 (Restrict access to the private field of newtype indexes)

Failed merges:

 - rust-lang#119968 (Remove unused/unnecessary features)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117561 (Stabilize `slice_first_last_chunk`)
 - rust-lang#117662 ([rustdoc] Allows links in headings)
 - rust-lang#119815 (Format sources into the error message when loading codegen backends)
 - rust-lang#119835 (Exhaustiveness: simplify empty pattern logic)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120009 (never_patterns: typecheck never patterns)
 - rust-lang#120122 (Don't add needs-triage to A-diagnostics)
 - rust-lang#120126 (Suggest `.swap()` when encountering conflicting borrows from `mem::swap` on a slice)
 - rust-lang#120134 (Restrict access to the private field of newtype indexes)

Failed merges:

 - rust-lang#119968 (Remove unused/unnecessary features)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cad609d into rust-lang:master Jan 20, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Rollup merge of rust-lang#117662 - GuillaumeGomez:links-in-headings, r=notriddle

[rustdoc] Allows links in headings

Reopening of rust-lang#94360.

# Explanations

Rustdoc currently doesn't follow the markdown spec on headings: we don't allow links in them. So instead of having headings linking to themselves, this PR generates an anchor on the left side like this:

![image](https://github.com/rust-lang/rust/assets/3050060/a118a7e9-5ef8-4d07-914f-46defc3245c3)

<details>
<summary>previous version</summary>

![image](https://github.com/rust-lang/rust/assets/3050060/c34fa844-9cd4-47dc-bb51-b37f5f66afee)

</details>

Having the anchor always displayed allows for mobile devices users to be able to have a link to the anchor. The different color used for the anchor itself is the same as links so people notice when looking at it that they can click on it.

You can test it [here](https://rustdoc.crud.net/imperio/links-in-headings/std/index.html).

cc `@camelid`
r? `@notriddle`
@GuillaumeGomez GuillaumeGomez deleted the links-in-headings branch January 20, 2024 11:04
@mgeisler
Copy link
Contributor

mgeisler commented Jan 20, 2024

Hey folks, I realize this was just merged... but I don't see this mentioned above. For me, a major use of self-links (in Rust documentation and elsewhere) is to copy-paste them into other places.

What I'm talking about is a workflow where I highlight a self-link like this:

image

Does Ctrl-c and then Ctrl-v somewhere else. The somewhere else could be here: Monotone Matrices.

The GitHub comment editor is clever enough to turn this into

[Monotone Matrices](https://docs.rs/smawk/latest/smawk/#monotone-matrices)

which I find super useful! I couldn't have written the link better myself 😄

For that to work, it's necessary that the link has useful text. Is that ability going away?

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jan 20, 2024

I didn't even know it was a possibility. I suppose it could be possible to replicate this behaviour on the anchor directly.

@mgeisler
Copy link
Contributor

Perhaps it can be replicated, I don't really know how it works behind the scenes.

I just wanted to mention it since regressing on this (unknown) feature is a step back for those that use it. That might of course just be me, in which case you can take this as me spreading awareness of it 😄

@GuillaumeGomez
Copy link
Member Author

Well, we were not markdown-compliant: headings are not supposed to be links. ^^'

@mgeisler
Copy link
Contributor

Well, we were not markdown-compliant:

Yeah, I see what you mean.

It can be argued in several ways. One way to see it would be to say that rustdoc simply transformed the input a bit before sending it to the Markdown (Commonmark) parser: # Foo becomes # [Foo](#foo). This is similar to how rustdoc already happily

So I would not call this case super clear 😄

headings are not supposed to be links. ^^'

Yeah, I agree! I find it a typographic no-no when users add links in headings... meaning I much prefer when headings link back to themselves 😉

If the headings themselves don't do that, I of course look for a TOC somewhere on the page. If that doesn't work, then I can of course do the classic thing and right-click to copy the link address and then write the link out normally.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 25, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 18, 2024
…Gomez

Fix heading anchors in doc pages.

This fixes the heading anchors on the standalone doc pages (the index, releases, etc.) so that the § symbol is only shown when the user hovers over the heading. This was changed in rust-lang#117662, but this CSS was not updated.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 18, 2024
…Gomez

Fix heading anchors in doc pages.

This fixes the heading anchors on the standalone doc pages (the index, releases, etc.) so that the § symbol is only shown when the user hovers over the heading. This was changed in rust-lang#117662, but this CSS was not updated.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2024
Fix heading anchors in doc pages.

This fixes the heading anchors on the standalone doc pages (the index, releases, etc.) so that the § symbol is only shown when the user hovers over the heading. This was changed in rust-lang#117662, but this CSS was not updated.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 18, 2024
…Gomez

Fix heading anchors in doc pages.

This fixes the heading anchors on the standalone doc pages (the index, releases, etc.) so that the § symbol is only shown when the user hovers over the heading. This was changed in rust-lang#117662, but this CSS was not updated.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2024
Rollup merge of rust-lang#122693 - ehuss:rust-css-header, r=GuillaumeGomez

Fix heading anchors in doc pages.

This fixes the heading anchors on the standalone doc pages (the index, releases, etc.) so that the § symbol is only shown when the user hovers over the heading. This was changed in rust-lang#117662, but this CSS was not updated.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 19, 2024
Fix heading anchors in doc pages.

This fixes the heading anchors on the standalone doc pages (the index, releases, etc.) so that the § symbol is only shown when the user hovers over the heading. This was changed in rust-lang/rust#117662, but this CSS was not updated.
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Mar 29, 2024
Pkgsrc changes:
 * Adapt checksums and patches.

Upstream chnages:

Version 1.77.0 (2024-03-21)
==========================

- [Reveal opaque types within the defining body for exhaustiveness checking.]
  (rust-lang/rust#116821)
- [Stabilize C-string literals.]
  (rust-lang/rust#117472)
- [Stabilize THIR unsafeck.]
  (rust-lang/rust#117673)
- [Add lint `static_mut_refs` to warn on references to mutable statics.]
  (rust-lang/rust#117556)
- [Support async recursive calls (as long as they have indirection).]
  (rust-lang/rust#117703)
- [Undeprecate lint `unstable_features` and make use of it in the compiler.]
  (rust-lang/rust#118639)
- [Make inductive cycles in coherence ambiguous always.]
  (rust-lang/rust#118649)
- [Get rid of type-driven traversal in const-eval interning]
  (rust-lang/rust#119044),
  only as a [future compatiblity lint]
  (rust-lang/rust#122204) for now.
- [Deny braced macro invocations in let-else.]
  (rust-lang/rust#119062)

Compiler
--------

- [Include lint `soft_unstable` in future breakage reports.]
  (rust-lang/rust#116274)
- [Make `i128` and `u128` 16-byte aligned on x86-based targets.]
  (rust-lang/rust#116672)
- [Use `--verbose` in diagnostic output.]
  (rust-lang/rust#119129)
- [Improve spacing between printed tokens.]
  (rust-lang/rust#120227)
- [Merge the `unused_tuple_struct_fields` lint into `dead_code`.]
  (rust-lang/rust#118297)
- [Error on incorrect implied bounds in well-formedness check]
  (rust-lang/rust#118553),
  with a temporary exception for Bevy.
- [Fix coverage instrumentation/reports for non-ASCII source code.]
  (rust-lang/rust#119033)
- [Fix `fn`/`const` items implied bounds and well-formedness check.]
  (rust-lang/rust#120019)
- [Promote `riscv32{im|imafc}-unknown-none-elf` targets to tier 2.]
  (rust-lang/rust#118704)
- Add several new tier 3 targets:
  - [`aarch64-unknown-illumos`]
    (rust-lang/rust#112936)
  - [`hexagon-unknown-none-elf`]
    (rust-lang/rust#117601)
  - [`riscv32imafc-esp-espidf`]
    (rust-lang/rust#119738)
  - [`riscv32im-risc0-zkvm-elf`]
    (rust-lang/rust#117958)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Implement `From<&[T; N]>` for `Cow<[T]>`.]
  (rust-lang/rust#113489)
- [Remove special-case handling of `vec.split_off
  (0)`.](rust-lang/rust#119917)

Stabilized APIs
---------------

- [`array::each_ref`]
  (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_ref)
- [`array::each_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_mut)
- [`core::net`]
  (https://doc.rust-lang.org/stable/core/net/index.html)
- [`f32::round_ties_even`]
  (https://doc.rust-lang.org/stable/std/primitive.f32.html#method.round_ties_even)
- [`f64::round_ties_even`]
  (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.round_ties_even)
- [`mem::offset_of!`]
  (https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html)
- [`slice::first_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk)
- [`slice::first_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk_mut)
- [`slice::split_first_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk)
- [`slice::split_first_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk_mut)
- [`slice::last_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk)
- [`slice::last_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk_mut)
- [`slice::split_last_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk)
- [`slice::split_last_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk_mut)
- [`slice::chunk_by`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by)
- [`slice::chunk_by_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by_mut)
- [`Bound::map`]
  (https://doc.rust-lang.org/stable/std/ops/enum.Bound.html#method.map)
- [`File::create_new`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.create_new)
- [`Mutex::clear_poison`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.clear_poison)
- [`RwLock::clear_poison`]
  (https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.clear_poison)

Cargo
-----

- [Extend the build directive syntax with `cargo::`.]
  (rust-lang/cargo#12201)
- [Stabilize metadata `id` format as `PackageIDSpec`.]
  (rust-lang/cargo#12914)
- [Pull out as `cargo-util-schemas` as a crate.]
  (rust-lang/cargo#13178)
- [Strip all debuginfo when debuginfo is not requested.]
  (rust-lang/cargo#13257)
- [Inherit jobserver from env for all kinds of runners.]
  (rust-lang/cargo#12776)
- [Deprecate rustc plugin support in cargo.]
  (rust-lang/cargo#13248)

Rustdoc
-----

- [Allows links in markdown headings.]
  (rust-lang/rust#117662)
- [Search for tuples and unit by type with `()`.]
  (rust-lang/rust#118194)
- [Clean up the source sidebar's hide button.]
  (rust-lang/rust#119066)
- [Prevent JS injection from `localStorage`.]
  (rust-lang/rust#120250)

Misc
----

- [Recommend version-sorting for all sorting in style guide.]
  (rust-lang/rust#115046)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

- [Add more weirdness to `weird-exprs.rs`.]
  (rust-lang/rust#119028)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.

None yet

9 participants