Closed Bug 994055 Opened 6 years ago Closed 5 years ago

Collapse inspector sidebar

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

It would be nice if the devtools inspector had a toggle sidebar icon in the UI somewhere.
In fact, exactly like the debugger has an icon to toggle the watched expressions/variables panel would be perfect.

The fact that the sidebar is closed or opened should be stored so it survives browser re-opening, just like we store the sidebar width today.

The icon and position of the icon should be the same as in the debugger.
Blocks: firebug-gaps
After discussing with Victor, it seems like this is a good next bug. I was originally going to file this as a good first bug, but decided against because it might be a good thing to make sure here that the toggle works exactly the same as in the debugger (using ViewHelpers.jsm) to simplify maintenance, and there could be some relative complexity there.

I'm still to find a mentor for this. If anyone is interested in taking this bug, I'll start looking for one.
I can volunteer but have very little experience in this area.
Whiteboard: [good next bug][lang=js][lang=xul]
I'd love to take this, although, I need to finish the loads of bugs I'm currently assigned to :p
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Patrick, can you mentor me please ? I'd love to have this feature in the inspector :)
Assignee: ntim007 → nobody
Flags: needinfo?(pbrosset)
Yes! Thanks for wanting to fix this.

There's one difference with the debugger in terms of UI: the debugger has it's own toolbar that spans the whole length of the panel, and that is used to hold the collapse/expand sidebar button. The inspector doesn't have that.
We could change this, but I'd say, in this bug, let's put the icon next to the search field, and we'll see later if/how we want to rework the UI.

In terms of code, I haven't looked at this part in the past, so I'll discover at the same time as you.
Here's what I can see:

- There's code already in the tree for handling collapsible sidepanels: ViewHelpers.jsm has a togglePane function just for that. A bunch of tools use it already: the debugger, netmonitor and webaudio editor. You can see it being used in debugger-view.js
- debugger.xul contains the debugger markup and has a :
<tabbox id="instruments-pane" ...> 
element which is passed to ViewHelpers.togglePane
- inspector.xul is the equivalent file but for the inspector, it also has a splitter and a tabbox, so I believe using the togglePane should work just fine.

In fact, open the inspector now on this page, then open scratchpad (in browser environment), and enter and run the following code:

let box = [...gDevTools._toolboxes][0][1];
let inspector = box.getPanel("inspector");
let pane = inspector.panelDoc.getElementById("inspector-sidebar")

ViewHelpers.togglePane({
  visible: false,
  animated: true
}, pane);

