Implement sidebar toggle button in Net Panel Toolbar

VERIFIED FIXED in Firefox 52

Status

DevTools
Netmonitor
P1
normal
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: steveck, Assigned: gasolin@mozilla.com)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [netmonitor])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
A subitem for Bug #1308426 that need to implement sidebar toggle button in Net Panel Toolbar.

Updated

2 years ago
Blocks: 1307743

Updated

2 years ago
Whiteboard: [netmonitor]

Updated

2 years ago
Flags: qe-verify+
QA Contact: ciprian.georgiu
(Assignee)

Updated

2 years ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
The UI element in Bug 1309193 (toggle button) & Bug 1309194 (summary text) are pretty simple, which contain 1 button or 1 text field.
https://github.com/mozilla/gecko-dev/blob/7e0678438a65e310f2392ca79b77cba6ec08f7f4/devtools/client/netmonitor/netmonitor.xul#L167

https://github.com/mozilla/gecko-dev/blob/7e0678438a65e310f2392ca79b77cba6ec08f7f4/devtools/client/netmonitor/netmonitor.xul#L162

I think we may able to directly render them in toolbar Bug 1308426 and invalid these 2 bugs?
Flags: needinfo?(odvarko)
I'd try to migrate the toggle button to React (this bug) first and see how simple it really is. I recall there were some troubles in the Inspector panel. Note that you also need Redux action and Redux state (side-bar visible/hidden).

You might use the Inspector panel as source of inspiration (no Redux in the Inspector panel though). 

Later, we can even share the component between these two panels. However it should be done as a follow up.

Honza
Flags: needinfo?(odvarko)

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Iteration: --- → 52.3 - Nov 7

Updated

2 years ago
Depends on: 1309191
Blocked on the resolution of bug 1309191.
Blocking dependency (bug 1309191) resolved.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

https://reviewboard.mozilla.org/r/88796/#review87986

::: devtools/client/locales/en-US/netmonitor.properties
(Diff revision 3)
>  # in the network toolbar for the performance analysis button.
>  netmonitor.toolbar.perf=Toggle performance analysis…
>  
> -# LOCALIZATION NOTE (netmonitor.panesButton.tooltip): This is the tooltip for
> -# the button that toggles the panes visible or hidden in the netmonitor UI.
> -netmonitor.panesButton.tooltip=Toggle network info

there's no non-state in new toggleButton, so remove that default status message

::: devtools/client/netmonitor/components/toggle-button.js:55
(Diff revision 3)
> +  }),
> +  (dispatch) => ({
> +    onToggle: () => {
> +      dispatch(Actions.toggleSidebar());
> +
> +      let requestsMenu = NetMonitorView.RequestsMenu;

the following `requestsMenu.selectedIndex` could be managed by redux once requestsMenu is migrated.

::: devtools/client/netmonitor/netmonitor-view.js:178
(Diff revision 3)
> -
> -    ViewHelpers.togglePane(flags, pane);
>  
>      if (flags.visible) {
>        this._body.classList.remove("pane-collapsed");
> -      button.classList.remove("pane-collapsed");
> +      gStore.dispatch(Actions.showSidebar(true));

detailPaneToggleButton related actions are managed by redux

::: devtools/client/netmonitor/toolbar-view.js
(Diff revision 3)
>   * Functions handling the toolbar view: expand/collapse button etc.
>   */
>  function ToolbarView() {
>    dumpn("ToolbarView was instantiated");
> -
> -  this._onTogglePanesPressed = this._onTogglePanesPressed.bind(this);

onTogglePanesPressed related actions are managed by react container component (toggle-button)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

https://reviewboard.mozilla.org/r/88796/#review88404

Nice!

Except of a small nit all look good to me.

R+ assuming Try is green (win build failed in the current try push)

There is one thing. Opening and closing the sidebar clears the current selection.
Since this issue has been there already before it sounds like candidate for
a follow up.

Thanks!
Honza

::: devtools/client/netmonitor/components/toggle-button.js:18
(Diff revision 3)
> +const Actions = require("../actions/index");
> +
> +// Shortcuts
> +const { button } = DOM;
> +
> +function ToggleButton({

nit: create a comment explaining why we have this component.
Attachment #8804993 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> There is one thing. Opening and closing the sidebar clears the current
> selection.
> Since this issue has been there already before it sounds like candidate for
> a follow up.

This is how the Netmonitor sidebar works and I'm not sure it's a bug. The "sidebar opened/closed" and the "toggle button enabled/disabled" are not independent pieces of information, but can be derived from the request list state:

- sidebar is opened if and only if some request is selected. Selecting a request always opens the sidebar. Closing the sidebar always clears the selection.

- toggle button is enabled if and only if there are >0 requests in the request list.

(There is buggy behavior when filters are applied - the filtered list can be empty, and a sidebar can be still displayed - an orphaned one with no corresponding row visible in the requests list)

Whether this behavior is desired in long-term is of course open for discussion, but I wouldn't change it as a part of this refactoring project.

A corrolary: In the fully refactored Redux store, there will be no "sidebar.visible" or "sidebar.toggleButtonDisabled" state variables. These values can be computed from the request list state.
(In reply to Jarda Snajdr [:jsnajdr] from comment #10)
> (In reply to Jan Honza Odvarko [:Honza] from comment #9)
> > There is one thing. Opening and closing the sidebar clears the current
> > selection.
> > Since this issue has been there already before it sounds like candidate for
> > a follow up.
> 
> This is how the Netmonitor sidebar works and I'm not sure it's a bug. The
> "sidebar opened/closed" and the "toggle button enabled/disabled" are not
> independent pieces of information, but can be derived from the request list
> state:
> 
> - sidebar is opened if and only if some request is selected. Selecting a
> request always opens the sidebar. Closing the sidebar always clears the
> selection.
I am still not convinced why closing the sidebar should clear the current selection. Anyways, I'll put this on a list of questions for Helen.

> 
> - toggle button is enabled if and only if there are >0 requests in the
> request list.
> 
> (There is buggy behavior when filters are applied - the filtered list can be
> empty, and a sidebar can be still displayed - an orphaned one with no
> corresponding row visible in the requests list)
> 
> Whether this behavior is desired in long-term is of course open for
> discussion, but I wouldn't change it as a part of this refactoring project.
Yes, definitely. This should be a follow up.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> > - sidebar is opened if and only if some request is selected. Selecting a
> > request always opens the sidebar. Closing the sidebar always clears the
> > selection.
> I am still not convinced why closing the sidebar should clear the current
> selection. Anyways, I'll put this on a list of questions for Helen.

Some observations from other devtools components:
- Storage inspector behaves the same way - item is selected iff its sidebar view is opened
- Inspector keeps the selected element selected even after the sidebar is closed
- Memory and Performance UI is "unfinished" - there's no way to unselect an item or close the sidebar once it's opened

There's another question - what's the point of making items selectable in the first place?
- in Netmonitor and Storage, the sole purpose is to select the item to be displayed in sidebar. Other than that, selection doesn't do anything. (Context menu doesn't really need it) And it's a simple list, not a tree.
- in Inspector, the selected item's box is highlighted in the content, and the DOM tree can be navigated with keyboard (expanded, collapsed). Selection has many more functions here.
- in Memory and Performance tools, there are also tree lists, not simple lists.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

https://reviewboard.mozilla.org/r/88796/#review88856

Looks good!
Attachment #8804993 - Flags: review?(jsnajdr) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2279cd89545e
Implement sidebar toggle button in Net Panel Toolbar;r=Honza,jsnajdr
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2279cd89545e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1315224
I can confirm that this bug is verified fixed on latest Nightly 52.a1 (2016-11-08) under the following platforms:
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86 LTS
There are no issues on sidebar toggle button in Net Panel Toolbar and based on that, I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.