1419088 - Hitting the Esc key on the console when sidebar is open should close it

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Comment 1

[Mike Park [:mparkms]]

Attachment: Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

Review commit: https://reviewboard.mozilla.org/r/208442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208442/

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

https://reviewboard.mozilla.org/r/208442/#review214240

There's one regression I see here compared with the old frontend:

1. Inspect a variable in the sidebar
2. Focus jsterm, type `window.` so that the autocomplete popup opens
3. Press ESC

In the old frontend the autocomplete popup closes but the sidebar stays open until you press ESC again. In the new frontend the sidebar closes as well. Can you take a look and see if that's an easy fix? The autocomplete popup closes around here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/jsterm.js#1165

::: devtools/client/locales/en-US/webconsole.properties:203
(Diff revision 1)
>  # Key shortcut used to close the Browser console (doesn't work in regular web console)
>  webconsole.close.key=CmdOrCtrl+W
>  
> +# LOCALIZATION NOTE (webconsole.closeSidebar.key)
> +# Key shortcut used to close the console sidebar.
> +webconsole.closeSidebar.key=Esc

I don't see value in localizing this string. Conventionally, we use ESC hardcoded to close things in the UI (like the autocomplete popup), so I suspect this is more likely to accidentally be changed per locale than on purpose.  I'd suggest we inline this both in the test and in webconsole.js

Comment 3

[Mike Park [:mparkms]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208442/diff/1-2/

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

https://reviewboard.mozilla.org/r/208442/#review214270

::: devtools/client/webconsole/new-webconsole.js:258
(Diff revision 2)
>                     this.window.close.bind(this.window));
>  
>        ZoomKeys.register(this.window);
> +    } else if (Services.prefs.getBoolPref(PREF_SIDEBAR_ENABLED)) {
> +      shortcuts.on("Esc", (name, event) => {
> +        if (event.target.className.includes("jsterm-input-node")) {

I looked into this a bit closer and I think we can accomplish the desired behavior with something like this as the callback to the key shortcut here:

```
if (!this.jsterm.autocompletePopup || !this.jsterm.autocompletePopup.isOpen) {
  this.newConsoleOutput.dispatchSidebarClose();
}
```

And then revert all changes in jsterm.js. I think this'll be the best option for now.

Comment 5

[Mike Park [:mparkms]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208442/diff/2-3/

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

https://reviewboard.mozilla.org/r/208442/#review214286

::: devtools/client/webconsole/jsterm.js:1170
(Diff revision 3)
>        case KeyCodes.DOM_VK_ESCAPE:
>          if (this.autocompletePopup.isOpen) {
>            this.clearCompletion();
>            event.preventDefault();
>            event.stopPropagation();
> -        } else if (this.sidebar) {
> +        } else if (this.sidebar || this.hud.NEW_CONSOLE_OUTPUT_ENABLED) {

I don't think the changes in this file are necessary anymore, are they?

Comment 7

[Mike Park [:mparkms]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208442/diff/3-4/

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8937721 [details]
Bug 1419088 - Hitting the Esc key on the console when sidebar is open should close it.

https://reviewboard.mozilla.org/r/208442/#review214300

Works for me with a green try push!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4bfd034c0ee
Hitting the Esc key on the console when sidebar is open should close it. r=bgrins

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/40ee4c5df6dd
Hitting the Esc key on the console when sidebar is open should close it. r=bgrins

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/40ee4c5df6dd

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/c4bfd034c0ee