Closed Bug 968079 Opened 6 years ago Closed 6 years ago

Update tabs.js metro library to use UI elements only

Categories

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

All
Windows 8.1
defect

Tracking

(firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: AndreeaMatei, Assigned: danisielm)

References

Details

(Whiteboard: [metro])

Attachments

(1 file, 11 obsolete files)

9.73 KB, patch
AndreeaMatei
: review+
whimboo
: review+
andrei
: checkin+
Details | Diff | Splinter Review
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.
Depends on: 880417
Andreea, please work on this once the first patch has been landed. Thanks.
Assignee: nobody → andreea.matei
Priority: -- → P2
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
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.
Assignee: andreea.matei → daniel.gherasim
Status: NEW → ASSIGNED
Attached patch bug_969079_fix_v1.patch (obsolete) — Splinter Review
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.
Attachment #8386022 - Flags: review?(andrei.eftimie)
Attachment #8386022 - Flags: review?(andreea.matei)
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.
Attachment #8386022 - Flags: review?(andrei.eftimie)
Attachment #8386022 - Flags: review?(andreea.matei)
Attachment #8386022 - Flags: review-
(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.");"
(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.
Attached patch bug_969079_fix_v1.2.patch (obsolete) — Splinter Review
Updated patch as requested.
Attachment #8386022 - Attachment is obsolete: true
Attachment #8389125 - Flags: review?(andrei.eftimie)
Attachment #8389125 - Flags: review?(andreea.matei)
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.
Attachment #8389125 - Flags: review?(andrei.eftimie)
Attachment #8389125 - Flags: review?(andreea.matei)
Attachment #8389125 - Flags: review-
Attached patch bug_969079_fix_v1.3.patch (obsolete) — Splinter Review
Updated patch as requested.
Attachment #8389125 - Attachment is obsolete: true
Attachment #8392204 - Flags: review?(andrei.eftimie)
Attachment #8392204 - Flags: review?(andreea.matei)
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.
Attachment #8392204 - Flags: review?(andrei.eftimie)
Attachment #8392204 - Flags: review?(andreea.matei)
Attachment #8392204 - Flags: review-
Attached patch bug_969079_fix_v1.4.patch (obsolete) — Splinter Review
Updated patch as requested.
Attachment #8392204 - Attachment is obsolete: true
Attachment #8392801 - Flags: review?(andrei.eftimie)
Attachment #8392801 - Flags: review?(andreea.matei)
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";
>  }
> });
Attachment #8392801 - Flags: review?(andrei.eftimie)
Attachment #8392801 - Flags: review?(andreea.matei)
Attachment #8392801 - Flags: review-
Attached patch bug_969079_fix_v2.patch (obsolete) — Splinter Review
Patch ready for a final review.
Attachment #8392801 - Attachment is obsolete: true
Attachment #8393526 - Flags: review?(hskupin)
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.
Attachment #8393526 - Flags: review?(hskupin) → review-
Attached patch bug_969079_fix_v2.1.patch (obsolete) — Splinter Review
(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.
Attachment #8393526 - Attachment is obsolete: true
Attachment #8399981 - Flags: review?(andrei.eftimie)
Attachment #8399981 - Flags: review?(andreea.matei)
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.
Attachment #8399981 - Flags: review?(andrei.eftimie)
Attachment #8399981 - Flags: review?(andreea.matei)
Attachment #8399981 - Flags: review-
Attached patch bug_968079_fix_v2.2.patch (obsolete) — Splinter Review
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,
Attachment #8399981 - Attachment is obsolete: true
Attachment #8401301 - Flags: review?(hskupin)
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.
Attachment #8401301 - Flags: review?(hskupin) → review-
Attached patch bug_968079_fix_v2.3.patch (obsolete) — Splinter Review
Used forEach and made requested changes. I hope it's ok now.
Attachment #8401301 - Attachment is obsolete: true
Attachment #8403145 - Flags: review?(andrei.eftimie)
Attachment #8403145 - Flags: review?(andreea.matei)
Comment on attachment 8403145 [details] [diff] [review]
bug_968079_fix_v2.3.patch

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

LGTM
Attachment #8403145 - Flags: review?(hskupin)
Attachment #8403145 - Flags: review?(andrei.eftimie)
Attachment #8403145 - Flags: review?(andreea.matei)
Attachment #8403145 - Flags: review+
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?
Attachment #8403145 - Flags: review?(hskupin) → review-
Attached patch bug_968079_fix_v2.4.patch (obsolete) — Splinter Review
(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.
Attachment #8403145 - Attachment is obsolete: true
Attachment #8406672 - Flags: review?(hskupin)
Attachment #8406672 - Flags: review?(andrei.eftimie)
Attachment #8406672 - Flags: review?(hskupin)
Attachment #8406672 - Flags: review?(andreea.matei)
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.
Attachment #8406672 - Flags: review?(andrei.eftimie)
Attachment #8406672 - Flags: review?(andreea.matei)
Attachment #8406672 - Flags: review+
Attached patch bug_968079_fix_v2.5.patch (obsolete) — Splinter Review
Changes were done.
Attachment #8406672 - Attachment is obsolete: true
Attachment #8408024 - Flags: review?(hskupin)
(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 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.
Attachment #8408024 - Flags: review?(hskupin) → review-
(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 ?
(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.
(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 ?
(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.
Attached patch bug_968079_fix_v2.6.patch (obsolete) — Splinter Review
Attachment #8408024 - Attachment is obsolete: true
Attachment #8408212 - Flags: review?(andreea.matei)
Sorry, missed some nits.
Attachment #8408212 - Attachment is obsolete: true
Attachment #8408212 - Flags: review?(andreea.matei)
Attachment #8408232 - Flags: review?(andreea.matei)
Comment on attachment 8408232 [details] [diff] [review]
bug_968079_fix_v2.7.patch

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

Great, thanks Daniel.
Attachment #8408232 - Flags: review?(hskupin)
Attachment #8408232 - Flags: review?(andreea.matei)
Attachment #8408232 - Flags: review+
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.
Attachment #8408232 - Flags: review?(hskupin) → review+
(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 ?
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.
(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.
Hm, not sure what I have seen this morning. Looks like we are fine here. Sorry.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.