Render Markdown in search results #77686

camelid · 2020-10-08T01:15:16Z

Fixes #32040.

Previously Markdown documentation was not rendered to HTML for search results,
which led to the output not being very readable, particularly for inline code.
This PR fixes that by rendering Markdown to HTML with the help of pulldown-cmark
(the library rustdoc uses to parse Markdown for the main text of documentation).
However, the text for the title attribute (the text shown when you hover over an
element) still uses the plain-text rendering since it is displayed in browsers
as plain-text.

Only these styles will be rendered; everything else is stripped away:

italics
bold
inline code

rust-highfive · 2020-10-08T01:15:19Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

rust-highfive · 2020-10-08T01:15:21Z

r? @jyn514

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

camelid · 2020-10-08T02:14:05Z

Here's what it looks like:

Still some bugs to fix, like the brackets in the code links. Interestingly, links are rendered correctly with plain text (i.e., rendered as just the link text), but not with inline code. Also, some of the summaries have an extra space before the ellipsis.

camelid · 2020-10-08T02:16:02Z

Also the summary for Command::arg0() isn't being rendered for some reason. Maybe it's because it's platform-specific? (It's not being rendered in the official docs either though, so not because of this.)

jyn514 · 2020-10-08T02:42:29Z

cc @euclio , you know this code better than I do.

jyn514 · 2020-10-08T02:44:52Z

@camelid have you considered the issues ollie mentioned in #68699 (comment) ?

camelid · 2020-10-08T02:47:05Z

Ah, I was trying to figure out where the alt-text was used. I'll add back the plain summary function in addition, and use that for alt-text.

jyn514 · 2020-10-08T02:49:39Z

cc @GuillaumeGomez , this replaces #68699. I'm not yet sure how it differs from that PR, although I think it does go through pulldown instead of re-parsing markdown in javascript.

camelid · 2020-10-08T02:51:41Z

Yes, this approach is to render the HTML using pulldown ahead of time.

camelid · 2020-10-08T23:31:51Z

Hmm, it's still rendering alt-text as HTML...

camelid · 2020-10-24T01:00:58Z

Rebased.

jyn514

Ah, I was trying to figure out where the alt-text was used. I'll add back the plain summary function in addition, and use that for alt-text.

Did you fix this? If so, can you add a test?

We discussed in PMs that you're still working on this so I'm not doing a full review for now - let me know when you've got this working and I'll take another look.

src/librustdoc/html/markdown.rs

src/librustdoc/html/static/main.js

src/librustdoc/html/render/mod.rs

src/librustdoc/html/markdown.rs

jyn514

I don't think I'm the best reviewer for this.

r? @GuillaumeGomez

src/librustdoc/html/static/main.js

src/librustdoc/html/markdown.rs

jyn514 · 2020-11-06T20:14:18Z

src/librustdoc/html/static/main.js