- As for the button, let's copy what is done in the debugger too: add a <toolbarbutton> in the inspector markup, next to the search field, and reuse the same icon (see debugger.inc.css).
- There's one edge case to take in consideration: when you resize the toolbox to make it smaller, at some stage the layout of the inspector changes and the splitter turns horizontal, so the sidebar is actually below the markup-view. It looks like ViewHelpers.togglePane doesn't handle this case well, so we should probably disable or hide the collapse/expand button when in horizontal mode (and file a bug to make togglePane work in this mode too).
Assignee: nobody → ntim007
Flags: needinfo?(pbrosset)
Whiteboard: [good next bug][lang=js][lang=xul] → [good next bug][lang=js][lang=xul][mentor=pbrosset]
Mentor: pbrosset
Whiteboard: [good next bug][lang=js][lang=xul][mentor=pbrosset] → [good next bug][lang=js][lang=xul]
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
is anyone working on this bug? can i take up this bug ?
(In reply to Praveenkumar[:speaker] from comment #5)
> is anyone working on this bug? can i take up this bug ?

Nobody is working on it :) Just assigned this bug to you.
Assignee: nobody → spkr322
Status: NEW → ASSIGNED
in which irc group i can find patrick or ntim
(In reply to Praveenkumar[:speaker] from comment #7)
> in which irc group i can find patrick or ntim
Please join #devtools on IRC (nick pbrosset). I'm in France, so you'll usually find me there between 8am and 8pm UTC+1.
Attached patch bug-994055-fix.patch (obsolete) — Splinter Review
Hi Patrick,

I have attached patch here. please review it. Also is it ok if I take this and work on it?

Thanks
Attachment #8483433 - Flags: feedback?(pbrosset)
The patch seems ok at first glance, I will have a closer look in a little while.
Praveen, this bug hasn't moved for a while, are you ok to transfer ownership to Jignesh?
Flags: needinfo?(spkr322)
Alright, assigning the bug to you Jignesh. I will be reviewing your patch later today.
Assignee: spkr322 → jigneshhk1992
Flags: needinfo?(spkr322)
Comment on attachment 8483433 [details] [diff] [review]
bug-994055-fix.patch

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

I've applied the patch and tested it, it's working fine. The only comment I have about it is about the expand icon size/position: it appears stretched out for me, at least on my laptop (could be the retina screen) and there is too much margin on its right side. I'll attach a screenshot later.

::: browser/devtools/inspector/inspector-panel.js
@@ +7,5 @@
>  const {Cc, Ci, Cu, Cr} = require("chrome");
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

Or, simpler: 
let {HostType} = require("devtools/framework/toolbox").Toolbox;

Which means that you can then use
HostType.SIDE
instead of 
devtools.Toolbox.HostType.SIDE

@@ +61,5 @@
>    this.panelWin = iframeWindow;
>    this.panelWin.inspector = this;
>    this._inspector = null;
>  
> +  this._onToggleSidepanePressed = this._onToggleSidepanePressed.bind(this);

Please camelcase this method name: _onToggleSidePanePressed.
Also, I think this line would be better placed just before "this._sidePaneToggleButton.addEventListener("mousedown", this._onToggleSidepanePressed, false);" inside the deferredOpen method.

@@ +127,5 @@
>      this.onBeforeNewSelection = this.onBeforeNewSelection.bind(this);
>      this.selection.on("before-new-node-front", this.onBeforeNewSelection);
>      this.onDetached = this.onDetached.bind(this);
>      this.selection.on("detached-front", this.onDetached);
> +    this.breadcrumbs = new HTMLBreadcrumbs(this);

Please add an empty line before this line, there used to be one to separate the creation of the breadcrumbs widget from the previous block, and I think it makes sense to keep it that way.

@@ +133,5 @@
> +    this._sidePaneToggleButton = this.panelDoc.getElementById("inspector-sidepane-toggle");
> +    this._sidePaneToggleButton.addEventListener("mousedown", this._onToggleSidepanePressed, false);
> +
> +    this._collapsePaneString = this.strings.GetStringFromName("collapsePanes");
> +    this._expandPaneString = this.strings.GetStringFromName("expandPanes");

I think accessing and storing these 2 strings here add to the overall complexity of the deferredOpen method, for little gain. 
The method is almost 100 lines long and has many blocks that have very little to do with each other. So, in order to try and keep it at least a little bit consistent, I would remove these declarations (see my comment in _onToggleSidepanePressed)

@@ +163,5 @@
>          }
>  
>        };
> +
> +      // Toggle sidebar button when host changes.

This function + the event listener on host-changed below + the call to the function further down are all inside 
if (this.target.isLocalTab) { ... }
which means this won't work unless you are debugging a local desktop firefox tab (won't work when debugging a b2g device).

Can you move them up to next to where you do "this._sidePaneToggleButton.addEventListener("mousedown", this._onToggleSidepanePressed, false);"?

