It's impossible to add a sidebar inside a tab

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
7 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

7 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

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

Comment 2

7 years ago
Posted patch draft (obsolete) — Splinter Review
Reporter

Comment 3

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

Updated

7 years ago
Blocks: 707906
Reporter

Updated

7 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

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

Updated

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

Comment 6

7 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

7 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

7 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

7 years ago
Posted patch v1.1 (obsolete) — Splinter Review
try (green): https://tbpl.mozilla.org/?tree=Try&rev=f4fcc366669c
Reporter

Updated

7 years ago
Attachment #637025 - Attachment is obsolete: true
Reporter

Updated

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

Updated

7 years ago
Blocks: 782593
Reporter

Comment 11

7 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

7 years ago
Posted patch v2 (obsolete) — Splinter Review
addressed Dao's comments
Reporter

Updated

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

Updated

7 years ago
Attachment #644935 - Attachment is obsolete: true
Reporter

Comment 15

7 years ago
Posted patch v1.1 (obsolete) — Splinter Review
addressed Dao's comments
Reporter

Updated

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

Comment 16

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

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

Updated

7 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

7 years ago
Posted patch v2.1Splinter Review
Addressed Dao's comments
Reporter

Updated

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

Updated

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

Updated

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

Comment 19

7 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: 7 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.