Clean up dom #84703

GuillaumeGomez · 2021-04-29T15:46:33Z

The commits come from #84480.

They were errors reported by the tidy script that we will use to ensure that the HTML generated by rustdoc is valid.

I checked carefully that there were no difference so in principle it should be exactly the same rendering but a double-check would be very appreciated in case I missed something.

Extra note: <h4> and some <h3> tags were replaced by <div> because they're not supposed to contain tags as they currently do.

r? @jsha

rust-highfive · 2021-04-29T15:46:36Z

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

A change occurred in the Ayu theme.

cc @Cldfire

GuillaumeGomez · 2021-04-29T15:48:04Z

src/tools/linkchecker/main.rs

@@ -387,7 +387,7 @@ fn maybe_redirect(source: &str) -> Option<String> {
    const REDIRECT: &str = "<p>Redirecting to <a href=";

    let mut lines = source.lines();
-    let redirect_line = lines.nth(6)?;
+    let redirect_line = lines.nth(7)?;


For information: this change is needed because of my change in the redirection file which added a title to the page.

jsha

Generally looks good! One possible tweak.

src/librustdoc/html/render/print_item.rs

src/librustdoc/html/static/main.js

GuillaumeGomez · 2021-04-29T20:07:18Z

@jsha Very good point! tidy with happy with href="#" so all good. :)

notriddle · 2021-04-29T21:05:16Z

src/librustdoc/html/render/mod.rs

