1101569 - Add an "all items" menu to the sidebar in cases of overflow

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

STR:

- Open the devtools
- Switch to either the inspector or the netmonitor
- Resize the sidebar panel to be as small as possible

==> Not all sidebar tabs are visible, and there is no way to access the hidden items.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1101569-sidebar-allmenu-button v1.patch

This is based on a very old patch I did more than 1 year ago when wanting to add a DOM view panel to the inspector.
I've rebased it and adapted it a little bit to our new CSS.

I'm not big on XUL, so some feedback on this from Victor would help a lot I think.

Here's what the patch contains:

- The ToolSidebar now has a way to add a '...' button at the end of the tabbox whenever there isn't enough space to show all tabs. This button lists all existing tabs and allows to switch from one to the other.
- To make this happen, I refactored a little bit the ToolSidebar class to accept an options object instead.
- I also made the netmonitor use this widget, which it wasn't before. That's why I made the telemetry thing optional.

I think that's basically it.
This lacks tests, but so far I only revived the patch basically, I'll get to those later if feedback on the approach is positive.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: sidebar.gif

Here's a gif demonstrating the new feature.

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> Created attachment 8525353 [details]
> sidebar.gif
> 
> Here's a gif demonstrating the new feature.
The gif is cut off a little bit on the right side. The [...] button is wider than what it appears to be in the gif.
I also realized there's a bug in the netmonitor: the menu contains a "preview" item, which isn't visible in the tabbox normally. I'll investigate this.

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> "preview" item, which isn't visible in the tabbox normally. I'll investigate
> this.

The preview tab is only visible for HTML requests.

Comment 5

[Patrick Brosset <:pbro>]

Yeah, that's what I realized after doing the gif. I'm in the process of fixing this.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug1101569-sidebar-allmenu-button v2.patch

Fixes the "preview" menu item problem in the netmonitor UI.
I'm not entirely happy yet with how "existing tabs" are added to the sidebar widget. I'm not sure all of the class methods work with these tabs. So I need to polish this part. And also add tests.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8525395 [details] [diff] [review]
bug1101569-sidebar-allmenu-button v2.patch

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

All of this looks pretty good to me. Lots of changes to sidebar.js, and I don't know the existing code very well, so you might want to request an additional review from someone who does.

::: browser/devtools/framework/sidebar.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  const {Cu} = require("chrome");
>  
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');

Nit: single quotes or double quotes should have consistent usage.

@@ +31,5 @@
> + *
> + * Tabs added through the addTab method are only identified by an ID and a URL
> + * which is used as the href of an iframe node that is inserted in the newly
> + * created tabpanel.
> + * Tabs already present before the ToolSidebar is created may contain anything.

Nit: I'd like to see all of these changes land in a separate bug, if splitting this patch won't take too much time. This bug is about adding this functionality to the netmonitor, not enhancing ToolSidebar. But I'll let you decide what's best.

::: browser/locales/en-US/chrome/browser/devtools/toolbox.properties
@@ +73,5 @@
>  
> +# LOCALIZATION NOTE (sidebar.showAllTabs.label)
> +# This is the label shown in the show all tabs button in the tabbed side
> +# bar, when there's no enough space to show all tabs at once
> +sidebar.showAllTabs.label=...

Use an ellipsis instead of dotdotdot.

Comment 8

[Patrick Brosset <:pbro>]

Thanks for the feedback Victor. I'm currently cleaning the patch up a bit:
- splitting it in 2, 1 for the sidebar changes, 1 to add the all-tabs menu to the inspector and netmonitor
- also fixing a few remaining issues and adding tests for the new sidebar features.

Brian: I'm working on a new sidebar test but somehow which basically adds a new tool panel to the toolbox, with a splitter and sidebar, and adding tabs in there. For some reason, I can't get the styling to work properly, although I'm pretty sure I've included everything needed.
Do you mind taking a quick look at this when you've got time? 
- open firefox
- open scratchpad, in browser environment mode
- copy/paste the code from here http://pastebin.mozilla.org/7925468
- run the code
=> the toolbox appears, with a new panel, but the devtools theme is missing completely, and I can't figure out why.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> Brian: I'm working on a new sidebar test but somehow which basically adds a
> new tool panel to the toolbox, with a splitter and sidebar, and adding tabs
> in there. For some reason, I can't get the styling to work properly,
> although I'm pretty sure I've included everything needed.
> Do you mind taking a quick look at this when you've got time? 
> - open firefox
> - open scratchpad, in browser environment mode
> - copy/paste the code from here http://pastebin.mozilla.org/7925468
> - run the code
> => the toolbox appears, with a new panel, but the devtools theme is missing
> completely, and I can't figure out why.

OK, so the theme-switching script doesn't seem to be running at all (even a log statement at the top of it never runs).  So the light-theme.css file is never being added.  My best guess would be that it's a security thing with running chrome scripts from a data URI.

I replaced the contents of debugger.xul with your markup and changed testToolURL = "chrome://browser/content/devtools/debugger.xul" and everything worked.

