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

Show triangle on the "Details" disclosure element in all cases #86589

Merged
merged 2 commits into from
Jun 28, 2021

Conversation

JohnTitor
Copy link
Member

Re-submission of #82805, fixes the style issue by applying the same margin as <p>.

Before

before 1
before 2

After

after 1
after 2

r? @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2021
@GuillaumeGomez
Copy link
Member

Thanks! What would be nice would be to have a GUI test for it, but not sure if it's actually possible...

Any idea @jsha? If not possible, then let's simply r+ it.

@jsha
Copy link
Contributor

jsha commented Jun 24, 2021

This strikes me as possibly the wrong fix. I think what's going on is that we have some CSS that hides the triangle for rustdoc's built-in toggles, since we want to use a ::before pseudo-element with content: [+] for that. But I guess that CSS is also hiding the triangle for <details> tags written in Markdown by the doc author. Can we make rustdoc's own CSS rules more specific to .rustdoc-toggle so it doesn't interfere with user-authored HTML?

Co-authored-by: Miguel Ojeda <ojeda@kernel.org>
@JohnTitor
Copy link
Member Author

JohnTitor commented Jun 25, 2021

This strikes me as possibly the wrong fix. I think what's going on is that we have some CSS that hides the triangle for rustdoc's built-in toggles, since we want to use a ::before pseudo-element with content: [+] for that. But I guess that CSS is also hiding the triangle for <details> tags written in Markdown by the doc author.

That's a good point, pushed another fix.

Can we make rustdoc's own CSS rules more specific to .rustdoc-toggle so it doesn't interfere with user-authored HTML?

I'm not familiar with rustdoc's CSS but I think the below should've worked here:

details.rustdoc-toggle > summary, details.undocumented > summary {
list-style: none;
}

But it does hide the normal summary's triangles, any idea?

@jsha
Copy link
Contributor

jsha commented Jun 25, 2021

Okay, I looked into this and it turns out the issue is actually a bug in the very old version of normalize.css we use (3.0.0, from 2014). In #82313 I tried to update to the latest, 8.0.1. But it was reverted in #82549 because of these regressions:

#82548
#82542

See these demos that show it is a problem with normalize.css:

https://jsbin.com/jorowazuli/edit?html,output

https://jsbin.com/kecozayaka/edit?html,output

The right path forward is to figure out why upgrading normalize.css caused those regressions, fix the issues, and then do the upgrade. Alternately, we may conclude that we no longer need normalize.css, which would be an even nicer outcome.

@JohnTitor
Copy link
Member Author

Thanks for investigating! Hmm, but given that we should take a look at it carefully and reviewing will taker a longer time not to regress anymore, I think this PR is still worth landing as a stopgap, no?
Though, I'm happy to work on upgrading normalize.css if you could provide some help, or I'm also happy if you would work on it.

@jsha
Copy link
Contributor

jsha commented Jun 27, 2021

You're right, it does make sense to land this as a stopgap. The current version looks good but I want to make sure we know to remove it after normalize.css is either upgraded or removed. Can you add a comment that says "Remove after normalize.css is either upgraded or removed (#86629)"?

@JohnTitor
Copy link
Member Author

Sure, added.

@jsha
Copy link
Contributor

jsha commented Jun 27, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2021

📌 Commit 14413ad has been approved by jsha

@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 Jun 27, 2021
@bors
Copy link
Contributor

bors commented Jun 27, 2021

⌛ Testing commit 14413ad with merge 3e9d7ec...

@bors
Copy link
Contributor

bors commented Jun 28, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 28, 2021
@bors bors merged commit 3e9d7ec into rust-lang:master Jun 28, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 28, 2021
@JohnTitor JohnTitor deleted the add-triangle-to-summary branch June 28, 2021 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants