1309193 - Implement sidebar toggle button in Net Panel Toolbar

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Steve Chung [:steveck]]

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

Comment 1

[[e-mail]]

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?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 3

[Marco Mucci [:MarcoM]]

Blocked on the resolution of bug 1309191.

Comment 4

[Marco Mucci [:MarcoM]]

Blocking dependency (bug 1309191) resolved.

Comment 5

[[e-mail]]

Attachment: Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

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

Comment 6

[[e-mail]]

Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

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

Comment 7

[[e-mail]]

Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

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

Comment 8

[[e-mail]]

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

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 10

[Jarda Snajdr [:jsnajdr]]

(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.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 12

[Jarda Snajdr [:jsnajdr]]

(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

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

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

Looks good!

Comment 14

[[e-mail]]

Comment on attachment 8804993 [details]
Bug 1309193 - Implement sidebar toggle button in Net Panel Toolbar;

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2279cd89545e
Implement sidebar toggle button in Net Panel Toolbar;r=Honza,jsnajdr

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2279cd89545e

Comment 17

[Ciprian Georgiu, Desktop QA]

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.