968079 - Update tabs.js metro library to use UI elements only

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

At this moment, the getters for length (number of open tabs) and selectedIndex use controller.tabs instead of actual UI elements. This is a follow-up bug for bug 880417.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Andreea, please work on this once the first patch has been landed. Thanks.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If you reference this bug in other bugs make sure to update the tasks which have to be done here. So beside the above items, also the .strip code should be removed so we get the elements only via the nodeCollector.

Here I think we should be able to skip the element. It's just an anonymous scrollbox. I don't think we need it. Just jump over it.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/tabs.xml#139

Comment 3

[Andreea Matei [:AndreeaMatei]]

I haven't found a way to jump over that strip element and have a patch ready here. Daniel will continue to look on this as I'm duty this week and he waits for review on his metro tests.

Comment 4

[Daniel Gherasim]

Attachment: bug_969079_fix_v1.patch

This is the XBL widget for the tabs:

> <hbox id="tabs-container">
>   <box id="tabs">
>     <!-- BINDED ELEMENTS -->
>       <xul:arrowscrollbox anonid = "tabs-scrollbox">
>         <documenttab />
>         <documenttab />
>         <documenttab selected="true"/>
>         <documenttab />
>       </xul:arrowscrollbox>
>     <!-- END -->
>   </box>
> </hbox>

I coulnd't find a way to skip the 'tabs-scrollbox' to get the 'documenttab' elements. We can get it now with 'getAnonymousElementByAttribute' and then query for the nodes with nodeCollector.

Comment 5

[Andrei Eftimie]

Comment on attachment 8386022 [details] [diff] [review]
bug_969079_fix_v1.patch

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

