Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 24, 2021
Merged

Fix clicking on anchors inside summary tags #91188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustdoc/html/static/js/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

e.preventDefault();
}
});
Expand Down
4 changes: 4 additions & 0 deletions src/test/rustdoc-gui/toggle-click-deadspace.goml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@ click: "h4.code-header" // This is the position of "pub" in "pub fn a_method"
assert-attribute: (".impl-items .rustdoc-toggle", {"open": ""})
click: ".impl-items .rustdoc-toggle summary::before" // This is the position of "[-]" next to that pub fn.
assert-attribute-false: (".impl-items .rustdoc-toggle", {"open": ""})

// Click the "Trait" part of "impl Trait" and verify it navigates.
click: "#impl-Trait h3 a:first-of-type"
assert-text: (".fqn .in-band", "Trait lib2::Trait")