Skip to content

[rustdoc] Allows links in headings #117662

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

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
Collaborator

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.

Sorry, something went wrong.

@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 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
Collaborator

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
Collaborator

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 bors merged commit cad609d into rust-lang:master Jan 20, 2024
@rustbot rustbot added this to the 1.77.0 milestone Jan 20, 2024
@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
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