I'm not sure if we want to package up a chrome file just for this test - maybe we could add it as a helper file and somehow enable xul content through a testing URL for this test?  Or maybe there is a pref we could override for the test that allows this behavior?

Comment 10

[Patrick Brosset <:pbro>]

Ah, thanks Brian for this. I had the impression it had something to do with the theme-switching script not working ok. This confirmed it. Thanks.
I'll probably end up creating a XUL file for the test and remove the data URI.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug1101569-sidebar-allmenu-button v3.patch

This new patch fixes Victor's review comments and adds a test. Also, I've removed the inspector-panel and netmonitor changes from this patch (they will be part of another patch I'll upload in a minute).

Dave: If I read sidebar.js' history correctly, you created that file. Victor started to review my changes but doesn't know this widget all that much. A review from you would be great.
My changes are basically about 2 things:
- adding an "all-tabs" menu when the tabs overflow,
- making the widget work with existing tabs and tabpanels (so that it can also be used with the netmonitor for instance).

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v1.patch

Using the new all-tabs menu in the inspector-panel and netmonitor's sidebars.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: bug1101569-sidebar-allmenu-button v4.patch

Thanks Dave for the review.
Rebased the patch.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v2.patch

Also rebased part 2.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug1101569-sidebar-allmenu-button v5.patch

Minor update after some netmonitor test failures.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v3.patch

Hey Victor, this part 2 has the required changes in the inspector and netmonitor to use the new sidebar widget.

2 specific details about the netmonitor:
- The panel didn't have telemetry probes for its sidebar, so I've added a new option to the sidebar widget for this purpose instead of adding probes. We can always add them later if we want to.
- The widget now has a toggleTab function to hide/show the HTML preview panel. I just had to slightly adapt one of the netmonitor tests for this to work.

Comment 17

[Patrick Brosset <:pbro>]

Green try build for both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0eafa5c0a83

Comment 18

[Patrick Brosset <:pbro>]

Attachment: bug1101569-sidebar-allmenu-button v6.patch

Pushed part 1 to fx-team: https://hg.mozilla.org/integration/fx-team/rev/e40dfb7e4ec7

(this new version of the patch has an updated commit message + fixes a minor bug (a typo really) I didn't see until try).
The corresponding try push URL is in the previous comment.

Comment 19

[Victor Porof [:vporof][:vp]]

Comment on attachment 8548745 [details] [diff] [review]
bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v3.patch

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

::: browser/devtools/netmonitor/test/browser_net_html-preview.js
@@ -22,5 @@
>          "The first tab in the details pane should be selected.");
>        is($("#preview-tab").hidden, true,
>          "The preview tab should be hidden for non html responses.");
> -      is($("#preview-tabpanel").hidden, true,
> -        "The preview tabpanel should be hidden for non html responses.");

Is the tabpanel not hidden anymore, or is this harder to test? I'd prefer to still have this check here if possible. 

IIRC, having a number of hidden tabs that's not equal to the number of hidden tabpanels makes the tabbox act strangely, but that may not be the case anymore.

Comment 20

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

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

Comment 21

[Patrick Brosset <:pbro>]

(In reply to Victor Porof [:vporof][:vp] from comment #19)
> Comment on attachment 8548745 [details] [diff] [review]
> bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v3.patch
> 
> Review of attachment 8548745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/netmonitor/test/browser_net_html-preview.js
> @@ -22,5 @@
> >          "The first tab in the details pane should be selected.");
> >        is($("#preview-tab").hidden, true,
> >          "The preview tab should be hidden for non html responses.");
> > -      is($("#preview-tabpanel").hidden, true,
> > -        "The preview tabpanel should be hidden for non html responses.");
> 
> Is the tabpanel not hidden anymore, or is this harder to test? I'd prefer to
> still have this check here if possible. 
tabpanels aren't hidden anymore, that's because the sidebar widget cannot guess the id of the tabpanel when toggleTab(id) is passed, since id corresponds to the tab object.
I'll rework this part to hide the tabpanel too.
> IIRC, having a number of hidden tabs that's not equal to the number of
> hidden tabpanels makes the tabbox act strangely, but that may not be the
> case anymore.
Ok, I didn't know, it feels better to hide both anyway, so I'll do that.

Comment 22

[Patrick Brosset <:pbro>]

Attachment: bug1101569-using-new-sidebar-overflow-in-inspector-netmonitor v4.patch

Fixed the last review comment. The toggleTab function now accepts an optional parameter which is used as the ID to locate the tabpanel to hide if provided. By default, the id of the tab will be used to try and get to the tabpanel.
(note that I re-ordered the arguments of this function to make more sense but this function is new in this release, so not used outside of the devtools code, and even there, only in 1 place).

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/0d7bbd6633c3

Comment 23

[Patrick Brosset <:pbro>]

Oh and the last try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0eafa5c0a83) was green for both patches.

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/0d7bbd6633c3