Last Comment Bug 717924 - Move the HTML tree view in the Page Inspector above the inspector toolbar
: Move the HTML tree view in the Page Inspector above the inspector toolbar
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on: 718259
Blocks: 719607 724507
  Show dependency treegraph
 
Reported: 2012-01-13 07:09 PST by Rob Campbell [:rc] (:robcee)
Modified: 2012-02-27 01:18 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (10.13 KB, patch)
2012-01-20 18:11 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.1 (12.38 KB, patch)
2012-01-28 10:30 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.2 (12.40 KB, patch)
2012-01-28 10:42 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.3 (13.96 KB, patch)
2012-01-28 11:25 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.4 (13.98 KB, patch)
2012-01-29 03:33 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
Patch v1.5 (13.99 KB, patch)
2012-01-30 10:33 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.6 (14.13 KB, patch)
2012-02-03 06:15 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Patch v1.6 - rebased (10.96 KB, patch)
2012-02-21 02:50 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
patch v1.7 (14.24 KB, patch)
2012-02-21 10:14 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
bottom border (4.04 KB, image/png)
2012-02-21 10:23 PST, Dão Gottwald [:dao]
no flags Details
patch v1.8 (13.94 KB, patch)
2012-02-21 11:04 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.9 (14.02 KB, patch)
2012-02-23 09:41 PST, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2012-01-13 07:09:57 PST
Currently, the HTML tree view is below the page inspector toolbar. We want to move this above the (inspector) toolbar to better isolate this view from the page-centric tools.

We may want to file a separate bug to split it out of the registerTools mechanism used by the Inspector as these tools should be specifically for (nodecentric) tools that show up in the node sidebar.
Comment 1 Paul Rouget [:paul] 2012-01-20 18:11:17 PST
Created attachment 590403 [details] [diff] [review]
patch v1
Comment 2 Rob Campbell [:rc] (:robcee) 2012-01-28 07:27:41 PST
Comment on attachment 590403 [details] [diff] [review]
patch v1

   openDocked: function TP_openDocked()
   {
     let treeBox = null;
-    let toolbar = this.IUI.toolbar.nextSibling; // Addons bar, typically
-    let toolbarParent =
-      this.IUI.browser.ownerDocument.getElementById("browser-bottombox");
     treeBox = this.document.createElement("vbox");
     treeBox.id = "inspector-tree-box";
     treeBox.state = "open"; // for the registerTools API.

we should probably lose this comment since we're not going to be going through registerTools anymore.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-01-28 08:00:35 PST
neat.

I don't think that's the styling we want on the splitter though.

Either what we have on the Web Console or a hidden splitter like we have on the Style sidebar.
Comment 4 Paul Rouget [:paul] 2012-01-28 10:30:35 PST
Created attachment 592418 [details] [diff] [review]
Patch v1.1
Comment 5 Paul Rouget [:paul] 2012-01-28 10:42:03 PST
Created attachment 592423 [details] [diff] [review]
Patch v1.2
Comment 6 Paul Rouget [:paul] 2012-01-28 11:25:49 PST
Created attachment 592428 [details] [diff] [review]
Patch v1.3
Comment 7 Paul Rouget [:paul] 2012-01-29 03:33:05 PST
Created attachment 592481 [details] [diff] [review]
Patch v1.4
Comment 8 Rob Campbell [:rc] (:robcee) 2012-01-30 10:02:35 PST
Comment on attachment 592481 [details] [diff] [review]
Patch v1.4

+      <toolbarbutton id="inspector-inspect-toolbutton"
+                      class="devtools-toolbarbutton"
+                      label="&inspectButton.label;"
+                      accesskey="&inspectButton.accesskey;"
+                      command="Inspector:Inspect"/>
+      <arrowscrollbox id="inspector-breadcrumbs"
+                      flex="1" orient="horizontal"
+                      clicktoscroll="true"/>

nit: indenting's a bit weird here. Should be lined up under the "i" in id.

in gnomestripe:
+/* Highlighter toolbar - HTML Tree */
+
+#inspector-tree-splitter {
+  -moz-appearance: none;
+  height: 4px;
+  border-top: 1px solid hsla(210,8%,5%,.45);
+  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
+}

