Closed Bug 1419088 Opened 2 years ago Closed 2 years ago

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

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: mpark)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

No description provided.
Blocks: 1380501
Severity: normal → enhancement
Priority: -- → P2
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
Attachment #8937721 - Flags: review?(bgrinstead)
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.
Attachment #8937721 - Flags: review?(bgrinstead)
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?
Attachment #8937721 - Flags: review?(bgrinstead)
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!
Attachment #8937721 - Flags: review?(bgrinstead) → review+
Assignee: nobody → mpark
Status: NEW → ASSIGNED
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4bfd034c0ee
Hitting the Esc key on the console when sidebar is open should close it. r=bgrins
Priority: P2 → P1
Whiteboard: [newconsole-mvp]
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/40ee4c5df6dd
Hitting the Esc key on the console when sidebar is open should close it. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/40ee4c5df6dd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.