@@ +229,5 @@
> +    }, sidePane);
> +
> +    if (visible) {
> +      button.removeAttribute("pane-collapsed");
> +      button.setAttribute("tooltiptext", this._collapsePaneString);

See my comment regarding strings. Just access the string here: this.strings.GetStringFromName("collapsePanes");

@@ +232,5 @@
> +      button.removeAttribute("pane-collapsed");
> +      button.setAttribute("tooltiptext", this._collapsePaneString);
> +    } else {
> +      button.setAttribute("pane-collapsed", "");
> +      button.setAttribute("tooltiptext", this._expandPaneString);

Same comment here.

@@ +565,5 @@
>      this.sidebar = null;
>  
>      this.nodemenu.removeEventListener("popupshowing", this._setupNodeMenu, true);
>      this.nodemenu.removeEventListener("popuphiding", this._resetNodeMenu, true);
> +    this._sidePaneToggleButton.addEventListener("mousedown", this._onToggleSidepanePressed, false);

This is the destroy function, you shouldn't be adding the event listener, removing it instead.

::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
@@ +24,5 @@
>  <!ENTITY inspectorCopyImageDataUri.label       "Copy Image Data-URL">
> +
> +<!-- LOCALIZATION NOTE (inspectorUI.panesButton.tooltip): This is the tooltip for
> +  -  the button that toggles the panes visible or hidden in the inspector UI. -->
> +<!ENTITY inspectorUI.panesButton.tooltip "Toggle panes">
\ No newline at end of file

Do we really need to define this new string? It isn't actually displayed in the UI at all. No sense in adding strings that aren't visible.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +55,5 @@
>  #LOCALIZATION NOTE: Used in the image preview tooltip when the image could not be loaded
>  eventsTooltip.openInDebugger=Open in Debugger
> +
> +# LOCALIZATION NOTE (collapsePanes): This is the tooltip for the button
> +# that collapses the left and right panes in the debugger UI.

I guess this comment was copy/pasted from the debugger properties file, please change "collapses the left and right panes in the debugger UI" by "collapses the right pane in the inspector UI".

@@ +56,5 @@
>  eventsTooltip.openInDebugger=Open in Debugger
> +
> +# LOCALIZATION NOTE (collapsePanes): This is the tooltip for the button
> +# that collapses the left and right panes in the debugger UI.
> +collapsePanes=Collapse panes

Can you prefix this string key with 'inspector.' ?

@@ +59,5 @@
> +# that collapses the left and right panes in the debugger UI.
> +collapsePanes=Collapse panes
> +
> +# LOCALIZATION NOTE (expandPanes): This is the tooltip for the button
> +# that expands the left and right panes in the debugger UI.

I guess this comment was copy/pasted from the debugger properties file, please change "expands the left and right panes in the debugger UI" by "expands the right pane in the inspector UI".

@@ +60,5 @@
> +collapsePanes=Collapse panes
> +
> +# LOCALIZATION NOTE (expandPanes): This is the tooltip for the button
> +# that expands the left and right panes in the debugger UI.
> +expandPanes=Expand panes

Can you prefix this string key with 'inspector.' ?

::: browser/themes/shared/devtools/inspector.css
@@ +42,5 @@
> +#inspector-sidepane-toggle {
> +  background: none;
> +  box-shadow: none;
> +  border: none;
> +  list-style-image: url(debugger-collapse.png);

Since we are now using this image (and the expand and the 2x versions) in more than just the debugger, I think we should rename it. Something like pane-collapse.png would fit better.
Please search for debugger-collapse.png in the whole /browser/ folder as there are several occurrences of it, and you'll need to change them all.
Attachment #8483433 - Flags: feedback?(pbrosset)
I have fixed all other changes except below one. Need help with that. 

> I've applied the patch and tested it, it's working fine. The only comment I
> have about it is about the expand icon size/position: it appears stretched
> out for me, at least on my laptop (could be the retina screen) and there is
> too much margin on its right side. I'll attach a screenshot later.

Expand icon looks same in debugger as well. Is there any way to fix the margin and stretched look of image?
(In reply to Jignesh Kakadiya [:jhk] from comment #13)
> I have fixed all other changes except below one. Need help with that. 
> 
> > I've applied the patch and tested it, it's working fine. The only comment I
> > have about it is about the expand icon size/position: it appears stretched
> > out for me, at least on my laptop (could be the retina screen) and there is
> > too much margin on its right side. I'll attach a screenshot later.
> 
> Expand icon looks same in debugger as well. Is there any way to fix the
> margin and stretched look of image?
Really? For me only the icon you added to the inspector looked stretched, the debugger icon was fine.
I realize I forgot to add the screenshot I promised last time. Here it is.
Attached patch bug-994055-fix.patch (obsolete) — Splinter Review
Here is the updated patch. I am still unable to reproduce the issue in my Macbook Pro. Is there any way to inspect developer tools CSS and fix this issue?
Attachment #8483433 - Attachment is obsolete: true
Attachment #8489210 - Flags: feedback?(pbrosset)
Comment on attachment 8489210 [details] [diff] [review]
bug-994055-fix.patch

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

Thanks for this new patch. I see all of my comments have been addressed.
I also can't see the stretched image anymore, so it means it must have been a temporary XUL-related problem I had on my fx-team clone.

2 remaining things before R+:
- minor: the patch needs rebasing
- more important: we need a new test for this new feature to avoid regressions in the future. Some guidelines:
  - add a new test in /browser/devtools/inspector/test
  - name it something like browser_inspector_sidebar-collapse.js
  - add it (in alphabetical order) in /browser/devtools/inspector/test/browser.ini
  - you can then run it with |./mach mochitest-devtools /browser/devtools/inspector/test/browser_inspector_sidebar-collpase.js|
  - for the test code itself, you can open the toolbox with the inspector panel on by default with |let { inspector, toolbox } = yield openInspectorForURL(TEST_URI);|
  - and you can then access the inspector document with |inspector.panelDoc|, which you can use to retrieve the expand/collapse icon from the DOM.
Attachment #8489210 - Flags: feedback?(pbrosset) → feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
Rebased and added a test. (Still kept the old author name, since he did most of the work).
Attachment #8489210 - Attachment is obsolete: true
Attachment #8522310 - Flags: review?(pbrosset)
Comment on attachment 8522310 [details] [diff] [review]
Patch v3

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

This looks great. Thanks ntim for picking up this patch. I'm building locally with the patch applied to give it a quick test and will then push it to try.
Attachment #8522310 - Flags: review?(pbrosset) → review+
Ok, the patch seems to work perfectly.
Here's a pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a3fb07a9012e
Thanks for pushing to try.

I've just noticed that the patch doesn't completely work on Windows, the button doesn't work unless you resize the sidebar first. I've investigated a bit, and I think it requires a width attribute to be set to work. Can you reproduce that issue ?
Flags: needinfo?(pbrosset)
Comment on attachment 8522310 [details] [diff] [review]
Patch v3

I feel ashamed, my own test is failing :p 
Looks like I have to use the mousedown event instead of click (in the test).
Attachment #8522310 - Flags: review+
Assignee: jigneshhk1992 → pbrosset
Mentor: pbrosset
Flags: needinfo?(pbrosset)
Whiteboard: [good next bug][lang=js][lang=xul]
Blocks: 1150889
/r/6599 - Bug 994055 - 1 - Adds a toggle sidebar panel button to the inspector; r=miker
/r/6601 - Bug 994055 - 2 - Tests for the inspector sidebar toggle button; r=miker

Pull down these commits:

hg pull -r 254fdeddcaab3c623382f2824ee44856c127e732 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8587977 - Flags: review?(mratcliffe)
Attachment #8587977 - Flags: review?(mratcliffe) → review+
Attachment #8522310 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d34c868e5f67
https://hg.mozilla.org/mozilla-central/rev/96271b3c370d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Depends on: 1152279
Attachment #8587977 - Attachment is obsolete: true
Attachment #8618116 - Flags: review+
Attachment #8618117 - Flags: review+
Depends on: 1200104
Blocks: 1200179
See Also: → 1339698
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.