@@ -1328,7 +1328,7 @@ fn render_impl(
                        .map(|item| format!("{}.{}", item.type_(), name));
                    write!(
                        w,
-                        "<h4 id=\"{}\" class=\"{}{}{}\">",
+                        "<div id=\"{}\" class=\"{}{}{}\">",


https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/heading_role ?

So it should be:

Suggested change

"<div id=\"{}\" class=\"{}{}{}\">",

"<div id=\"{}\" class=\"{}{}{}\" role="heading" aria-level="4">",

if I understood correctly?

Oh, if so, that'll also allow me to simplify the CSS changes quite a lot.

Yeah, that's what I'm thinking.

bors · 2021-04-30T02:26:26Z

☔ The latest upstream changes (presumably #84729) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2021-04-30T10:19:01Z

Applied @notriddle's suggestion which allowed me to simplify the CSS changes. I also rebased.

jsha · 2021-04-30T18:12:24Z

@GuillaumeGomez can you update the PR description to describe why the <h4> tags had to go?

GuillaumeGomez · 2021-04-30T18:23:02Z

@jsha Good idea! I updated the description.

jsha · 2021-04-30T18:33:45Z

Looking at this again, I'm not sure the switch to <div> is the right fix. According to https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/heading_role:

The best way to use this role is to not use it at all, and instead use the native heading tags <h1> through <h6> as shown in the example above. The heading role and aria-level attribute should only be used to retrofit accessibility on legacy code that you cannot make major changes to.

What's the exact message you're getting from tidy? Are we confident tidy is correct about it?

This stack overflow suggests heading tags can contain "phrasing content": https://stackoverflow.com/a/9543834

The list of "phrasing content" (https://html.spec.whatwg.org/multipage/dom.html#phrasing-content) include <a> and <span> (and many others). What are we including inside our headings that's not among those? Maybe we have a <div> somewhere in there that should be a <span>?

GuillaumeGomez · 2021-04-30T19:19:13Z

We have <code> and a few other things in our <h3>/<h4> tags. In our case, I'm not even sure that "aria" role is actually useful (but it definitely made the CSS simpler).

jsha · 2021-04-30T19:24:07Z

We have <code> and a few other things in our <h3>/<h4> tags.

code is also on the list of phrasing content, so it should be allowed inside hN tags. What's the exact error message?

GuillaumeGomez · 2021-04-30T21:10:35Z

Here is a small sample of the tidy output on string doc page:

line 160 column 184 - Warning: missing </summary> before <h3>
line 160 column 483 - Warning: discarding unexpected </summary>
line 160 column 553 - Warning: missing </summary> before <h4>
line 160 column 1014 - Warning: discarding unexpected </summary>
line 172 column 53 - Warning: missing </summary> before <h4>
line 172 column 511 - Warning: discarding unexpected </summary>
line 200 column 53 - Warning: missing </summary> before <h4>
line 200 column 888 - Warning: discarding unexpected </summary>
line 238 column 53 - Warning: missing </summary> before <h4>
line 238 column 689 - Warning: discarding unexpected </summary>
line 272 column 53 - Warning: missing </summary> before <h4>
line 272 column 823 - Warning: discarding unexpected </summary>
line 288 column 53 - Warning: missing </summary> before <h4>
line 288 column 631 - Warning: discarding unexpected </summary>
line 304 column 53 - Warning: missing </summary> before <h4>
line 305 column 23 - Warning: discarding unexpected </summary>
line 324 column 53 - Warning: missing </summary> before <h4>
line 324 column 806 - Warning: discarding unexpected </summary>
line 362 column 53 - Warning: missing </summary> before <h4>
line 362 column 724 - Warning: discarding unexpected </summary>
line 382 column 53 - Warning: missing </summary> before <h4>
line 382 column 463 - Warning: missing </span> before <div>
line 382 column 434 - Warning: missing </span> before <div>
line 382 column 104 - Warning: missing </code> before <div>
line 382 column 501 - Warning: <div> isn't allowed in <h4> elements
line 382 column 541 - Warning: missing </span> before <h3>
line 382 column 501 - Warning: missing </h4> before <h3>
line 382 column 584 - Warning: inserting implicit <code>
line 382 column 584 - Warning: inserting implicit <span>
line 382 column 758 - Warning: inserting implicit <code>
line 382 column 758 - Warning: inserting implicit <span>
line 382 column 17 - Warning: missing </details>
line 382 column 1203 - Warning: discarding unexpected </span>
line 382 column 1210 - Warning: discarding unexpected </span>
line 382 column 1217 - Warning: discarding unexpected </code>
line 382 column 1367 - Warning: discarding unexpected </h4>
line 382 column 1372 - Warning: discarding unexpected </summary>
line 392 column 53 - Warning: missing </summary> before <h4>
line 392 column 466 - Warning: discarding unexpected </summary>
line 400 column 53 - Warning: missing </summary> before <h4>
line 400 column 490 - Warning: discarding unexpected </summary>
line 411 column 53 - Warning: missing </summary> before <h4>
line 411 column 409 - Warning: discarding unexpected </summary>
line 421 column 53 - Warning: missing </summary> before <h4>

jsha · 2021-04-30T21:27:06Z

Aha, this is what I was worried about:

Warning: <div> isn't allowed in <h4> elements

Instead of changing all our <h4> to <div>, we should figure out where we're emitting a <div> inside an <h4> and change that to a span.

line 200 column 53 - Warning: missing </summary> before <h4>

This looks like we failed to close a tag somewhere (and doesn't seemed to be part of this PR, just wanted to acknowledge since it's a big part of the output).

jsha · 2021-04-30T21:31:11Z

It looks like one significant candidate is:

<div class="notable-traits-tooltiptext">

notriddle · 2021-04-30T21:33:54Z

Apparently, headings inside <summary> are a huge mess, because the standards actually seem to be contradicting each other. If we can figure out how to not nest headers inside of summaries, that might be a good idea.

This poses us — and browser and assistive technology developers — with a contradiction:

Headings are permitted in <summary> elements to provide in-page navigational assistance.

Buttons strip the semantics out of anything (like headings) nested within them.

Unfortunately, assistive technologies are inconsistent in how they’ve handled this situation. Some screen-reading technologies, like NVDA and Apple’s VoiceOver, do acknowledge headings inside <summary> elements. JAWS, on the other hand, does not.

jsha · 2021-04-30T21:36:42Z

In other words, it probably doesn’t hurt to put a heading there. It just may not always help.

From reading that article, it sounds like the only issue is that JAWS won't interpret headings in summaries as headings. To me that suggests we should keep them as headings, but also add an aria-role to ensure they are treated as headings by JAWS. Assuming that works. Do you have a copy of JAWS to test?

notriddle · 2021-04-30T22:03:14Z

Unfortunately, no I don't.

GuillaumeGomez · 2021-05-01T12:45:50Z

@jsha I think it shouldn't have been h3 and h4 tags to begin with. Instead, I suggest to replace the aria info with data-level or something along the line. What do you think?

jsha · 2021-05-01T16:37:40Z

I think <h3> and <h4> actually are a decent representation of the content - they do in fact act like headings. But I'm not particularly attached to them. It would also be reasonable to make them <span>. But then you will probably get the same lint, since span is supposed to also contain only phrasing content:

https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-span-element

I think the tidy check is actually pointing out for us a wider fix, which is great! That's what tidy tools should be, at their best. The notable-traits tooltip should not be part of the <h4> for method definitions.

Try this: visit https://doc.rust-lang.org/std/boxed/struct.Box.html and run in the console document.querySelectorAll(".notable a"). You'll see 101 entries, each of them part of a whole little sub-document showing notable traits for something. And there's a lot of duplication among these, wasting bytes and DOM complexity.

I think we should build the various notable-traits tooltips potentially required by a document in a single, hidden DIV at the end of the document, then use JS to put them into place on hover. That allows us to de-duplicate them. It makes it so we're not embedding a <div> (with its own complex sub-structure of headings and links) into our method headings. And it would make it easy to position the tooltip so it's fully-onscreen with no wrapping, something the current tooltip doesn't do very well.

GuillaumeGomez · 2021-05-01T16:42:47Z

But it would require JS to work whereas it doesn't need it currently. I think we should rely as little as possible on JS if possible. So maybe we could work around this limitation with page-specific CSS or something along the line?

jsha · 2021-05-01T17:00:44Z

Fair point about requiring JS for the tooltips where we currently don't. Another possibility, which solves the "don't put divs inside spans" problem but not the duplication problem, would be to move the (i) and its corresponding tooltip to just outside of the <h4> (or <span>), and give it display: inline-block so it flows right.

GuillaumeGomez · 2021-06-02T20:50:07Z

Replaced the <h3> in notable traits and fixed the other comments. :)

jsha · 2021-06-02T21:30:57Z

@bors r+

Excellent. Thanks for sticking through all my comments. I'm really happy with the results: not only have we made tidy happy, we've significantly cleaned up and clarified our DOM and CSS.

bors · 2021-06-02T21:30:59Z

📌 Commit 0daf8ac has been approved by jsha

bors · 2021-06-02T23:11:45Z

⌛ Testing commit 0daf8ac with merge da86509...

bors · 2021-06-03T01:59:27Z

☀️ Test successful - checks-actions
Approved by: jsha
Pushing da86509 to master...

GuillaumeGomez · 2021-06-03T08:08:34Z

Excellent. Thanks for sticking through all my comments. I'm really happy with the results: not only have we made tidy happy, we've significantly cleaned up and clarified our DOM and CSS.

And thank you for the great insight and reviews!

These were changed in rust-lang#84703.

…ulacrum Fix a bug in the linkchecker There was a small typo in the linkchecker (in rust-lang#85652) that caused it to report a `#` fragment link error pointing to the wrong file (it was displaying the path to the source file, not the target of the link). This also includes a few other changes: - Fixes the tests due to some changes in the redirect handling in rust-lang#84703. - Adds the tests to rustbuild to run whenever the linkchecker itself is run. - Updates the tests to validate more of the output (so that a mistake like this would have been caught). Closes rust-lang#86144

Part of rust-lang#87059 Partially reverts rust-lang#84703 Preview at: https://notriddle.com/notriddle-rustdoc-test/real-headers/std/index.html

…atch, r=GuillaumeGomez Rustdoc accessibility: use real headers for doc items Part of rust-lang#87059 Partially reverts rust-lang#84703 Preview at: https://notriddle.com/notriddle-rustdoc-test/real-headers/std/index.html

Part of rust-lang#87059 Partially reverts rust-lang#84703 Heavily modified for beta backport needs.

Part of rust-lang#87059 Partially reverts rust-lang#84703 Backport of rust-lang#87390 Preview at: https://notriddle.com/notriddle-rustdoc-test/real-headers/std/index.html

Part of rust-lang#87059 Partially reverts rust-lang#84703 Preview at: https://notriddle.com/notriddle-rustdoc-test/real-headers/std/index.html

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 Apr 29, 2021

rust-highfive assigned jsha Apr 29, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2021

GuillaumeGomez commented Apr 29, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the cleanup-dom branch from e97b4f9 to c7cc23f Compare April 29, 2021 18:29

jsha reviewed Apr 29, 2021

View reviewed changes

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

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

GuillaumeGomez force-pushed the cleanup-dom branch from c7cc23f to 9dd46ab Compare April 29, 2021 20:06

notriddle reviewed Apr 29, 2021

View reviewed changes

GuillaumeGomez force-pushed the cleanup-dom branch from 9dd46ab to 43a3794 Compare April 30, 2021 10:18

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 Jun 2, 2021

jsha approved these changes Jun 2, 2021

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2021

bors merged commit da86509 into rust-lang:master Jun 3, 2021

rustbot added this to the 1.54.0 milestone Jun 3, 2021

bors mentioned this pull request Jun 3, 2021

rustdoc: staggered layout for module contents on mobile #85651

Merged

GuillaumeGomez deleted the cleanup-dom branch June 3, 2021 08:07

cynecx mentioned this pull request Jun 6, 2021

rustdoc: almost all text is bold #86069

Closed

ehuss added a commit to ehuss/rust that referenced this pull request Jun 8, 2021

Fix linkchecker redirection tests.

7dd2577

These were changed in rust-lang#84703.

ehuss mentioned this pull request Jun 9, 2021

Fix a bug in the linkchecker #86156

Merged

camelid mentioned this pull request Jun 13, 2021

regression: Type aliases are bolded #86256

Closed

notriddle mentioned this pull request Jul 22, 2021

Rustdoc accessibility: use real headers for doc items #87390

Closed

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2021

Rustdoc accessibility: use real headers for doc items

76a3b60

Part of rust-lang#87059 Partially reverts rust-lang#84703 Preview at: https://notriddle.com/notriddle-rustdoc-test/real-headers/std/index.html

GuillaumeGomez pushed a commit to pietroalbini/rust that referenced this pull request Jul 26, 2021

Rustdoc accessibility: use real headers for doc items

22bbbe8

Part of rust-lang#87059 Partially reverts rust-lang#84703 Heavily modified for beta backport needs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up dom #84703

Clean up dom #84703

GuillaumeGomez commented Apr 29, 2021 •

edited

Loading

rust-highfive commented Apr 29, 2021

GuillaumeGomez Apr 29, 2021

This comment has been minimized.

jsha left a comment

GuillaumeGomez commented Apr 29, 2021

notriddle Apr 29, 2021

GuillaumeGomez Apr 29, 2021 •

edited

Loading

GuillaumeGomez Apr 29, 2021

notriddle Apr 29, 2021

bors commented Apr 30, 2021

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021 •

edited

Loading

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021 •

edited

Loading

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021

jsha commented Apr 30, 2021

notriddle commented Apr 30, 2021 •

edited

Loading

jsha commented Apr 30, 2021

notriddle commented Apr 30, 2021

GuillaumeGomez commented May 1, 2021 •

edited

Loading

jsha commented May 1, 2021

GuillaumeGomez commented May 1, 2021

jsha commented May 1, 2021

GuillaumeGomez commented Jun 2, 2021

jsha commented Jun 2, 2021

bors commented Jun 2, 2021

bors commented Jun 2, 2021

bors commented Jun 3, 2021

GuillaumeGomez commented Jun 3, 2021

	"<div id=\"{}\" class=\"{}{}{}\">",
	"<div id=\"{}\" class=\"{}{}{}\" role="heading" aria-level="4">",

Clean up dom #84703

Clean up dom #84703

Conversation

GuillaumeGomez commented Apr 29, 2021 • edited Loading

rust-highfive commented Apr 29, 2021

GuillaumeGomez Apr 29, 2021

Choose a reason for hiding this comment

This comment has been minimized.

jsha left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 29, 2021

notriddle Apr 29, 2021

Choose a reason for hiding this comment

GuillaumeGomez Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez Apr 29, 2021

Choose a reason for hiding this comment

notriddle Apr 29, 2021

Choose a reason for hiding this comment

bors commented Apr 30, 2021

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021 • edited Loading

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021 • edited Loading

GuillaumeGomez commented Apr 30, 2021

jsha commented Apr 30, 2021

jsha commented Apr 30, 2021

notriddle commented Apr 30, 2021 • edited Loading

jsha commented Apr 30, 2021

notriddle commented Apr 30, 2021

GuillaumeGomez commented May 1, 2021 • edited Loading

jsha commented May 1, 2021

GuillaumeGomez commented May 1, 2021

jsha commented May 1, 2021

GuillaumeGomez commented Jun 2, 2021

jsha commented Jun 2, 2021

bors commented Jun 2, 2021

bors commented Jun 2, 2021

bors commented Jun 3, 2021

GuillaumeGomez commented Jun 3, 2021

GuillaumeGomez commented Apr 29, 2021 •

edited

Loading

GuillaumeGomez Apr 29, 2021 •

edited

Loading

jsha commented Apr 30, 2021 •

edited

Loading

jsha commented Apr 30, 2021 •

edited

Loading

notriddle commented Apr 30, 2021 •

edited

Loading

GuillaumeGomez commented May 1, 2021 •

edited

Loading