Closed
Bug 635895
Opened 13 years ago
Closed 13 years ago
Test failure in testBackgroundTabScrolling.js | Error: Link 1 title is visible for the tab
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aaronmt, Assigned: aaronmt)
References
()
Details
(Keywords: regression, Whiteboard: [mozmill-test-failure])
Attachments
(1 file, 2 obsolete files)
2.16 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•13 years ago
|
||
Change to the browser in bug 625320 http://hg.mozilla.org/mozilla-central/rev/3017b3459639
Blocks: 625320
Assignee | ||
Comment 2•13 years ago
|
||
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
Attachment #514214 -
Flags: review?(hskupin)
Comment 3•13 years ago
|
||
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.
Attachment #514214 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 4•13 years ago
|
||
Refactor from comment #3
Attachment #514371 -
Flags: review?(hskupin)
Updated•13 years ago
|
Attachment #514214 -
Attachment is obsolete: true
Comment 5•13 years ago
|
||
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.
Attachment #514371 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
(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;
Assignee | ||
Comment 8•13 years ago
|
||
Ok, thanks for the example.
Attachment #514371 -
Attachment is obsolete: true
Attachment #514781 -
Flags: review?(hskupin)
Comment 9•13 years ago
|
||
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.
Attachment #514781 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Landed in default as: http://hg.mozilla.org/qa/mozmill-tests/rev/6a43afcb74e3
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•