Move HTML panel to a docked "panel" in the main browser window (highlighter)

VERIFIED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: rc)

Tracking

unspecified
Firefox 9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][best: 2h; likely: 1d; worst: 3d][testday-20111125])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
The HTML panel should be docked into the main browser window by default. It should be animated on opening and closing similar to the way the Web Console animates.
(Assignee)

Updated

6 years ago
Blocks: 671294

Updated

6 years ago
Whiteboard: [minotaur]
(Assignee)

Comment 1

6 years ago
Could take a few days if the styling turns out to be tricky. Not anticipating issues though.
Priority: -- → P1
Whiteboard: [minotaur] → [minotaur][best: 2h; likely: 1d; worst: 3d]
(Assignee)

Updated

6 years ago
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
Created attachment 559249 [details] [diff] [review]
docked tree panel WIP

have a look.

There's a dead method in TreePanel.jsm (createSplitter) that I'm not using and can remove. The createResizer sets a style attribute on the resizer that should be moved into the theme files probably with a bumpy background so users can see there's something there before hovering over it.

There are no tests and the code path to access the previous panel-based layout is currently unavailable.

other than that... it's based on the patch to move the tree panel and it's dependencies in bug 681651.
Attachment #559249 - Flags: review?(mihai.sucan)
(Assignee)

Updated

6 years ago
Depends on: 681651
Comment on attachment 559249 [details] [diff] [review]
docked tree panel WIP

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

Patch looks great! r+ for a WIP, but please address the comments below.

One general comment: panel size is not remembered. This is "weird" when the user opens/reopens the panel, or when he switches tabs. We should have persistence, at least per browser session. (follow up bug report?)

::: browser/devtools/highlighter/TreePanel.jsm
@@ +66,4 @@
>  
>    /**
>     * container.
> +   * @returns xul:panel|xul:vbox|Object element

/**
 * The tree panel container element.
 * @returns xul:panel|xul:vbox|null
 *          xul:panel is returned when the tree panel is not docked, or xul:vbox when when the tree panel is docked. null is returned when no container is available.
 */