no border-bottom?

from gnomestripe/inspector.css:

 
 body {
   margin: 0;
   overflow: auto;
   font-family: Lucida Grande, sans-serif;
   font-size: 11px;
-  border-top: 1px solid #BBB9BA;
+  padding-top: 5px;

why this change? Does the padding here accomodate all of the splitter?

Seems ok, r+ with the indentation fixed.
Comment 9 Paul Rouget [:paul] 2012-01-30 10:18:10 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> in gnomestripe:
> +/* Highlighter toolbar - HTML Tree */
> +
> +#inspector-tree-splitter {
> +  -moz-appearance: none;
> +  height: 4px;
> +  border-top: 1px solid hsla(210,8%,5%,.45);
> +  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
> +}
> 
> no border-bottom?

No. Do you think we need one?

> from gnomestripe/inspector.css:
> 
>  
>  body {
>    margin: 0;
>    overflow: auto;
>    font-family: Lucida Grande, sans-serif;
>    font-size: 11px;
> -  border-top: 1px solid #BBB9BA;
> +  padding-top: 5px;
> 
> why this change? Does the padding here accomodate all of the splitter?

Without that, immediately after the splitter we see the doctype, stuck to the splitter. It's weird.
Comment 10 Paul Rouget [:paul] 2012-01-30 10:33:10 PST
Created attachment 592771 [details] [diff] [review]
Patch v1.5
Comment 11 Rob Campbell [:rc] (:robcee) 2012-01-30 15:32:56 PST
(In reply to Paul Rouget [:paul] from comment #9)
> (In reply to Rob Campbell [:rc] (robcee) from comment #8)
> > in gnomestripe:
> > +/* Highlighter toolbar - HTML Tree */
> > +
> > +#inspector-tree-splitter {
> > +  -moz-appearance: none;
> > +  height: 4px;
> > +  border-top: 1px solid hsla(210,8%,5%,.45);
> > +  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
> > +}
> > 
> > no border-bottom?
> 
> No. Do you think we need one?

Possibly. I'd defer to shorlander and co. for that one.

> > -  border-top: 1px solid #BBB9BA;
> > +  padding-top: 5px;
> > 
> > why this change? Does the padding here accomodate all of the splitter?
> 
> Without that, immediately after the splitter we see the doctype, stuck to
> the splitter. It's weird.

Ah, makes sense.
Comment 12 Rob Campbell [:rc] (:robcee) 2012-02-03 06:09:29 PST
is this ready to go? I'd like to land it.
Comment 13 Paul Rouget [:paul] 2012-02-03 06:15:53 PST
Created attachment 594155 [details] [diff] [review]
Patch v1.6
Comment 14 Paul Rouget [:paul] 2012-02-03 06:20:56 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #12)
> is this ready to go? I'd like to land it.

We need a review from Dão first.
Comment 15 Rob Campbell [:rc] (:robcee) 2012-02-06 07:54:59 PST
ping for review! This is holding up other patches.
Comment 16 Dão Gottwald [:dao] 2012-02-10 08:41:07 PST
Comment on attachment 592771 [details] [diff] [review]
Patch v1.5

>+      <toolbarbutton id="highlighter-closebutton"
>+                      oncommand="InspectorUI.closeInspectorUI(false);"
>+                      tooltiptext="&inspectCloseButton.tooltiptext;"/>

>+        <toolbarbutton id="inspector-3D-button"
>+                        class="devtools-toolbarbutton"
>+                        hidden="true"
>+                        label="&inspect3DViewButton.label;"
>+                        accesskey="&inspect3DViewButton.accesskey;"
>+                        command="Inspector:Tilt"/>
>+        <toolbarbutton id="inspector-style-button"
>+                        class="devtools-toolbarbutton"
>+                        label="&inspectStyleButton.label;"
>+                        accesskey="&inspectStyleButton.accesskey;"
>+                        command="Inspector:Sidebar"/>

>+      <toolbarbutton id="highlighter-closebutton"
>+                      oncommand="InspectorUI.closeInspectorUI(false);"
>+                      tooltiptext="&inspectCloseButton.tooltiptext;"/>

indentation is off

>       this.IUI.toolbar.removeAttribute("treepanel-open");

still need to remove this

>+#inspector-tree-splitter {
>+  -moz-appearance: none;
>+  height: 4px;
>+  border-top: 1px solid hsla(210,8%,5%,.45);
>+  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
>+}

This should have a height of 1px similarly to devtools-side-splitter being 1px wide.

I also noticed that the toolbar has a bogus bottom border, although not caused by this patch.

Why is there a newer patch attached while the review request flag remains on this one?
Comment 17 Rob Campbell [:rc] (:robcee) 2012-02-21 02:50:17 PST
Created attachment 599073 [details] [diff] [review]
Patch v1.6 - rebased
Comment 18 Paul Rouget [:paul] 2012-02-21 10:14:53 PST
Created attachment 599243 [details] [diff] [review]
patch v1.7

indentation fixed, removed removeAttribute()
Comment 19 Paul Rouget [:paul] 2012-02-21 10:18:54 PST
(In reply to Dão Gottwald [:dao] from comment #16)
> >+#inspector-tree-splitter {
> >+  -moz-appearance: none;
> >+  height: 4px;
> >+  border-top: 1px solid hsla(210,8%,5%,.45);
> >+  background: -moz-linear-gradient(top, hsl(210,11%,36%), hsl(210,11%,18%));
> >+}
> 
> This should have a height of 1px similarly to devtools-side-splitter being 1px wide.

It's what I did at first, but that looks too weird.
We need a clear separation between the content and the panel here.

> I also noticed that the toolbar has a bogus bottom border, although not caused by this patch.

Can you be more specific?

> Why is there a newer patch attached while the review request flag remains on this one?

Sorry about that. The only difference is the addition of a border-bottom to the #inspector-tree-splitter
Comment 20 Dão Gottwald [:dao] 2012-02-21 10:23:05 PST
Created attachment 599247 [details]
bottom border

(In reply to Paul Rouget [:paul] from comment #19)
> > I also noticed that the toolbar has a bogus bottom border, although not caused by this patch.
> 
> Can you be more specific?
Comment 21 Dão Gottwald [:dao] 2012-02-21 10:24:40 PST
(In reply to Paul Rouget [:paul] from comment #19)
> > This should have a height of 1px similarly to devtools-side-splitter being 1px wide.
> 
> It's what I did at first, but that looks too weird.
> We need a clear separation between the content and the panel here.

Why exactly would this be an issue here but not for devtools-side-splitter?
Comment 22 Paul Rouget [:paul] 2012-02-21 10:43:35 PST
(In reply to Dão Gottwald [:dao] from comment #21)
> (In reply to Paul Rouget [:paul] from comment #19)
> > > This should have a height of 1px similarly to devtools-side-splitter being 1px wide.
> > 
> > It's what I did at first, but that looks too weird.
> > We need a clear separation between the content and the panel here.
> 
> Why exactly would this be an issue here but not for devtools-side-splitter?

The devtools-side-splitter doesn't cut any content. The inspector-tree-splitter does.
Screenshot:
  1px splitter http://i.imgur.com/IHlSe.png
  4px splitter http://i.imgur.com/n1PhO.png
Comment 23 Paul Rouget [:paul] 2012-02-21 10:44:11 PST
I am not strongly against having a 1px splitter though.
Comment 24 Paul Rouget [:paul] 2012-02-21 10:45:58 PST
Stephen, can you please tell us what we should do here?
Comment 25 Paul Rouget [:paul] 2012-02-21 10:46:15 PST
Stephen said 1px.
Comment 26 Paul Rouget [:paul] 2012-02-21 11:04:01 PST
Created attachment 599269 [details] [diff] [review]
patch v1.8

1px splitter
Comment 27 Paul Rouget [:paul] 2012-02-23 09:41:54 PST
Created attachment 600058 [details] [diff] [review]
patch v1.9
Comment 28 Paul Rouget [:paul] 2012-02-24 07:16:01 PST
Thank you Dão.
Comment 30 Tim Taubert [:ttaubert] 2012-02-27 01:18:19 PST
https://hg.mozilla.org/mozilla-central/rev/3996ac18197d

Note You need to log in before you can comment on or make changes to this bug.