768442 - It's impossible to add a sidebar inside a tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Paul Rouget [:paul]]

We want to be able to add tools into a sidebar that is part of the tab, not the whole firefox window.

For example, we have to destroy the Inspector when we switch to a new tab, because it lives outside of the tab. We want the sidebar to be automatically hidden when we switch to a new tab.

So the sidebar needs to live inside the notification box.

It's possible to add content to the notification box, but it will be vertically aligned (below the browser). We want to be able to add element on the sides.

Comment 1

[Paul Rouget [:paul]]

I suggest that we introduce a <hbox> around the browser and inside the tab .

Comment 2

[Paul Rouget [:paul]]

Attachment: draft

Comment 3

[Paul Rouget [:paul]]

This would make our life much easier. We could add tools inside this <hbox>.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 636691 [details] [diff] [review]
draft

This is becoming a mess that people not having witnessed the progress will have a hard time understanding...

Where possible, tabbrowser should be agnostic about external features. If the container is for side bars, call it accordingly. browserContainer, on the other hand, could use a clearer name.

The endless firstChild and parentNode chains are exceptionally ugly.

>+                  <xul:browser type="content-primary" message="true" disablehistory="true"
>+                              xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup"/>

indentation is off

>             // This will unload the document. An unload handler could remove
>             // dependant tabs, so it's important that the tabbrowser is now in
>             // a consistent state (tab removed, tab positions updated, etc.).
>-            panel.removeChild(this.getBrowserContainer(browser));
>+            panel.removeChild(this.getDevtoolsContainer(browser));

This should just remove the browser, not some random container.

Comment 5

[Paul Rouget [:paul]]

Attachment: v1

Comment 6

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #4)
> This is becoming a mess that people not having witnessed the progress will
> have a hard time understanding...

Let me know if you think of a better solution. Maybe I should add comments to explain the original purpose of the sidebarContainer and browserContainer.

I have addressed your comments.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 637025 [details] [diff] [review]
v1

>+            browserContainer.setAttribute("anonid", "browserContainer");
>+            browserContainer.appendChild(stack);
>+            browserContainer.setAttribute("flex", "1");
>+
>+            // Create the sidebar container
>+            var sidebarContainer = document.createElementNS(
>+                                      "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                      "hbox");
>+            sidebarContainer.setAttribute("anonid", "sidebarContainer");
>+            sidebarContainer.appendChild(browserContainer);
>+            sidebarContainer.setAttribute("flex", "1");

Why are you setting anonid attributes on these elements?

>             var panel = this.getNotificationBox(browser);
>+            var stack = this.getBrowserContainer(browser).firstChild;
> 
>             // This will unload the document. An unload handler could remove
>             // dependant tabs, so it's important that the tabbrowser is now in
>             // a consistent state (tab removed, tab positions updated, etc.).
>-            panel.removeChild(this.getBrowserContainer(browser));
>+            stack.removeChild(browser);

browser.parentNode.removeChild(browser);

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 637025 [details] [diff] [review]
v1

see previous comment

Comment 9

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 637025 [details] [diff] [review]
> v1
> 
> >+            browserContainer.setAttribute("anonid", "browserContainer");
> >+            browserContainer.appendChild(stack);
> >+            browserContainer.setAttribute("flex", "1");
> >+
> >+            // Create the sidebar container
> >+            var sidebarContainer = document.createElementNS(
> >+                                      "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> >+                                      "hbox");
> >+            sidebarContainer.setAttribute("anonid", "sidebarContainer");
> >+            sidebarContainer.appendChild(browserContainer);
> >+            sidebarContainer.setAttribute("flex", "1");
> 
> Why are you setting anonid attributes on these elements?

Because I add them in the XML too. I'm not sure to understand why there are 2 ways to build the tab markup, but I thought it's important to keep the exact same markup.

Comment 10

[Paul Rouget [:paul]]

Attachment: v1.1

try (green): https://tbpl.mozilla.org/?tree=Try&rev=f4fcc366669c

Comment 11

[Paul Rouget [:paul]]

Dao: review ping

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 644935 [details] [diff] [review]
v1.1

>+                <xul:stack flex="1" anonid="browserStack" paul="1">

>+            stack.setAttribute("paul", "2");

?

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 644935 [details] [diff] [review]
v1.1

>             stack.setAttribute("anonid", "browserStack");

>+            browserContainer.setAttribute("anonid", "browserContainer");

>+            sidebarContainer.setAttribute("anonid", "sidebarContainer");

The point of having the anonid attribute is being able to use it for getAnonymousElementByAttribute, which is only needed for the first browser element hardcoded in the markup. You seem to be setting the anonid attribute for styling. You should use classes for that.

Comment 14

[Paul Rouget [:paul]]

Attachment: v2

addressed Dao's comments

Comment 15

[Paul Rouget [:paul]]

Attachment: v1.1

addressed Dao's comments

Comment 16

[Paul Rouget [:paul]]

Comment on attachment 656804 [details] [diff] [review]
v1.1

(sorry, wrong bug)

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 656801 [details] [diff] [review]
v2

>+            sidebarContainer.className = "sidebarContainer";

Please name this "browserSidebarContainer".

>       <constructor>
>         <![CDATA[
>-          this.mCurrentBrowser = this.mPanelContainer.firstChild.firstChild.firstChild.firstChild;
>+          this.mCurrentBrowser = document.getAnonymousElementByAttribute(this, "anonid", "browserStack").firstChild;

Can you just give the browser an anonid, e.g. "initialBrowser", and get rid of the other anonids?

Comment 18

[Paul Rouget [:paul]]

Attachment: v2.1

Addressed Dao's comments

Comment 19

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/8662609802b8

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8662609802b8