@@ +71,5 @@
>    get container()
>    {
> +    if (this.openInDock) {
> +      let vbox = this.document.getElementById("inspector-tree-box");
> +      return vbox ? vbox : { state: "closed" };

This should just return vbox, even if it's null. The object mock is not something we'd like in the longer run. Callers need to deal with the "no container" case.

@@ +144,5 @@
>  
> +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()
> +  {
> +    this.treeIFrame.removeEventListener("load", this.loadedInitializeTreePanel,
> +      true);

This event listener removal doesn't work. You do:

this.treeIFrame.addEventListener("load",
  this.loadedInitializeTreePanel.bind(this), true);

The bound function is a different function, so the removeEventListener call is ineffective.

@@ +170,4 @@
>      }
>  
> +    if (this.openInDock) { // Create vbox
> +      return this.openDocked();

The open() method returns void. openDocked() also returns void.

I would suggest: this.openDocked(); return; (to avoid confusion)

@@ +181,1 @@
>          treePanelShown, false);

This removeEventListener() call is also ineffective. The bound function is different.

@@ +208,5 @@
>    },
>  
> +  openDocked: function TP_openDocked()
> +  {
> +    let treeBox = null;

This line is not needed. Below you can just do: let treeBox = this.document...

@@ +210,5 @@
> +  openDocked: function TP_openDocked()
> +  {
> +    let treeBox = null;
> +    let toolbar = this.IUI.toolbar.nextSibling; // Addons bar, typically
> +    let toolbarParent = toolbar.parentNode;

This code is probably prone to errors. Can't we get to that element in a more specific way? Does it have an ID? or something

@@ +213,5 @@
> +    let toolbar = this.IUI.toolbar.nextSibling; // Addons bar, typically
> +    let toolbarParent = toolbar.parentNode;
> +    treeBox = this.document.createElement("vbox");
> +    treeBox.id = "inspector-tree-box";
> +    treeBox.state = "open"; // for the registerTools API.

Instead of this change I'd prefer updating our isOpen() implementation to correctly handle the docked mode.

@@ +215,5 @@
> +    treeBox = this.document.createElement("vbox");
> +    treeBox.id = "inspector-tree-box";
> +    treeBox.state = "open"; // for the registerTools API.
> +    treeBox.minHeight = 10;
> +    treeBox.setAttribute("flex", "1");

Doesn't treeBox.flex = 1 work here?

@@ +218,5 @@
> +    treeBox.minHeight = 10;
> +    treeBox.setAttribute("flex", "1");
> +    toolbarParent.insertBefore(treeBox, toolbar);
> +    this.createResizer();
> +    this.treeIFrame = treeBox.appendChild(this.treeIFrame);

appendChild() returns the appended child element - no need to do the prop update. Just do:

treeBox.appendChild(this.treeIFrame);

@@ +225,5 @@
> +
> +    let src = this.treeIFrame.getAttribute("src");
> +    if (src != "chrome://browser/content/inspector.html") {
> +      this.treeIFrame.setAttribute("src",
> +        "chrome://browser/content/inspector.html");

We should put this URI into a constant at the top of the JSM.

@@ +229,5 @@
> +        "chrome://browser/content/inspector.html");
> +    } else {
> +      this.treeIFrame.contentWindow.location.reload();
> +    }
> +    return;

This is not needed here.

@@ +242,5 @@
> +    resizer.id = "inspector-horizontal-splitter";
> +    resizer.setAttribute("dir", "top");
> +    resizer.setAttribute("flex", "1");
> +    resizer.setAttribute("element", "inspector-tree-box");
> +    resizer.setAttribute("style", "background: none !important; -moz-appearance: none; cursor: n-resize;");

This should probably be in the inspector stylesheet?

@@ +251,5 @@
> +
> +  /**
> +   * Create a splitter before the inspector toolbar. NOT USED. (but I'd like to)
> +   */
> +  createSplitter: function TP_createSplitter()

In the final patch I suggest that this is either dropped or used. It doesn't make sense to add unused code.
Attachment #559249 - Flags: review?(mihai.sucan) → review+
(Assignee)

Updated

6 years ago
Depends on: 650794
(Assignee)

Comment 4

6 years ago
Created attachment 561450 [details] [diff] [review]
docked tree panel v1

Addressed Mihai's comments. Have a quick look-over to see that I caught everything.

Will add a followup to persist tree panel height.
Attachment #561450 - Flags: review?(mihai.sucan)
(Assignee)

Comment 5

6 years ago
Created attachment 561462 [details] [diff] [review]
docked tree panel v1.1 - requires patch from bug 687854
Attachment #561462 - Flags: review?(gavin.sharp)
Comment on attachment 561450 [details] [diff] [review]
docked tree panel v1

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

Patch looks good! I only have two minor comments. See below.

::: browser/devtools/highlighter/TreePanel.jsm
@@ +145,5 @@
>      if (this.IUI.selection)
>        this.select(this.IUI.selection, true);
>    },
>  
> +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()

I really don't like this method.

@@ +192,5 @@
> +      this.container.removeEventListener("popupshown",
> +        boundTreePanelShown, false);
> +
> +        this.treeIFrame.addEventListener("load",
> +          boundLoadedInitializeTreePanel, true);

Wrong indentation level for these two lines.

@@ +236,5 @@
> +    treeBox.appendChild(this.treeIFrame);
> +
> +    let boundLoadedInitializeTreePanel =
> +      this.boundLoadedInitializeTreePanel =
> +        this.loadedInitializeTreePanel.bind(this);

I would prefer that instead of the boundLoadedInitializeTreePanel() function, you'd do something like the treePanelShown() above.

(same in both locations where the treeIframe is loaded)
Attachment #561450 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 7

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 561450 [details] [diff] [review]
> docked tree panel v1
> 
> Review of attachment 561450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good! I only have two minor comments. See below.
> 
> ::: browser/devtools/highlighter/TreePanel.jsm
> @@ +145,5 @@
> >      if (this.IUI.selection)
> >        this.select(this.IUI.selection, true);
> >    },
> >  
> > +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()
> 
> I really don't like this method.

Well, it's better than duplicating it in both locations, IMO. Have any recommendations?

