635895 - Test failure in testBackgroundTabScrolling.js | Error: Link 1 title is visible for the tab

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

Module: testBackgroundTabScrolling.js
Test: testScrollBackgroundTabIntoView
Error: Link 1 title is visible for the tab
Branch: 4.0 - default
Platform: All

http://hg.mozilla.org/qa/mozmill-tests/file/ba578a76d549/firefox/testTabbedBrowsing/testBackgroundTabScrolling.js#l130

http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=4.0&platform=All&test=firefox%2FtestTabbedBrowsing%2FtestBackgroundTabScrolling.js&func=testScrollBackgroundTabIntoView

Recent report: http://hg.mozilla.org/qa/mozmill-tests/file/ba578a76d549/firefox/testTabbedBrowsing/testBackgroundTabScrolling.js#l130

Comment 1

[Aaron Train [:aaronmt]]

Change to the browser in bug 625320
http://hg.mozilla.org/mozilla-central/rev/3017b3459639

Comment 2

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Index adjustments due to two new additions into the popup menu: Tab Groups and a menu separator. When lastIndex is used, this patch advances the lastIndex by '2', and readjusts the iterator condition

Comment 3

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

Comment on attachment 514214 [details] [diff] [review]
Patch v1 - (default)

>   // Check that the correct title is shown for all tabs except the last one
>-  for (var i = 1; i < lastIndex; i++) {
>+  // Note: lastIndex + 2 due to Tab Groups and menu seperator additions
>+  for (var i = 3; i < lastIndex + 2; i++) {

Re-using lastIndex here causes a lot of confusion. I would suggest we rename it to lastTabIndex and create a new variable for the number of entries in the all tabs menu list.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Refactor from comment #3

Comment 5

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

Comment on attachment 514371 [details] [diff] [review]
Patch v2 - (default)

>+  // Number of all items in the 'List all Tabs' menu
>+  var numOfAllTabsItems = allTabsPopup.getNode().childNodes.length; 

For consistence please also use the last index and not the overall number of entries. Also a name like lastMenuItemIndex would be better to understand I think.

Comment 6

[Aaron Train [:aaronmt]]

(In reply to comment #5)
> Comment on attachment 514371 [details] [diff] [review]
> Patch v2 - (default)
> 
> >+  // Number of all items in the 'List all Tabs' menu
> >+  var numOfAllTabsItems = allTabsPopup.getNode().childNodes.length; 
> 
> For consistence please also use the last index and not the overall number of
> entries. Also a name like lastMenuItemIndex would be better to understand I
> think.

Not sure what you're asking here. We need to get a list of all items (which include: 'Tab Groups', and the menu separator), followed by the list of tabs. The last tab index will be equal to the number of elements returned from the childNodes NodeList array.

Comment 7

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

(In reply to comment #6)
> The last tab index will be equal to the number of elements returned from the
> childNodes NodeList array.

That's not true. There is a difference of 2. What I want to see is:

var lastMenuItemIndex = allTabsPopup.getNode().childNodes.length - 1;

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Ok, thanks for the example.

Comment 9

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

Comment on attachment 514781 [details] [diff] [review]
Patch v3 - (default)

That's a lot cleaner and better to understand. Thanks. Lets get this in on default for todays testrun.

Comment 10

[Aaron Train [:aaronmt]]

Landed in default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/6a43afcb74e3