Closed Bug 1003153 Opened 6 years ago Closed 7 months ago

Add keyboard shortcut to hide sidebar

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: rcampbell, Assigned: dhyey35, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 6 obsolete files)

We currently only have the small "Sidebar Close" button in the toolbar to close the sidepanel (net request/response details view). We should make this easier.

I suggest using the ESC key to close the sidebar.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [mentor=robcee][lang=js]
Can't use ESC since that is the toggle split console key.
We already override that at many places like closing web console sidebar, in responsive view, etc. (but only when there is a need to override)
Hi Rob,
I am interested to try out this one. But before that, could you please tell me that are we talking about the "Hide Request details" button in the sidebar and the action associated with it? Also;as I am new to firefox development if you can guide as to the files associated with Netmonitor in the source then I would try to understand the code which would make it easier when we actually arrive at an alternative solution. 

Thanks!
Flags: needinfo?(nobody)
Flags: needinfo?(nobody) → needinfo?(rcampbell)
Hi Priyank,

If you haven't already, take a look in https://wiki.mozilla.org/DevTools/Hacking. It should help you get set up and running.

The button in question is pointed to in this screenshot:

https://www.evernote.com/shard/s2/sh/013c3b9d-eeca-4b33-ac2c-8fe381570189/8652c5cde20268eeb4633bd658ee41b7/res/c18f9c2a-2587-422d-bd96-f08134801ef6/skitch.png

When you click that, the sidebar closes.

the button is defined:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor.xul#129

and the sidebar is defined in (search for SideBarView and toggle):

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js

Nick rightly points out that the ESC key is used for the split-console. Playing around a bit more with the Network Monitor, the next keys I tried were the arrow and enter key. Currently the enter key doesn't do anything (unless you're editing a request). We could try using that but making sure we don't mess up editing responses and requests.

Make sure to use the NeedInfo flag below to ask for help or find me in irc://irc.mozilla.org/#devtools.

happy hacking.
Flags: needinfo?(rcampbell)
Mentor: rcampbell
Whiteboard: [mentor=robcee][lang=js] → [lang=js]
I see that this thread hasn't been active for a few months. On the latest nightly, I couldn't find any shortcut.

1.) Is this still available or are you [Priyank] still working on it? If you are [Priyank], feel free to do so. If not, I think I can handle this as my first bug.
2.) Do we need a UX person to give input on what key to use? If they want to override ESC, how would I do that? I think adding something such as CTRL+ESC would still preserve the usage of the ESC key while still having a way to close out of it. I would also add a tooltip to the close button so it says `Close Developer Tools (CTRL-ESC)`.

I noticed that this is more of a developer tools issue than just the netmonitor.

Any help would be appreciated!
(In reply to Annonomus Penguin from comment #5)
> 1.) Is this still available or are you [Priyank] still working on it? If you
> are [Priyank], feel free to do so. If not, I think I can handle this as my
> first bug.

Since no patch was ever attached and the bug is unassigned, feel free to go for it! :)

For your other questions, I'd suggest flagging robcee (use needinfo below) or asking in #devtools.
Attached patch close.patch (obsolete) — Splinter Review
I haven't tested the patch , made few changes , just want to know whether i am in right track :)
Flags: needinfo?(rcampbell)
Comment on attachment 8450247 [details] [diff] [review]
close.patch

Review of attachment 8450247 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/netmonitor/netmonitor.xul
@@ +22,5 @@
>    <script type="text/javascript" src="netmonitor-view.js"/>
>  
> +  <keyset id="netmonitorKeys">
> +    <key id="addSidebarCloseKey"
> +         keycode="VK_RIGHT VK_ENTER"

you can only have one value in the keycode attribute. You're looking for ESC here, aren't you?

@@ +26,5 @@
> +         keycode="VK_RIGHT VK_ENTER"
> +         command="sidebarCloseCommand"/>
> +  </keyset>
> +
> +  <command id="netmonitorCommand">

I you want this to be a <commandset>. Rename the id to "netmonitorCommands" since it can have more than just one.

@@ +28,5 @@
> +  </keyset>
> +
> +  <command id="netmonitorCommand">
> +    <command id="sidebarCloseCommand"
> +             oncommand="SidebarView.Options.toggle()"/>

this looks like it should work.
Flags: needinfo?(rcampbell)
Assignee: nobody → lviknesh
Attached patch sidebar-close.patch (obsolete) — Splinter Review
pressing ESC still closes toggle split console , is there anything i am doing wrong ?? 
Thank you :)
Attachment #8450247 - Attachment is obsolete: true
Flags: needinfo?(rcampbell)
Flags: needinfo?(rcampbell)
Attached patch arrows-WIP (obsolete) — Splinter Review
Patch adds arrow key controls to the netmonitor view.

←/⏎ - expand sidebar.
→   - collapse sidebar.

Needs tests.
Assignee: lviknesh → rcampbell
Attachment #8456292 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Mentor: rcampbell → vporof
Attached patch arrows (obsolete) — Splinter Review
added some tests.
Attachment #8470204 - Attachment is obsolete: true
Attachment #8470983 - Flags: review?(vporof)
Comment on attachment 8470983 [details] [diff] [review]
arrows