> @@ +192,5 @@
> > +      this.container.removeEventListener("popupshown",
> > +        boundTreePanelShown, false);
> > +
> > +        this.treeIFrame.addEventListener("load",
> > +          boundLoadedInitializeTreePanel, true);
> 
> Wrong indentation level for these two lines.

oops!

> @@ +236,5 @@
> > +    treeBox.appendChild(this.treeIFrame);
> > +
> > +    let boundLoadedInitializeTreePanel =
> > +      this.boundLoadedInitializeTreePanel =
> > +        this.loadedInitializeTreePanel.bind(this);
> 
> I would prefer that instead of the boundLoadedInitializeTreePanel()
> function, you'd do something like the treePanelShown() above.
> 
> (same in both locations where the treeIframe is loaded)

You mean inline the function definition? Again, I'm not keen on duplicating it if at all possible, though it is only a few lines...
(Assignee)

Comment 8

6 years ago
ok, I tried it out and inlined the two functions. Looks somewhat better. :)
(Assignee)

Comment 9

6 years ago
Created attachment 561709 [details] [diff] [review]
[in-fx-team] docked tree panel v1.2 - requires patch from bug 687854

updated based on mihai's comments.
Attachment #561462 - Attachment is obsolete: true
Attachment #561709 - Flags: review?(gavin.sharp)
Attachment #561462 - Flags: review?(gavin.sharp)
(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > Comment on attachment 561450 [details] [diff] [review]
> > docked tree panel v1
> > 
> > Review of attachment 561450 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Patch looks good! I only have two minor comments. See below.
> > 
> > ::: browser/devtools/highlighter/TreePanel.jsm
> > @@ +145,5 @@
> > >      if (this.IUI.selection)
> > >        this.select(this.IUI.selection, true);
> > >    },
> > >  
> > > +  loadedInitializeTreePanel: function TP_loadedInitializeTreePanel()
> > 
> > I really don't like this method.
> 
> Well, it's better than duplicating it in both locations, IMO. Have any
> recommendations?
> 
> > @@ +236,5 @@
> > > +    treeBox.appendChild(this.treeIFrame);
> > > +
> > > +    let boundLoadedInitializeTreePanel =
> > > +      this.boundLoadedInitializeTreePanel =
> > > +        this.loadedInitializeTreePanel.bind(this);
> > 
> > I would prefer that instead of the boundLoadedInitializeTreePanel()
> > function, you'd do something like the treePanelShown() above.
> > 
> > (same in both locations where the treeIframe is loaded)
> 
> You mean inline the function definition? Again, I'm not keen on duplicating
> it if at all possible, though it is only a few lines...

Exactly, it's only a line. We have the event handler, initializeIFrame(), but we wrap it in a function that removes the event listener. My point was that for a single line there's no point to add a method to the TreePanel object.

Thanks for the patch update!
Attachment #561709 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #561450 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #559249 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Comment on attachment 561709 [details] [diff] [review]
[in-fx-team] docked tree panel v1.2 - requires patch from bug 687854

https://hg.mozilla.org/integration/fx-team/rev/b1c0b12a5f65
Attachment #561709 - Attachment description: docked tree panel v1.2 - requires patch from bug 687854 → [in-fx-team] docked tree panel v1.2 - requires patch from bug 687854
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/mozilla-central/rev/b1c0b12a5f65
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Backed out because of various new mochitest-browser-chrome leaks. Unfortunately, this landed with a bunch of other interweaved patches. I only felt comfortable ruling out f297a626dd13 and 7d5311c92e04.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

6 years ago
relanded:

https://hg.mozilla.org/integration/fx-team/rev/e9e8df65e841
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 9 → ---
https://hg.mozilla.org/mozilla-central/rev/e9e8df65e841
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9

Comment 16

6 years ago
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111123 Firefox/10.0a2
Verified fixed. Screencast: http://screencast.com/t/s50of7J58
Status: RESOLVED → VERIFIED
Whiteboard: [minotaur][best: 2h; likely: 1d; worst: 3d] → [minotaur][best: 2h; likely: 1d; worst: 3d][testday-20111125]
You need to log in before you can comment on or make changes to this bug.