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: Fix item info display overflow #95684

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

GuillaumeGomez
Copy link
Member

I came across this issue when reading local Iterator docs (reproduced in this screenshot):

Screenshot from 2022-04-05 17-45-13

The problem comes from the fact that span isn't display: block by default. Since item-info was already present on <div> in other places, I moved the last one to div as well.

r? @notriddle

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 5, 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 Apr 5, 2022
@GuillaumeGomez GuillaumeGomez force-pushed the fix-item-info-overflow branch from 027f12c to 41b0247 Compare April 5, 2022 16:11
@GuillaumeGomez GuillaumeGomez added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Apr 5, 2022
@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit 41b0247 has been approved by notriddle

@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 Apr 5, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2022
Rollup of 4 pull requests

Successful merges:

 - rust-lang#95659 (Rely on #[link] attribute for unwind on Fuchsia.)
 - rust-lang#95684 (rustdoc: Fix item info display overflow)
 - rust-lang#95693 (interp: pass TyCtxt to Machine methods that do not take InterpCx)
 - rust-lang#95699 (fix: Vec leak when capacity is 0)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 84e1aa2 into rust-lang:master Apr 6, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 6, 2022
@GuillaumeGomez GuillaumeGomez deleted the fix-item-info-overflow branch April 6, 2022 09:25
@jsha
Copy link
Contributor

jsha commented Apr 6, 2022

I think this is not the right fix. I originally changed this from a <div> to a <span> in 32f62607c314 in order to make our HTML valid. The item-info appears inside an <summary>, and those are not allowed to contain <div>. The idea there was to apply display: block to the span.

If the <span> is not getting display: block maybe we could fix the CSS instead?

@notriddle
Copy link
Contributor

I'd be fine with that, but if we're going to pursue valid HTML, then we really should also add automated tests to ensure we don't accidentally regress.

@GuillaumeGomez
Copy link
Member Author

I think this is not the right fix. I originally changed this from a <div> to a <span> in 32f62607c314 in order to make our HTML valid. The item-info appears inside an <summary>, and those are not allowed to contain <div>. The idea there was to apply display: block to the span.

If the <span> is not getting display: block maybe we could fix the CSS instead?

I went with that originally but ended up with switching to div for coherency. I'll make it the other way then with a fix to the CSS.

@jsha
Copy link
Contributor

jsha commented Apr 6, 2022

if we're going to pursue valid HTML, then we really should also add automated tests to ensure we don't accidentally regress.

Totally agreed; I took a shortcut on the previous PR because it seemed like it was going to be hard to get htmltidy (https://github.com/htacg/tidy-html5) integrated into the CI runners. But I should do the work and get it done. :-)

@GuillaumeGomez
Copy link
Member Author

I opened #95738 to switch all item-info items to span and made the CSS change.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 6, 2022
…span, r=jsha

Switch item-info from div to span

Following discussion in rust-lang#95684.

cc `@jsha`
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) 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.

6 participants