Review of attachment 8470983 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +741,4 @@
>      }
>    },
>  
> +  _handleKeyPress: function(aName, aEvent) {

Nit: please document this event handler.

@@ +741,5 @@
>      }
>    },
>  
> +  _handleKeyPress: function(aName, aEvent) {
> +    switch(aEvent.keyCode) {

Nit: space between switch and (

@@ +1568,4 @@
>     * The selection listener for this container.
>     */
>    _onSelect: function({ detail: item }) {
> +    if (this.arrowClosed)

Nit: curly braces everywhere.

::: browser/devtools/netmonitor/test/browser_net_accessibility-02.js
@@ +114,4 @@
>        EventUtils.sendKey("DOWN", window);
>        check(19, true);
>  
> +      debugger;

Leftover. Remove this.

@@ +120,4 @@
>  
> +      // selectedIndex is getting reset to 0 here?
> +      // I guess because we're clicking inside the side-menu-widget-item.
> +      // Not sure what the original intent of this test was.

The initial intention here was to check if an item is selected in the table when clicking on one while the sidebar is closed.

Remove this comment.
Attachment #8470983 - Flags: review?(vporof) → review+
Attached patch arrows (obsolete) — Splinter Review
Attachment #8470983 - Attachment is obsolete: true
Attachment #8474570 - Flags: review+
Attached patch arrows (obsolete) — Splinter Review
fixed a test failure in browser_net_pane-toggle.js found in try.
Attachment #8474570 - Attachment is obsolete: true
Attachment #8474625 - Flags: review+
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/7ee849f1c0c4
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][land-in-fx-team] → [lang=js][fixed-in-fx-team]
Flags: needinfo?(rcampbell)
Assignee: rcampbell → nobody
Status: ASSIGNED → NEW
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Is this bug still alive ? 

Can we have the following.
- Escape: close sidebar 
- Shift + Escape: open the console
Mentor: vporof → odvarko
Summary: Make it easier to close the sidebar → Add keyboard shortcut to hide sidebar
I think a function key to "open/close sidebar" would be appropriate, now that it is a proper action just to open or close the sidebar from the toolbar. I saw [F9] as a possible key to do it. Currently has no function in Firefox.

So I would recommend that [F9] become the "open/close sidebar" keyboard shortcut.

What do y'all think?
It used to be [F9], but it broke 14years ago as per https://bugzilla.mozilla.org/show_bug.cgi?id=230023.

I'd suggest [F4] to match other browsers.
Product: Firefox → DevTools

Hey,
Is this issue still open, if yes, I would like to work on it?

I chose to have just Esc key as I checked chrome and they are doing the same. When details view is open Esc will close and to open console user will have to press Esc when details view is not open ( Same behaviour as chrome )

Flags: needinfo?(odvarko)

(In reply to Paarmita Bhargava from comment #21)

Hey,
Is this issue still open, if yes, I would like to work on it?

Yes, thanks for helping!

Honza

Flags: needinfo?(odvarko)

(In reply to dhyey35 from comment #22)

@dhyey35: You need to claim the bug first before start working on it. I had to assign it to Paarmita Bhargava since she asked first, sorry.

Honza

I think that parity with Chrome is good idea here. So, ESC works for me.

Honza

Assignee: nobody → paarmita1998
Priority: -- → P3
Status: NEW → ASSIGNED

(In reply to dhyey35 from comment #23)

I chose to have just Esc key as I checked chrome and they are doing the same. When details view is open Esc will close and to open console user will have to press Esc when details view is not open ( Same behaviour as chrome )

Paarmita just replied on Slack expressing no interest, so assigning the bug to you.
(please let me know in case you are not interested anymore)

Thanks!
Honza

Assignee: paarmita1998 → dhyey35

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #25)

(In reply to dhyey35 from comment #22)

@dhyey35: You need to claim the bug first before start working on it. I had to assign it to Paarmita Bhargava since she asked first, sorry.

Honza

Sorry, will take care :)

Will fix the whitespace issue and update the code.

Also should I write tests for this ?

Flags: needinfo?(odvarko)

(In reply to dhyey35 from comment #28)

Also should I write tests for this ?

Yes, please!

Honza

Flags: needinfo?(odvarko)

Honza can you please review https://bugzilla.mozilla.org/show_bug.cgi?id=1258809. Writing tests for this depends on that file.

Flags: needinfo?(odvarko)

(In reply to dhyey35 from comment #30)

Honza can you please review https://bugzilla.mozilla.org/show_bug.cgi?id=1258809. Writing tests for this depends on that file.
Done

Honza

Flags: needinfo?(odvarko)

@dhyey35: this is very close to landing, did you see my comments in Phabricator?

Honza

Flags: needinfo?(dhyey35)

Hi Honza, Yes I have read the comments, I am waiting for https://bugzilla.mozilla.org/show_bug.cgi?id=1258809 to get merged so that I can make the changes suggested by you and also add a test.

Or Should I add test in an another bug ?

Flags: needinfo?(dhyey35) → needinfo?(odvarko)

True. Good news is that I landed bug 1258809 today :-)

Honza

Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d021cea258c7
Add keyboard shortcut to hide sidebar. r=Honza
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.