The default bug view has changed. See this FAQ.

It's impossible to add a sidebar inside a tab

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Unassigned)

Tracking

Trunk
Firefox 18
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
I suggest that we introduce a <hbox> around the browser and inside the tab .
(Reporter)

Comment 2

5 years ago
Created attachment 636691 [details] [diff] [review]
draft
(Reporter)

Comment 3

5 years ago
This would make our life much easier. We could add tools inside this <hbox>.
(Reporter)

Updated

5 years ago
Blocks: 707906
(Reporter)

Updated

5 years ago
Attachment #636691 - Flags: review?(dolske)
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.
(Reporter)

Comment 5

5 years ago
Created attachment 637025 [details] [diff] [review]
v1
(Reporter)

Updated

5 years ago
Attachment #636691 - Attachment is obsolete: true
Attachment #636691 - Flags: review?(dolske)
(Reporter)

Comment 6

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

Updated

5 years ago
Attachment #637025 - Flags: review?(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 on attachment 637025 [details] [diff] [review]
v1

see previous comment
Attachment #637025 - Flags: review?(dao) → review-
(Reporter)

Comment 9

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

Comment 10

5 years ago
Created attachment 644935 [details] [diff] [review]
v1.1

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

Updated

5 years ago
Attachment #637025 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #644935 - Flags: review?(dao)
(Reporter)

Updated

5 years ago
Blocks: 782593
(Reporter)

Comment 11

5 years ago
Dao: review ping
Comment on attachment 644935 [details] [diff] [review]
v1.1

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

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

?
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.
Attachment #644935 - Flags: review?(dao) → review-
(Reporter)

Comment 14

5 years ago
Created attachment 656801 [details] [diff] [review]
v2

addressed Dao's comments
(Reporter)

Updated

5 years ago
Attachment #656801 - Flags: review?(dao)
(Reporter)

Updated

5 years ago
Attachment #644935 - Attachment is obsolete: true
(Reporter)

Comment 15

5 years ago
Created attachment 656804 [details] [diff] [review]
v1.1

addressed Dao's comments
(Reporter)

Updated

5 years ago
Attachment #656801 - Attachment is obsolete: true
Attachment #656801 - Flags: review?(dao)
(Reporter)

Comment 16

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

(sorry, wrong bug)
Attachment #656804 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #656801 - Attachment is obsolete: false
Attachment #656801 - Flags: review?(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?
(Reporter)

Comment 18

5 years ago
Created attachment 658042 [details] [diff] [review]
v2.1

Addressed Dao's comments
(Reporter)

Updated

5 years ago
Attachment #656801 - Attachment is obsolete: true
Attachment #656801 - Flags: review?(dao)
(Reporter)

Updated

5 years ago
Attachment #658042 - Flags: review?(dao)

Updated

5 years ago
Attachment #658042 - Flags: review?(dao) → review+
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Reporter)

Comment 19

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/8662609802b8
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8662609802b8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.