Fix escape handling #85438
Fix escape handling #85438
Conversation
|
Some changes occurred in HTML/CSS/JS. |
rust-highfive
added
the
S-waiting-on-review
| @@ -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")) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It's simply because if the element doesn't exist, it doesn't have the class and enter the condition. :)
There was a problem hiding this comment.
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 && .
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Now I fully understand. Thanks for explaining.
There was a problem hiding this comment.
@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.
|
@bors r+ |
|
📌 Commit d314b06 has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup |
… r=jsha Fix escape handling Currently, when we press Escape while on the search results, nothing is happening, this PR fixes it. More information: it's because in case the element doesn't exist, `hasClass` will return `null`, which coerces into `false` with the `!` comparison operator. But even if it returned `false`, it would still be an issue because if the element doesn't exist, it means it's hidden so in this case it's just as good, hence the additional check I added. r? `@jsha`
Rollup of 8 pull requests Successful merges: - rust-lang#83366 (Stabilize extended_key_value_attributes) - rust-lang#83767 (Fix v0 symbol mangling bug) - rust-lang#84883 (compiletest: "fix" FileCheck with --allow-unused-prefixes) - rust-lang#85274 (Only pass --[no-]gc-sections if linker is GNU ld.) - rust-lang#85297 (bootstrap: build cargo only if requested in tools) - rust-lang#85396 (rustdoc: restore header sizes) - rust-lang#85425 (Fix must_use on `Option::is_none`) - rust-lang#85438 (Fix escape handling) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Currently, when we press Escape while on the search results, nothing is happening, this PR fixes it.
More information: it's because in case the element doesn't exist,
hasClasswill returnnull, which coerces intofalsewith the!comparison operator. But even if it returnedfalse, it would still be an issue because if the element doesn't exist, it means it's hidden so in this case it's just as good, hence the additional check I added.r? @jsha