::: metrofirefox/lib/ui/tabs.js
@@ +67,5 @@
> +    var tabs = this.getElements({type: "tabs"});
> +    var index = 0;
> +    for(var i in tabs) {
> +      var selected = tabs[i].getNode().getAttribute("selected");
> +      if(selected !== undefined && !!selected == true) {

The second check won't work. Any string is truthy.
> !!"false" // true

And if we're changing the second check we don't need to do the first one either:
> if (selected === "true")

This will be false even if `selected` is undefined.

@@ +68,5 @@
> +    var index = 0;
> +    for(var i in tabs) {
> +      var selected = tabs[i].getNode().getAttribute("selected");
> +      if(selected !== undefined && !!selected == true) {
> +        index = i;

We can return directly here and skip using `index`.
This might also improve our performance since we'll skip the rest of the tabs once we find the active one.

@@ +77,5 @@
> +
> +  /**
> +   * Select the tab with the given index
> +   *
> +   * @param {number} index

nit: the name is aIndex

@@ +83,3 @@
>     */
>    set selectedIndex(aIndex) {
> +    var tab = this.getTab(aIndex);

What happens for an index outside the current number of tabs?
I think we should probably bail.

Comment 6

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #5)
> > +    var tabs = this.getElements({type: "tabs"});
> > +    var index = 0;
> > +    for(var i in tabs) {
> > +      var selected = tabs[i].getNode().getAttribute("selected");
> > +      if(selected !== undefined && !!selected == true) {
> 
> The second check won't work. Any string is truthy.
> > !!"false" // true
> 
> And if we're changing the second check we don't need to do the first one
> either:
> > if (selected === "true")
> 
> This will be false even if `selected` is undefined.

Given that the selected attribute is only set to the visible tab, 
would be ok to use  this : 
> if(tabs[i].getNode().hasAttribute("selected")) 
?

> 
> @@ +83,3 @@
> >     */
> >    set selectedIndex(aIndex) {
> > +    var tab = this.getTab(aIndex);
> 
> What happens for an index outside the current number of tabs?
> I think we should probably bail.

Is it enough to check for the tab existence here ?
> assert.ok(tab.exists(), "Tab has been found.");"

Comment 7

[Andrei Eftimie]

(In reply to daniel.gherasim from comment #6)
> (In reply to Andrei Eftimie from comment #5)
> > > +    var tabs = this.getElements({type: "tabs"});
> > > +    var index = 0;
> > > +    for(var i in tabs) {
> > > +      var selected = tabs[i].getNode().getAttribute("selected");
> > > +      if(selected !== undefined && !!selected == true) {
> > 
> > The second check won't work. Any string is truthy.
> > > !!"false" // true
> > 
> > And if we're changing the second check we don't need to do the first one
> > either:
> > > if (selected === "true")
> > 
> > This will be false even if `selected` is undefined.
> 
> Given that the selected attribute is only set to the visible tab, 
> would be ok to use  this : 
> > if(tabs[i].getNode().hasAttribute("selected")) 
> ?

If `selected` only exists on the current tab then it should be fine.
Please check to make sure however if once a tab has been active, and lost its active status that we don't have the `selected` attribute (but with `false` as a value).

> > 
> > @@ +83,3 @@
> > >     */
> > >    set selectedIndex(aIndex) {
> > > +    var tab = this.getTab(aIndex);
> > 
> > What happens for an index outside the current number of tabs?
> > I think we should probably bail.
> 
> Is it enough to check for the tab existence here ?
> > assert.ok(tab.exists(), "Tab has been found.");"

I think a check bit more robust might be needed. 
Maybe even include this in one of the lib tests to try retrieving a tab with an index that is out of bounds.

Comment 8

[Daniel Gherasim]

Attachment: bug_969079_fix_v1.2.patch

Updated patch as requested.

Comment 9

[Andrei Eftimie]

Comment on attachment 8389125 [details] [diff] [review]
bug_969079_fix_v1.2.patch

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

::: metrofirefox/lib/ui/tabs.js
@@ +65,5 @@
>     */
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +
> +    for(var i in tabs) {

You could use:
> tabs.forEach((tab, i) => { [...] })

@@ +66,5 @@
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +
> +    for(var i in tabs) {
> +      if(tabs[i].getNode().hasAttribute("selected") &&

nit: missing space

@@ +67,5 @@
> +    var tabs = this.getElements({type: "tabs"});
> +
> +    for(var i in tabs) {
> +      if(tabs[i].getNode().hasAttribute("selected") &&
> +         !!tabs[i].getNode().getAttribute("selected") === true) {

This will not work because:
> !!"false" // true

You'll need to check for "true" as a string.

@@ +68,5 @@
> +
> +    for(var i in tabs) {
> +      if(tabs[i].getNode().hasAttribute("selected") &&
> +         !!tabs[i].getNode().getAttribute("selected") === true) {
> +        return parseInt(i);

This will already be an integer if you use forEach for the iteration.

@@ +83,4 @@
>     */
>    set selectedIndex(aIndex) {
> +    var tab = this.getTab(aIndex);
> +    assert.ok(tab !== undefined && tab.exists(), "Tab has been found.");

This check should be in getTab().

I've tested and if we try to select a tab with an index greater than the number of opened tabs we just return `undefined`.
We should fail there.

@@ +85,5 @@
> +    var tab = this.getTab(aIndex);
> +    assert.ok(tab !== undefined && tab.exists(), "Tab has been found.");
> +    tab.tap();
> +    var self = this;
> +    assert.waitFor(function () {

You can use fat arrow function here.

Comment 10

[Daniel Gherasim]

Attachment: bug_969079_fix_v1.3.patch

Updated patch as requested.

Comment 11

[Andrei Eftimie]

Comment on attachment 8392204 [details] [diff] [review]
bug_969079_fix_v1.3.patch

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

Just a few more changes and we should be good.
Thanks.

::: metrofirefox/lib/ui/tabs.js
@@ +68,5 @@
> +    var index = -1;
> +    tabs.forEach((tab, i) => {
> +      if (tabs[i].getNode().hasAttribute("selected") &&
> +          tabs[i].getNode().getAttribute("selected") === "true") {
> +        index = i;

I'm not sold on the need to use the `index` variable.
You can return `i` directly here.

@@ +72,5 @@
> +        index = i;
> +        return;
> +      }
> +    });
> +    return index;

And return -1 here.
With these, you can remove `index` altogether.

@@ +84,5 @@
>     */
>    set selectedIndex(aIndex) {
> +    var tab = this.getTab(aIndex);
> +    tab.tap();
> +    var self = this;

You don't need to use self anymore.
Fat arrow functions bind `this` to the enclosing scope.

@@ +172,5 @@
>     *
>     * @returns {ElemBase} The requested tab
>     */
>    getTab : function tabBrowser_getTab(aIndex) {
>      if (aIndex === undefined)

While we are here please update this to use curly braces.

@@ +175,5 @@
>    getTab : function tabBrowser_getTab(aIndex) {
>      if (aIndex === undefined)
>        aIndex = this.selectedIndex;
>  
> +    var tabsList = this.getElements({type: "tabs"});

I would name this `tabList`.
The suffix `List` already implies a multitude of tabs.

@@ +176,5 @@
>      if (aIndex === undefined)
>        aIndex = this.selectedIndex;
>  
> +    var tabsList = this.getElements({type: "tabs"});
> +    assert.ok(tabsList[aIndex] !== undefined && tabsList[aIndex].exists(),

Just checking for `tabsList[aIndex]` should be enough in the first check. `undefined` is falsy.

Comment 12

[Daniel Gherasim]

Attachment: bug_969079_fix_v1.4.patch

Updated patch as requested.

Comment 13

[Andrei Eftimie]

Comment on attachment 8392801 [details] [diff] [review]
bug_969079_fix_v1.4.patch

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

Looks good.
Please ask a review from Henrik with the small update mentioned.

::: metrofirefox/lib/ui/tabs.js
@@ +65,5 @@
>     */
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +    var index = -1;
> +    tabs.some((tab, i) => {

I like the use of `some` here. But with this, you can further simplify the code a bit:
> tabs.some((tab, i) => {
>  index = i;
>  return tab.getNode().hasAttribute("selected") &&
>         tab.getNode().getAttribute("selected") === "true";
>  }
> });

Comment 14

[Daniel Gherasim]

Attachment: bug_969079_fix_v2.patch

Patch ready for a final review.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8393526 [details] [diff] [review]
bug_969079_fix_v2.patch

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

::: metrofirefox/lib/ui/tabs.js
@@ +65,5 @@
>     */
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +    var index = -1;
> +    tabs.some((tab, i) => {

Please add an empty line above for separation. Also instead of some() you want to use reduce(). It will finally return the index. So no need to have another variable.

@@ +68,5 @@
> +    var index = -1;
> +    tabs.some((tab, i) => {
> +      index = i;
> +      return tabs[i].getNode().hasAttribute("selected") &&
> +             tabs[i].getNode().getAttribute("selected") === "true";

Given that getAttribute() would return null if the attribute does not exist, it will be enough to have the second check for true.

@@ +70,5 @@
> +      index = i;
> +      return tabs[i].getNode().hasAttribute("selected") &&
> +             tabs[i].getNode().getAttribute("selected") === "true";
> +    });
> +    return index;

Please separate the return line by an empty line.

@@ +82,5 @@
>     */
>    set selectedIndex(aIndex) {
> +    var tab = this.getTab(aIndex);
> +    tab.tap();
> +    assert.waitFor(() => this.selectedIndex === aIndex,

Brackets please around the comparison.

@@ +128,5 @@
>      var document = this._controller.window.document;
>  
>      switch(aSpec.type) {
>        case "closeButton":
> +        var node = document.getAnonymousElementByAttribute(this.getTab().getNode(),

Lets use the nodeCollector here.

@@ +146,5 @@
>          elem = new findElement.ID(document, "tabs-container");
>          break;
>        case "tabs":
> +        var tabs = new findElement.ID(document, "tabs").getNode();
> +        var root = document.getAnonymousElementByAttribute(tabs, "anonid", "tabs-scrollbox");

Please already make use of the node collector here. Also why can't we directly reach documenttab nodes?

@@ +151,3 @@
>          var nodeCollector = new domUtils.nodeCollector(root);
> +        nodeCollector.queryNodes("documenttab");
> +        return nodeCollector.elements;

no return here but an assignment to elem.

@@ +165,3 @@
>     * Get the tab at the specified index
>     *
>     * @param {number} aIndex

aIndex is optional

@@ +175,5 @@
> +    }
> +
> +    var tabList = this.getElements({type: "tabs"});
> +    assert.ok(tabList[aIndex] && tabList[aIndex].exists(),
> +              "Tab has been found.");

If we want to do that please also add the specified index to the message string.

Comment 16

[Daniel Gherasim]

Attachment: bug_969079_fix_v2.1.patch

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 8393526 [details] [diff] [review]
> bug_969079_fix_v2.patch
> 
> Review of attachment 8393526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: metrofirefox/lib/ui/tabs.js
> @@ +65,5 @@
> >     */
> >    get selectedIndex() {
> > +    var tabs = this.getElements({type: "tabs"});
> > +    var index = -1;
> > +    tabs.some((tab, i) => {
> 
> Please add an empty line above for separation. Also instead of some() you
> want to use reduce(). It will finally return the index. So no need to have
> another variable.
> 

Using reduce we won't stop when we find the tab. 
So a simple 'for' I think it's better here.

> @@ +146,5 @@
> >          elem = new findElement.ID(document, "tabs-container");
> >          break;
> >        case "tabs":
> > +        var tabs = new findElement.ID(document, "tabs").getNode();
> > +        var root = document.getAnonymousElementByAttribute(tabs, "anonid", "tabs-scrollbox");
> 
> Please already make use of the node collector here. Also why can't we
> directly reach documenttab nodes?
> 

The parent of the documenttab nodes is also binded so we fail when trying to get them.
This solution works for now, I also tried with nodeCollector but it doesn't work.

Comment 17

[Andrei Eftimie]

Comment on attachment 8399981 [details] [diff] [review]
bug_969079_fix_v2.1.patch

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

Please ask a review from Henrik with these fixes.

::: metrofirefox/lib/ui/tabs.js
@@ +66,5 @@
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +
> +    for(var i in tabs) {
> +      if(tabs[i].getNode().getAttribute("selected") === "true") {

nit: please add a space before the opening round bracket for both the `for` and the `if`

@@ +83,5 @@
>     */
>    set selectedIndex(aIndex) {
> +    var tab = this.getTab(aIndex);
> +    tab.tap();
> +    assert.waitFor(() => (this.selectedIndex === aIndex),

The requested change was for curly braces. You'll also need to add the `return` keyword

@@ +131,5 @@
>      switch(aSpec.type) {
>        case "closeButton":
> +        var nodeCollector = new domUtils.nodeCollector(aSpec.value.getNode());
> +        nodeCollector.queryAnonymousNode("anonid", "close");
> +        elem = nodeCollector.elements[0];

You should leave the whole array here.

Comment 18

[Daniel Gherasim]

Attachment: bug_968079_fix_v2.2.patch

Henrik, I just can't skip the anonymous element 'tabs-scrollbox'.

This is the last 5 elements in stack for an 'documenttab' element:

vbox(id="tray") > hbox(id="tabs-container") > box(id="tabs") > arrowscrollbox(anonid="tabs-scrollbox") > documenttab([selected="true"])

Last 2 are binded, having the root as "tabs" and querying for documenttab we get an empty array all the time. Do you have any idea why is this happening and why we can't skip it ?

In rest I made the requested changes and it works fine.
Thanks,

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8401301 [details] [diff] [review]
bug_968079_fix_v2.2.patch

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

r- mostly because of the strange code in selectedIndex. Otherwise just nits. So one more round and then we can most likely call it finished.

::: metrofirefox/lib/ui/tabs.js
@@ +69,5 @@
> +    for (var i in tabs) {
> +      if (tabs[i].getNode().getAttribute("selected") === "true") {
> +        return parseInt(i);
> +      }
> +    }

Well, in that case please make it a 'forEach(function (aItem, aIndex))'. I don't see how parseInt() will work on a MozMillElement.

@@ +71,5 @@
> +        return parseInt(i);
> +      }
> +    }
> +
> +    return -1;

Shouldn't we always have a tab selected?

@@ +83,4 @@
>     */
>    set selectedIndex(aIndex) {
> +    var tab = this.getTab(aIndex);
> +    tab.tap();

nit: tab seems to be not necessary here. Why not directly calling getTab().tap()?

@@ +129,5 @@
>      var document = this._controller.window.document;
>  
>      switch(aSpec.type) {
>        case "closeButton":
> +        var nodeCollector = new domUtils.nodeCollector(aSpec.value.getNode());

Create the instance before the switch statement. Same for the case below.

@@ +131,5 @@
>      switch(aSpec.type) {
>        case "closeButton":
> +        var nodeCollector = new domUtils.nodeCollector(aSpec.value.getNode());
> +        nodeCollector.queryAnonymousNode("anonid", "close");
> +        elem = nodeCollector.elements;

Call it elems and always assign an array, similar to other Firefox ui modules. That way you don't need the check for isArray at the end.

@@ +151,5 @@
> +        var nodeCollector = new domUtils.nodeCollector(tabs);
> +        nodeCollector.queryAnonymousNode("anonid", "tabs-scrollbox");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryNodes("documenttab");
> +        elem = nodeCollector.elements;

If there is no other way, lets keep it as it is. No need to spend more time on it.

Comment 20

[Daniel Gherasim]

Attachment: bug_968079_fix_v2.3.patch

Used forEach and made requested changes. I hope it's ok now.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8403145 [details] [diff] [review]
bug_968079_fix_v2.3.patch

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

LGTM

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8403145 [details] [diff] [review]
bug_968079_fix_v2.3.patch

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

::: metrofirefox/lib/ui/tabs.js
@@ +64,5 @@
>     * @returns {Number} Index of the active tab
>     */
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +    var index = 0;

So you updated your code by a question, which I have raised last time. The change was not good, so I would suggest that zou revert the initial value to -1. This is just in case we hit a problem with tabs, and none is selected. I had such a problem already twice but I'm not able to reproduce it.

@@ +66,5 @@
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +    var index = 0;
> +    tabs.forEach(function (aItem, aIndex) {
> +      if(tabs[aIndex].getNode().getAttribute("selected") === "true")

Why aren't you using 'aItem.getNode()'? Also there is a missing blank after 'if'.

@@ +127,2 @@
>      var document = this._controller.window.document;
> +    var nodeCollector = new domUtils.nodeCollector(document);

I don't see why we have to use document as root here. Shouldn't the tabs container (findElement.ID(document, "tabs")) do it?

@@ +129,4 @@
>  
>      switch(aSpec.type) {
>        case "closeButton":
> +        nodeCollector.root = aSpec.value.getNode();

Should we assert for a valid value?

Comment 23

[Daniel Gherasim]

Attachment: bug_968079_fix_v2.4.patch

(In reply to Henrik Skupin (:whimboo) from comment #22)
> Comment on attachment 8403145 [details] [diff] [review]
> @@ +127,2 @@
> >      var document = this._controller.window.document;
> > +    var nodeCollector = new domUtils.nodeCollector(document);
> 
> I don't see why we have to use document as root here. Shouldn't the tabs
> container (findElement.ID(document, "tabs")) do it?
> 

It does but we change the root in every case so at this point it doesn't matter what the root is. 

> @@ +129,4 @@
> >  
> >      switch(aSpec.type) {
> >        case "closeButton":
> > +        nodeCollector.root = aSpec.value.getNode();
> 
> Should we assert for a valid value?

We should, I did that.

Comment 24

[Andrei Eftimie]

Comment on attachment 8406672 [details] [diff] [review]
bug_968079_fix_v2.4.patch

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

Looks good to me, r+ with the mentioned nit fixed.

::: metrofirefox/lib/ui/tabs.js
@@ +66,5 @@
>    get selectedIndex() {
> +    var tabs = this.getElements({type: "tabs"});
> +    var index = -1;
> +    tabs.forEach(function (aItem, aIndex) {
> +      if (aItem.getNode().getAttribute("selected") === "true")

As per our style guide, please always use curly braces.

Comment 25

[Daniel Gherasim]

Attachment: bug_968079_fix_v2.5.patch

Changes were done.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to daniel.gherasim from comment #23)
> > I don't see why we have to use document as root here. Shouldn't the tabs
> > container (findElement.ID(document, "tabs")) do it?
> 
> It does but we change the root in every case so at this point it doesn't
> matter what the root is. 

Oh it does. At least when we want to add more elements to the switch condition. Usually you always want to have the top-most node for the kind of UI you are wrapping here as root. In case of tabs it will be the tabs container. If specific cases need a root set which is located deeper in the tree, it's up for those cases, but it should not affect all the others. Also doing a search for nodes via a selector will take longer started from the document as root as when done via the tabs container.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8408024 [details] [diff] [review]
bug_968079_fix_v2.5.patch

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

We are close and I think the next version should be the final one. If you have questions to the comments inline please let me know. Otherwise lets get this done today.

::: metrofirefox/lib/ui/tabs.js
@@ +126,5 @@
>    getElements : function tabBrowser_getElements(aSpec) {
> +    var spec = aSpec || {};
> +    var elems = null;
> +    var root = new findElement.ID(this._controller.window.document,
> +                                  "tabs-container");

Instead of specifying a fixed root here, even with a call to findElement.ID(), you should make it conditionally like the getElements() method in addons.js: 

var root = parent ? parent.getNode() : this._controller.tabs.activeTab;

In our case we would call getElements() initially in the constructor of the class with the document as parent. That way we get the element we can cache and make use of for getElements() calls without a parent parameter.

@@ +151,3 @@
>          break;
>        case "tabsContainer":
> +        elems = [root];

Also same applies here to findElement.* calls. All of them should be part of the switch block, and not happen before it like you did above.

Comment 28

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Comment on attachment 8408024 [details] [diff] [review]
> bug_968079_fix_v2.5.patch
> 
> Review of attachment 8408024 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We are close and I think the next version should be the final one. If you
> have questions to the comments inline please let me know. Otherwise lets get
> this done today.
> 
> ::: metrofirefox/lib/ui/tabs.js
> @@ +126,5 @@
> >    getElements : function tabBrowser_getElements(aSpec) {
> > +    var spec = aSpec || {};
> > +    var elems = null;
> > +    var root = new findElement.ID(this._controller.window.document,
> > +                                  "tabs-container");
> 
> Instead of specifying a fixed root here, even with a call to
> findElement.ID(), you should make it conditionally like the getElements()
> method in addons.js: 
> 
> var root = parent ? parent.getNode() : this._controller.tabs.activeTab;
> 
> In our case we would call getElements() initially in the constructor of the
> class with the document as parent. That way we get the element we can cache
> and make use of for getElements() calls without a parent parameter.
> 
> @@ +151,3 @@
> >          break;
> >        case "tabsContainer":
> > +        elems = [root];
> 
> Also same applies here to findElement.* calls. All of them should be part of
> the switch block, and not happen before it like you did above.

From what I understand from the last comment, you want something like this : 
In the constructor of TabBrowser : 
> this._tabsContainerNode = this.getElement({type: "tabsContainer",
>                                            parent: this._controller.window.document}).getNode();

and then in getElements : 
> var root = parent ? parent : this._tabsContainerNode;
> var nodeCollector = new domUtils.nodeCollector(root);

and for getting the elements we will be using the root node. (findElement.ID(root,___))

In that case for elements are not in the tabsContainer (5 out of 7 elements), we should define a parent (most likely window.document). 

Is that correct ?

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to daniel.gherasim from comment #28)
> From what I understand from the last comment, you want something like this : 
> In the constructor of TabBrowser : 
> > this._tabsContainerNode = this.getElement({type: "tabsContainer",
> >                                            parent: this._controller.window.document}).getNode();

Correct. I even would define a _root property for that. I think we have to same on other ui classes already. So it's identical class design. But I think we should keep the element and not save it as a DOM node.

> and then in getElements : 
> > var root = parent ? parent : this._tabsContainerNode;
> > var nodeCollector = new domUtils.nodeCollector(root);

Right.

> and for getting the elements we will be using the root node.
> (findElement.ID(root,___))

This may not work for findElement but only for nodeCollector. You will have to test that.

> In that case for elements are not in the tabsContainer (5 out of 7
> elements), we should define a parent (most likely window.document). 

Which are outside, and how many levels apart? In such a case the tabs container would not be the root.

Comment 30

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #29)
> (In reply to daniel.gherasim from comment #28)
> > In that case for elements are not in the tabsContainer (5 out of 7
> > elements), we should define a parent (most likely window.document). 
> 
> Which are outside, and how many levels apart? In such a case the tabs
> container would not be the root.

Tray is the parent of the tabsContainer, might be set as root but we have the sidebar buttons which are under window (id=main-window) > stack (id=stack). 

So stack is the closest element that is the parent of all our needed elements. Should I use it or use window.document as root ?

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to daniel.gherasim from comment #30)
> Tray is the parent of the tabsContainer, might be set as root but we have
> the sidebar buttons which are under window (id=main-window) > stack
> (id=stack). 

Those buttons then might not fit well into the tabbrowser. I don't want to see a refactoring as of now, so lets keep them where they are and just add a comment that both would have to be moved somewhere else.

Beside that if nothing else is located beside tabs-container, I would keep it as parent.

Comment 32

[Daniel Gherasim]

Attachment: bug_968079_fix_v2.6.patch

Comment 33

[Daniel Gherasim]

Attachment: bug_968079_fix_v2.7.patch

Sorry, missed some nits.

Comment 34

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8408232 [details] [diff] [review]
bug_968079_fix_v2.7.patch

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

Great, thanks Daniel.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8408232 [details] [diff] [review]
bug_968079_fix_v2.7.patch

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

With the following two nits fixed, you have my r+

::: metrofirefox/lib/ui/tabs.js
@@ -233,5 @@
>      function checkTabAnimationEnd() { self.animationend = true; }
>  
>      // Add event listener to wait until the tab has been opened
>      this._controller.window.addEventListener("TabOpen", checkTabOpened);
> -    this._tabsContainerNode.addEventListener("animationend", checkTabAnimationEnd);

_tabsContainerNode is not needed anymore. So please remove its declaration.

@@ -302,5 @@
>      function checkTabClosed() { self.closed = true; }
>      function checkTabAnimationEnd() { self.animationend = true; }
>  
>      this._controller.window.addEventListener("TabClose", checkTabClosed, false);
> -    this._tabsContainerNode.addEventListener("animationend", checkTabAnimationEnd);

Same here.

Comment 36

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #35)
> Comment on attachment 8408232 [details] [diff] [review]
> bug_968079_fix_v2.7.patch
> 
> Review of attachment 8408232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With the following two nits fixed, you have my r+
> 
> ::: metrofirefox/lib/ui/tabs.js
> @@ -233,5 @@
> >      function checkTabAnimationEnd() { self.animationend = true; }
> >  
> >      // Add event listener to wait until the tab has been opened
> >      this._controller.window.addEventListener("TabOpen", checkTabOpened);
> > -    this._tabsContainerNode.addEventListener("animationend", checkTabAnimationEnd);
> 
> _tabsContainerNode is not needed anymore. So please remove its declaration.
> 
> @@ -302,5 @@
> >      function checkTabClosed() { self.closed = true; }
> >      function checkTabAnimationEnd() { self.animationend = true; }
> >  
> >      this._controller.window.addEventListener("TabClose", checkTabClosed, false);
> > -    this._tabsContainerNode.addEventListener("animationend", checkTabAnimationEnd);
> 
> Same here.

Hmm, those lines are already removed (they are with - in front) & replaced with this._root.getNode() in that patch, or you were reffering to something else ?

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't refer to its usage but its declaration. Please check some lines above all those code. We don't have to retrieve an element which is not used anymore.

Comment 38

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #37)
> I don't refer to its usage but its declaration. Please check some lines
> above all those code. We don't have to retrieve an element which is not used
> anymore.

Do you mean the oldest 
> this._tabsContainerNode = this.getElement({type: "tabsContainer"}).getNode(); 
which have been renamed to 
> this._root = this.getElement({type: "tabsContainer", parent: this._controller.window.document});
because I don't find any declaration or existance of _tabsContainerNode in the latest patch.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hm, not sure what I have seen this morning. Looks like we are fine here. Sorry.

Comment 40

[Andrei Eftimie]

Comment on attachment 8408232 [details] [diff] [review]
bug_968079_fix_v2.7.patch

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/ecb8015fdc1b (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/848f3952232c (mozilla-aurora)