Move the HTML tree view in the Page Inspector above the inspector toolbar

RESOLVED FIXED in Firefox 13

Status

defect
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: rcampbell, Assigned: paul)

Tracking

unspecified
Firefox 13
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

Reporter

Description

8 years ago
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.
Reporter

Updated

8 years ago
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
Assignee

Comment 1

7 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #590403 - Flags: review?(rcampbell)
Assignee

Updated

7 years ago
Assignee: nobody → paul
Assignee

Updated

7 years ago
Depends on: 718259
Assignee

Updated

7 years ago
Blocks: 719607
Reporter

Comment 2

7 years ago
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.
Reporter

Comment 3

7 years ago
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.
Assignee

Comment 4

7 years ago
Posted patch Patch v1.1 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #590403 - Attachment is obsolete: true
Attachment #590403 - Flags: review?(rcampbell)
Assignee

Comment 5

7 years ago
Posted patch Patch v1.2 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #592418 - Attachment is obsolete: true
Assignee

Comment 6

7 years ago
Posted patch Patch v1.3 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #592423 - Attachment is obsolete: true
Assignee

Comment 7

7 years ago
Posted patch Patch v1.4 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #592428 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592481 - Flags: review?(rcampbell)
Reporter

Comment 8

7 years ago
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.
Attachment #592481 - Flags: review?(rcampbell) → review+
Assignee

Comment 9

7 years ago
(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.
Assignee

Comment 10

7 years ago
Posted patch Patch v1.5 (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Attachment #592481 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Whiteboard: [land-in-fx-team]
Assignee

Updated

7 years ago
Whiteboard: [land-in-fx-team]
Assignee

Updated

7 years ago
Attachment #592771 - Flags: review?(dao)
Reporter

Comment 11

7 years ago
(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.
Reporter

Updated

7 years ago
Status: NEW → ASSIGNED
Reporter

Comment 12

7 years ago
is this ready to go? I'd like to land it.
Target Milestone: --- → Firefox 12
Assignee

Comment 13

7 years ago
Posted patch Patch v1.6 (obsolete) — Splinter Review
Assignee

Comment 14

7 years ago
(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.
Target Milestone: Firefox 12 → ---
Assignee

Updated

7 years ago
Blocks: 724507
Reporter

Comment 15

7 years ago
ping for review! This is holding up other patches.
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?
Attachment #592771 - Flags: review?(dao)
Reporter

Comment 17

7 years ago
Posted patch Patch v1.6 - rebased (obsolete) — Splinter Review
Attachment #592771 - Attachment is obsolete: true
Attachment #594155 - Attachment is obsolete: true
Assignee

Comment 18

7 years ago
Posted patch patch v1.7 (obsolete) — Splinter Review
indentation fixed, removed removeAttribute()
Assignee

Comment 19

7 years ago
(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
Posted image 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?
(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?
Assignee

Comment 22

7 years ago
(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
Assignee

Comment 23

7 years ago
I am not strongly against having a 1px splitter though.
Assignee

Comment 24

7 years ago
Stephen, can you please tell us what we should do here?
Assignee

Comment 25

7 years ago
Stephen said 1px.
Assignee

Comment 26

7 years ago
Posted patch patch v1.8 (obsolete) — Splinter Review
1px splitter
Assignee

Updated

7 years ago
Attachment #599243 - Attachment is obsolete: true
Assignee

Comment 27

7 years ago
Posted patch patch v1.9Splinter Review
Assignee

Updated

7 years ago
Attachment #599269 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #600058 - Flags: review?(dao)
Attachment #600058 - Flags: review?(dao) → review+
Attachment #599073 - Attachment is obsolete: true
Assignee

Comment 28

7 years ago
Thank you Dão.
Whiteboard: [land-in-fx-team]
Assignee

Comment 29

7 years ago
https://hg.mozilla.org/integration/fx-team/rev/3996ac18197d
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3996ac18197d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13

Updated

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