638270 - TabView.getGroups() fails for active group

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Attachment: Sample test

Trying to get a reference to the active tab group fails when using the preferred method TabView.getGroups().

Case 1 Fails "parent.getNode is not a function": 
var activeGroup = activeTabView.getGroups({filter: "active"});
var newTabButton = activeTabView.getElement({
  type: "group_newTabButton",
  parent: activeGroup
});

Case 2 Passes:
var activeGroup = activeTabView.getElement({
  type: "groups",
  subtype: "active"
});
var newTabButton = activeTabView.getElement({
  type: "group_newTabButton",
  parent: activeGroup
});

Comment 1

[u279076]

After some initial debugging, it appears as though Spec.filter is being lost.

Spec.filter == "active" inside getGroups()
Spec.filter == undefined inside getElements()

Comment 2

[u279076]

(In reply to comment #1)
> After some initial debugging, it appears as though Spec.filter is being lost.
> 
> Spec.filter == "active" inside getGroups()
> Spec.filter == undefined inside getElements()

Nevermind, I failed to see that this was being set to spec.subtype in getElements().  I'll continue to debug further.

Comment 3

[u279076]

Unrelated to this bug, but I think I see a typo in the API:

getElement : function tabView_getElement(aSpec) {
getElements : function tabView_getElement(aSpec) {

Should getElements not have an 's' at the end?

Comment 4

[u279076]

Speaking with Aaron on IRC, he thinks:
"nodeCollector will require some adjustments in relation to the DOM hierarchy change as of recent in comparison to what's in the API"

Aaron, can you please elaborate? Should we get another bug on file?

Comment 5

[Aaron Train [:aaronmt]]

(In reply to comment #4)
> Speaking with Aaron on IRC, he thinks:
> "nodeCollector will require some adjustments in relation to the DOM hierarchy
> change as of recent in comparison to what's in the API"
> 
> Aaron, can you please elaborate? Should we get another bug on file?
Well, you omitted the I think. I had trouble in the previous WIP patch from the refactor getting certain nodes from getElements, the nodeCollector was returning empty for example on specifying the exit/search elements. Perhaps the hierarchy changed and this needs adjustment, perhaps not. Check what happens when you change comment #3 first

Comment 6

[u279076]

(In reply to comment #5)
> Check what happens when you change comment #3 first

Well, I don't want to change it unless it is really a typo. However, in testing, changing it has no effect on the error.

Where is nodeCollector source code? Any suggestions on debugging the hierarchy?

Comment 7

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/qa/mozmill-tests/file/f65431495a98/shared-modules/tabview.js#l572

nodeCollector is used for each case in the switch in getElements. DOM Inspector will show the hierarchy.

Comment 8

[u279076]

(In reply to comment #7)
> http://hg.mozilla.org/qa/mozmill-tests/file/f65431495a98/shared-modules/tabview.js#l572
> 
> nodeCollector is used for each case in the switch in getElements. DOM Inspector
> will show the hierarchy.

Thanks, I'll see if I can investigate this further.

Comment 9

[u279076]

I've discovered why this fails.  It's because getGroups() returns an array.  In cases where we just want the active group, this means an array of a single element.  The reason this fails is because we are trying to do array.getNode() instead of array[0].getNode() so it fails.

My initial proposal is to add another function to the Tab View API specific to active tab groups so that we only return the first element of the array.  This should be fairly safe as there should be only one active group (we can add assertions to the function for extra measure if desired).

  getActiveGroup : function tabView_getActiveGroup(aSpec) {
    var spec = aSpec || {};
    
    // There should only be one 'active' group so return the first index
    return this.getGroups(aSpec)[0];
  },

Comment 10

[u279076]

NOTE: I've tested this and it corrects the failure in bug 636191.

Comment 11

[u279076]

Actually, we probably don't even need aSpec in this function so it can be simplified down to the return statement.

Comment 12

[u279076]

There are two other alternatives to what I propose in comment 9:

1) We simply add an IF to getGroups() to return an element if getElements().length is 1 or an array if getElements().length > 1

2) Leave handling of the array up to the test, so var activeGroup = activeTabView.getGroups({filter: "active"})[0];

Comment 13

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

(In reply to comment #12)
> 1) We simply add an IF to getGroups() to return an element if
> getElements().length is 1 or an array if getElements().length > 1

No. As the method name states it will always return an array. You can't return a single element in cases when only one group is present. That will break the API.

(In reply to comment #9)
>   getActiveGroup : function tabView_getActiveGroup(aSpec) {
>     var spec = aSpec || {};
> 
>     // There should only be one 'active' group so return the first index
>     return this.getGroups(aSpec)[0];
>   },

Feel free to add this. But instead of a function it should be a getter, so you can do tabView.activeGroup. As you say no spec is needed here, just hard-code the filter for the getGroups call.

Beside that always check the firefox/helperClasses/tabView.js file which contains basic tests and which should work. It would have saved you some time with the investigation process. If you add the above method please also update the test itself. Thanks.

Comment 14

[u279076]

Attachment: Patch v1

Adds a getter for the active tab group element.

Comment 15

[u279076]

Attachment: Patch v1.1

Made a small revision.

Comment 16

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

Comment on attachment 516620 [details] [diff] [review]
Patch v1.1

>+  get activeTabGroup() {
>+    var activeTabGroup = this.getGroups({filter: "active"})[0];
>+    return activeTabGroup;
>+  },

Please rename it to activeGroup to give lesser confusion with tabs in groups. Also you can directly return the value from this.getGroups.

Comment 17

[u279076]

Attachment: Patch v1.2

Comment 18

[u279076]

Attachment: Patch v1.3 [checked-in]

Moved into Groups section as per IRC discussion.

Comment 19

[u279076]

Comment on attachment 516624 [details] [diff] [review]
Patch v1.3 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/11d6d6d98712 [default]