-        return html.replace('<code>', '`').replace('</code>', '`')
-                   .replace(/<[^>]+>/, '');
+        var dom = new DOMParser().parseFromString(
+            html.replace('<code>', '`').replace('</code>', '`'),


This bit is no longer necessary I think.

Suggested change

html.replace('<code>', '`').replace('</code>', '`'),

html,

Oh I take it back - it is necessary if you want to replace code blocks with `. Not sure why rustdoc is doing that, though.

It's not strictly necessary, but I think it's better to do it this way, for one since it used to use ` here.

Well, it kinda makes sense. However, we also keep other HTML tags like <b> and <i>. Should we also "revert" them into markdown too?

It's not strictly necessary, but I think it's better to do it this way, for one since it used to use ` here.

My comment still stands. :p

we have the same issue as in #68699 (comment) that we don't know what was originally html and what was markdown

I do not think that is correct. The example given in #68699 (comment) was:

/// foo `this is a code span` bar \`this isn't\` baz pub struct Foo;

That is stored in the search index as:

foo <code>this is a code span</code> bar `this isn't` baz

And then convertHTMLToPlaintext converts that into:

foo `this is a code span` bar `this isn't` baz

Which is exactly the same as the current hover text (that you get with rustdoc nightly).

That said, I am open to just stripping every HTML tag if we decide that would be better. I just don't think #68699 (comment) is relevant.

@camelid the test case is the other way:

/// foo <code>this is a code span</code> in HTML /// /// `this is markdown` baz pub struct Foo;

There's no way to get back that info after the fact.

I just copy-pasted the test case from the comment you linked to. The comment doesn't seem to be taking issue with the example you're giving. This is the full comment text:

No. The summaries stored in the search index don't contain that information. For:

/// foo `this is a code span` bar \`this isn't\` baz pub struct Foo;

the search index contains:

foo `this is a code span` bar `this isn't` baz

There is no way in JavaScript to know which backticks meant codeblocks and which didn't.

As I've said, to do this right the summaries need to be rendered and stored in the search index as HTML.

It is saying that converting backticks into HTML is a problem because they both mean inline code and literal backticks. Converting in the opposite direction is loss-less.

And, either way, I don't see how converting both `code` and <code>code</code> into `code` when this is just for alt-text is an issue. If for some reason this is a blocker, I am absolutely fine to just strip all HTML tags.

I'm not 100% sure about this but I don't think it should block this change. So if @GuillaumeGomez thinks this is good to go I do too.

I said I didn't. 😛

camelid · 2020-12-03T22:00:36Z

@Mark-Simulacrum Wait, but this will make the squashed commit show as authored by bors.

@bors squash-

bors · 2020-12-03T22:00:38Z

@camelid: 🔑 Insufficient privileges: not in try users

jyn514 · 2020-12-03T22:03:31Z

@bors squash-

Mark-Simulacrum · 2020-12-03T22:05:00Z

I am confused -- my understanding is that the functionality in bors does not produce that result? It'll set the committer to bors most likely but that seems fine?

camelid · 2020-12-03T22:07:34Z

I'm not sure, but I'd rather squash by hand if you want me to squash.

Mark-Simulacrum · 2020-12-03T22:08:12Z

I would personally like to at least get rid of the "Fix typo" commit, I don't care strongly about others though.

Previously Markdown documentation was not rendered to HTML for search results, which led to the output not being very readable, particularly for inline code. This PR fixes that by rendering Markdown to HTML with the help of pulldown-cmark (the library rustdoc uses to parse Markdown for the main text of documentation). However, the text for the title attribute (the text shown when you hover over an element) still uses the plain-text rendering since it is displayed in browsers as plain-text. Only these styles will be rendered; everything else is stripped away: * *italics* * **bold** * `inline code`

@GuillaumeGomez

@GuillaumeGomez was concerned about browser compatibility.

Finally!

Accidentally removed in rebase.

camelid · 2020-12-03T22:12:30Z

I squashed in the "Fix typo" commit. @Mark-Simulacrum could you r=GuillaumeGomez?

Mark-Simulacrum · 2020-12-03T22:14:04Z

I guess 376507f is also a typo but I'm out of energy here so @bors r=GuillaumeGomez delegate+

bors · 2020-12-03T22:14:06Z

📌 Commit 376507f has been approved by GuillaumeGomez

bors · 2020-12-03T22:14:08Z

✌️ @camelid can now approve this pull request

camelid · 2020-12-03T22:14:50Z

If it's a big deal, I can squash more away, but I would really like to get this merged.

Rollup of 11 pull requests Successful merges: - rust-lang#77686 (Render Markdown in search results) - rust-lang#79541 (Doc keyword lint pass) - rust-lang#79602 (Fix SGX CI) - rust-lang#79611 (Use more std:: instead of core:: in docs for consistency) - rust-lang#79623 (Pass around Symbols instead of Idents in doctree) - rust-lang#79627 (Update cargo) - rust-lang#79631 (disable a ptr equality test on Miri) - rust-lang#79638 (Use `item_name` instead of pretty printing for resolving `Self` on intra-doc links) - rust-lang#79646 (rustc_metadata: Remove some dead code) - rust-lang#79664 (move interpret::MemoryKind::Heap to const eval) - rust-lang#79678 (Fix some clippy lints) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2020-12-04T07:11:32Z

⌛ Testing commit 376507f with merge e9dd18c...

GuillaumeGomez · 2020-12-04T10:20:19Z

If it's a big deal, I can squash more away, but I would really like to get this merged.

Don't worry it wasn't a big issue. Thanks a lot for working on this!

camelid · 2020-12-04T20:12:53Z

Yay! Excited to look at the docs for the next nightly :)

I added `summary_opts()` before I cut the branch for rust-lang#77686 (2 months ago!), so this "slipped through the cracks".

…Gomez Use `summary_opts()` in another spot I added `summary_opts()` before I cut the branch for rust-lang#77686 (2 months ago!), so this "slipped through the cracks".

camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 8, 2020

rust-highfive assigned jyn514 Oct 8, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2020

camelid force-pushed the rustdoc-render-search-results branch 2 times, most recently from e766c01 to aa88d52 Compare October 8, 2020 23:23

This comment has been minimized.

Sign in to view

camelid force-pushed the rustdoc-render-search-results branch from b2fe8de to e1e172f Compare October 24, 2020 01:00

jyn514 reviewed Oct 24, 2020

View reviewed changes

src/librustdoc/html/markdown.rs Outdated Show resolved Hide resolved

src/librustdoc/html/static/main.js Show resolved Hide resolved

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

src/librustdoc/html/markdown.rs Outdated Show resolved Hide resolved

jyn514 added the A-rustdoc-search Area: Rustdoc's search feature label Oct 24, 2020

jyn514 reviewed Oct 24, 2020

View reviewed changes

src/librustdoc/html/markdown.rs Outdated Show resolved Hide resolved

jyn514 reviewed Oct 26, 2020

View reviewed changes

src/librustdoc/html/static/main.js Outdated Show resolved Hide resolved

jyn514 assigned GuillaumeGomez and unassigned jyn514 Oct 26, 2020

camelid mentioned this pull request Oct 30, 2020

Keep code coloring in search results short text #68699

Closed

GuillaumeGomez reviewed Nov 4, 2020

View reviewed changes

src/librustdoc/html/static/main.js Outdated Show resolved Hide resolved

GuillaumeGomez mentioned this pull request Nov 6, 2020

Add title for crate links #78805

Closed

camelid marked this pull request as ready for review November 6, 2020 20:09

GuillaumeGomez reviewed Nov 6, 2020

View reviewed changes

src/librustdoc/html/markdown.rs Outdated Show resolved Hide resolved

jyn514 reviewed Nov 6, 2020

View reviewed changes

camelid added 6 commits December 3, 2020 14:11

Use createElement and innerHTML instead of DOMParser

e178030

@GuillaumeGomez was concerned about browser compatibility.

Make length_limit a usize

07e9426

Add rustdoc-js test

b903519

Finally!

Add more rustdoc-js test cases

f0cf5a9

Add missing feature flag

376507f

Accidentally removed in rebase.

camelid force-pushed the rustdoc-render-search-results branch from 4cb30b2 to 376507f Compare December 3, 2020 22:12

Dylan-DPC-zz mentioned this pull request Dec 4, 2020

Rollup of 11 pull requests #79686

Merged

bors merged commit 6f2fbc1 into rust-lang:master Dec 4, 2020

rustbot added this to the 1.50.0 milestone Dec 4, 2020

camelid deleted the rustdoc-render-search-results branch December 4, 2020 20:12

camelid mentioned this pull request Dec 7, 2020

Use summary_opts() in another spot #79780

Merged

camelid added a commit to camelid/rust that referenced this pull request Dec 7, 2020

Use summary_opts() in another spot

2ee34a0

I added `summary_opts()` before I cut the branch for rust-lang#77686 (2 months ago!), so this "slipped through the cracks".

Render Markdown in search results #77686

Render Markdown in search results #77686

Conversation

camelid commented Oct 8, 2020 • edited Loading

rust-highfive commented Oct 8, 2020

rust-highfive commented Oct 8, 2020

camelid commented Oct 8, 2020

camelid commented Oct 8, 2020

jyn514 commented Oct 8, 2020

jyn514 commented Oct 8, 2020

camelid commented Oct 8, 2020

jyn514 commented Oct 8, 2020

camelid commented Oct 8, 2020

camelid commented Oct 8, 2020

This comment has been minimized.

camelid commented Oct 24, 2020

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid Nov 6, 2020 • edited Loading

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

camelid commented Dec 3, 2020

bors commented Dec 3, 2020

jyn514 commented Dec 3, 2020

Mark-Simulacrum commented Dec 3, 2020

camelid commented Dec 3, 2020

Mark-Simulacrum commented Dec 3, 2020

camelid commented Dec 3, 2020

Mark-Simulacrum commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

camelid commented Dec 3, 2020

bors commented Dec 4, 2020

GuillaumeGomez commented Dec 4, 2020

camelid commented Dec 4, 2020

camelid commented Oct 8, 2020 •

edited

Loading

camelid Nov 6, 2020 •

edited

Loading

camelid Nov 6, 2020 •

edited

Loading