Skip to content

Fix escape handling #85438

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 2 commits into from
May 19, 2021
Merged

Fix escape handling #85438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
281dea4
Fix escape handling on search results
GuillaumeGomez May 18, 2021
d314b06
Add GUI tests for escape handling
GuillaumeGomez May 18, 2021
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
4 changes: 2 additions & 2 deletions src/librustdoc/html/static/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,9 +428,9 @@ function hideThemeButtonState() {
function handleEscape(ev) {
var help = getHelpElement(false);
var search = searchState.outputElement();
if (!hasClass(help, "hidden")) {
if (help && !hasClass(help, "hidden")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasClass is defined like so:

function hasClass(elem, className) {
    return elem && elem.classList && elem.classList.contains(className);
}

So I think this should be redundant. Can you explain (in the PR description) what the bug was and how this fixes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's simply because if the element doesn't exist, it doesn't have the class and enter the condition. :)

Copy link
Contributor

@pickfire pickfire May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other cases? I don't see the first help && part, some of them have this and some does not. Seemed confusing to me.

Others may easily make a mistake here later. I see quite a few hasClass and roughly half of them have the first help && .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, now I see the issue. The problem is that these if clauses are exclusive. So we were mistakenly executing the "hide help" clause, which prevented us from executing the "hide search" clause.

What about making both of these clauses unconditional? If you hit escape, it's reasonable to try and hide the help, and also try and hide the search. Worst case, it's a no-op.

Relatedly: Why does only the search clause have a preventDefault()? Do we need a preventDefault() at all here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we might want to close the help without closing the search results (my GUI test actually enforces this behaviour). Also, this change is mostly because of the migration of the boolean comparison. With === false, it wouldn't enter because hasClass would return null.

As for preventDefault, no idea. Can be removed I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I fully understand. Thanks for explaining.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pickfire Well, not really, in case we want to check that if the element exists, it's hidden, if it doesn't exist then it's all good. You can see it as follows too: if the element doesn't exist, it doesn't implement the class. 😃

@jsha I'll open an issue to make another cleanup round around hasClass usage just in case.

displayHelp(false, ev, help);
} else if (!hasClass(search, "hidden")) {
} else if (search && !hasClass(search, "hidden")) {
searchState.clearInputTimeout();
ev.preventDefault();
searchState.hideResults(search);
Expand Down
27 changes: 27 additions & 0 deletions src/test/rustdoc-gui/escape-key.goml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
goto: file://|DOC_PATH|/test_docs/index.html
// First, we check that the search results are hidden when the Escape key is pressed.
write: (".search-input", "test")
wait-for: "#search > h1" // The search element is empty before the first search
assert: ("#search", "class", "content")
assert: ("#main", "class", "content hidden")
press-key: "Escape"
assert: ("#search", "class", "content hidden")
assert: ("#main", "class", "content")

// Check that focusing the search input brings back the search results
focus: ".search-input"
assert: ("#search", "class", "content")
assert: ("#main", "class", "content hidden")

// Now let's check that when the help popup is displayed and we press Escape, it doesn't
// hide the search results too.
click: "#help-button"
assert: ("#help", "class", "")
press-key: "Escape"
assert: ("#help", "class", "hidden")
assert: ("#search", "class", "content")
assert: ("#main", "class", "content hidden")

// FIXME: Once https://github.com/rust-lang/rust/pull/84462 is merged, add check to ensure
// that Escape hides the search results when a result is focused.
// press-key: "ArrowDown"