-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix clicking on anchors inside summary tags #91188
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
Conversation
Some changes occurred in HTML/CSS/JS. |
@bors r+ |
📌 Commit 04cb0dc has been approved by |
@@ -888,7 +888,7 @@ function hideThemeButtonState() { | |||
|
|||
onEachLazy(document.querySelectorAll(".rustdoc-toggle > summary:not(.hideme)"), function(el) { | |||
el.addEventListener("click", function(e) { | |||
if (e.target.tagName != "SUMMARY") { | |||
if (e.target.tagName != "SUMMARY" && e.target.tagName != "A") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely an improvement, but it seems somewhat fragile. Are there other tags that are used as click targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buttons and forms have a default behavior on click, but those don't show up in our summaries. You're right that this feels fragile, though. Perhaps as a followup we should flip the logic and say "preventDefault if the click target is h3
, h4
, etc". That seems to be the usual click target in these when it's not the anchor, but need to do a little testing to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick check shows we'd also have to cover <div class="stab unstable">
and friends - the item-info divs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but those don't show up in our summaries.
A quick check shows we'd also have to cover
<div class="stab unstable">
and friends - the item-info divs.
I thought item-infos don't show up in the summaries either? Or do they and I'm misunderstanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item-infos are currently inside the summary. The idea is that they should be shown even when the whole document is collapsed, so you can see at a glance what is deprecated, unstable, platform-specific, etc.
Those and the "notable traits" hover div are the main surprising things that are inside summaries right now.
We want this to get in before nightly becomes beta: @bors p=1 |
☀️ Test successful - checks-actions |
Finished benchmarking commit (b426445): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Fixes #91184
r? @camelid