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

Support links in headings #94360

Closed

Conversation

GuillaumeGomez
Copy link
Member

Follow-up of #93808.

Thanks to @notriddle, I was reminded that you could have links in markdown headings, meaning that this change was very bad. So instead, I now show the anchor all the time (up to debate) and allow links in headings:

Screenshot from 2022-02-25 15-37-03

You can test it here.

r? @jsha

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Feb 25, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2022
@rust-log-analyzer

This comment has been minimized.

@camelid camelid added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Mar 3, 2022
@camelid
Copy link
Member

camelid commented Mar 3, 2022

This would be a new, stable feature, and honestly, I don't think we should support this. It's a niche feature and adds unnecessary UI complexity.

@GuillaumeGomez
Copy link
Member Author

Niche I don't know. However it's definitely part of the markdown spec which we're supposed to follow. The problem here is that we disregarded it. Since the anchor is now displayed all the time, the UI is actually simpler in my opinion (compared to before).

@notriddle
Copy link
Contributor

I like the idea of supporting links in headers. It's good for interoperability, so you can use #![doc = include_str("../README.md")] to render the same markdown as GitHub and crates.io, both of which support this. The CommonMark spec — which we claim to support — says that the contents of headers are parsed as inlines, and links are valid inlines.

But I don't like a persistent, always-visible section link. #59840 is all about getting rid of this kind of clutter.

In rough order of my own preferences:

  1. Support links in headers. Hide the section link except when you mouse over the header. Put it at the end, kinda like this, so that you don't get conflicts with the hide/show button:

    <h1 id=my-heading>My Heading<a class=section-header>&nbsp;§</a></h1>
    <style>h1 .section-header { visibility: hidden; padding: 0.25em 0; font-size: 0.5em } h1:hover .section-header, h1 .section-header:active, h1 .section-header:focus { visibility: visible }</style>
    

    The smaller font size keeps it from causing too much excessive line wrapping, even though it technically takes up space. Using visibility:hidden instead of display:none means stuff doesn't jump around when you move your mouse. And the nbsp keeps it from becoming a typographical widow, forcing the last word to wrap along with the section link.

  2. If you really don't want to support links in headers, use the pulldown-cmark to actually strip them out. That way, at least we don't generate broken HTML when someone tries to use it.

    Something like this:

    struct StripLinksInHeaders<P> {
        in_header: bool,
        parser: P,
    }
    impl<P> Iterator for StripLinksInHeader<P> where P: Iterator<Item=Event> {
        type Item = Event;
        fn next(&mut self) -> Option<Event> {
            let mut n = self.parser.next()?;
            if n == Event(Start(Header)) { self.in_header = true; }
            while self.in_header && (n == Event(Start(Link)) || n == Event(End(Link))) {
                n = self.parser.next()?;
            }
            if n == Event(End(Header)) { self.in_header = false; }
            Some(n)
        }
    }
    

@camelid
Copy link
Member

camelid commented Mar 4, 2022

Since the anchor is now displayed all the time, the UI is actually simpler in my opinion (compared to before).

The click target is then very small though.

  1. Hide the section link except when you mouse over the header. Put it at the end, kinda like this, so that you don't get conflicts with the hide/show button:

Hmm, but then it's in a different position depending on how long the header is.

@GuillaumeGomez
Copy link
Member Author

I'd prefer to keep the anchor on the left too (because it's where it is for all other headings). A middle-ground could be to turn the visibility off by default so it keeps taking the space but is visible only when you hover the heading.

@camelid
Copy link
Member

camelid commented Aug 2, 2022

I'm inclined to close this PR since it seems like more discussion is needed. What do you think?

@GuillaumeGomez
Copy link
Member Author

We don't have a tracking issue. Let's open one to continue the discussion there and then close this PR.

@camelid
Copy link
Member

camelid commented Aug 8, 2022

Ok, I opened a tracking issue and summarized my understanding of the current state of this issue: #100254

@camelid camelid closed this Aug 8, 2022
@GuillaumeGomez
Copy link
Member Author

Thanks!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 19, 2024
…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`
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`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 21, 2024
[rustdoc] Allows links in headings

Reopening of rust-lang/rust#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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants