Need regression tests for tabstrip scrolling

VERIFIED FIXED in Firefox 3.6a1

Status

()

defect
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Natch, Assigned: Natch)

Tracking

Trunk
Firefox 3.6a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

With the landing of bug 457651, some regressions spawned that would have been nice to detect with automated tests, I'll attempt to create some now.
Testing the scrolling is a bit complicated due to the fact that it's animated and runs overtime, not instantaneously. I've been cooking up a patch that involves timeouts, which I know are frowned upon, but in this case it's the only real option. I've set it up in a way that's guaranteed to yield the correct result, though (I think). Should I just scratch the scrolling test and just write up a test for other random stuff on the tabbar (not even sure what that is yet)? Any hints as to what can be done about this? Alternatively this bug can be morphed into testing the scrollbox implementation, seeing how it handles various bindings with/without their own getScrollableElements. That doesn't help for the browser chrome test though.
Flags: in-testsuite?
So that wasn't entirely true as you can set smoothScroll to false. I'll post a patch here hopefully tomorrow with some tests.
Posted patch some tests (obsolete) — Splinter Review
Not so extensive really, because it's very hard to get reliable results with the rest of scrollbox's methods. It does test though (crudely) for bug 474964. Let me know if there's anything else you want me to test.
Assignee: nobody → highmind63
Attachment #359216 - Flags: review?(gavin.sharp)
Comment on attachment 359216 [details] [diff] [review]
some tests

>+                 browser_tabbrowser.js \

Should this be browser_tabscrolling.js instead?

>+  tabStrip.scrollBoxObject.ensureElementIsVisible(firstTab);

>+  tabStrip.scrollBoxObject.scrollByIndex(1);

This looks like it should be tabStrip.ensureElementIsVisible and tabStrip.scrollByIndex, assuming you want to test with scrollbox.xml's high-level code.
Comment on attachment 359216 [details] [diff] [review]
some tests

Yes, I probably should. I'll also try testing _distanceScroll.
Attachment #359216 - Attachment is obsolete: true
Attachment #359216 - Flags: review?(gavin.sharp)
Posted patch tests (obsolete) — Splinter Review
Ok, here is a much better version of the tests. I also fixed a mistake in the previous one which didn't set smoothScroll correctly.
Attachment #359412 - Flags: review?(dao)
Attachment #359412 - Flags: review?(dao) → review-
Comment on attachment 359412 [details] [diff] [review]
tests

>+const tabCountForOverflow = Math.floor(tabStripBox.width / 100);

You should read browser.tabs.tabMinWidth here.

>+function getBoxDimensions(aElement) {
>+  var boxObject = {};
>+  var rect = aElement.getBoundingClientRect();
>+  boxObject.width = rect.width;
>+  boxObject.left = rect.left;
>+  boxObject.right = rect.right;
>+  return boxObject;
>+}

Just use the rect object directly?

>+  is(rect.left + tabBorderLeft, upButtonBox.right, "Tabs should scroll to start");

What's tabBorderLeft doing in that calculation?

>+  var element = tabStrip._elementFromPoint(rect.left + 1);

Shouldn't this work without +1? If it doesn't, maybe that's a bug?
(In reply to comment #7)
> >+  is(rect.left + tabBorderLeft, upButtonBox.right, "Tabs should scroll to start");
> 
> What's tabBorderLeft doing in that calculation?

tabBorderLeft is the amount of border width of the tab that apparently doesn't get calculated by getBoundingClientRect. Maybe I'm wrong, but these tests fail otherwise. I've also confirmed that when I set selectedTab to the first tab it fails by 1, as opposed to 2 when it is not selected.
> >+  var element = tabStrip._elementFromPoint(rect.left + 1);
> 
> Shouldn't this work without +1? If it doesn't, maybe that's a bug?

Yes, I'll fix that with the next patch.
> >+const tabCountForOverflow = Math.floor(tabStripBox.width / 100);
>
> You should read browser.tabs.tabMinWidth here.

Well, the profile should be a clean one, and the pref by default is set to 100, but I'll just check mTabMinWidth and take that number.
Posted patch Updated to comments (obsolete) — Splinter Review
Ok, I hope this is fine.
Attachment #359412 - Attachment is obsolete: true
Attachment #359432 - Flags: review?(dao)
Comment on attachment 359432 [details] [diff] [review]
Updated to comments

I suspect this passes by chance on your system because adding the left tab border happens to fix the calculation. However, getBoundingClientRect includes the border, so in general this doesn't make sense. Off-hand I can't tell you what's missing, but a tab border seems highly unlikely.

>+                 browser_tabbrowser.js \

This needs a name that indicates that this is about tabstrip scrolling rather than a full tabbrowser testsuite.

>+function getBoxDimensions(aElement) {
>+  return aElement.getBoundingClientRect();
>+}

At this point, you can probably get rid of that function.

>+  tabContainer.lastChild.addEventListener("load", runOverflowTests, false);

This seems a little bit strange. What exactly are you waiting for? The tab element itself doesn't load.
Attachment #359432 - Flags: review?(dao) → review-
>+  tabContainer.lastChild.addEventListener("load", runOverflowTests, false);

> This seems a little bit strange. What exactly are you waiting for? The tab
> element itself doesn't load.

The tab sends a load event when it's finished loading (about:blank I presume), it was necessary for the test to get it working at all.

I think this will have to be WONTFIX as the scrolling, even with smoothScroll set to false, seems to be running on a timer, so there's no real effective way of testing it. Note, I've already tried setting the scrollIncrement pref to no avail, although that pretty much makes sense...

_elementFromPoint is testable though, and scrollByIndex was very reliable for me, exactly why I'm not sure. The +2 _always_ worked for me on _distanceScroll and ensureElementIsVisible, interestingly enough. I'll be happy to retake this if someone can explain to me what's going on here, the downButton is always reliable, whereas the upButton is always 2 pixels off.

-> default assignee
Assignee: highmind63 → nobody
(In reply to comment #12)
> >+  tabContainer.lastChild.addEventListener("load", runOverflowTests, false);
> 
> > This seems a little bit strange. What exactly are you waiting for? The tab
> > element itself doesn't load.
> 
> The tab sends a load event when it's finished loading (about:blank I presume),
> it was necessary for the test to get it working at all.

That's probably the favicon loading.

> I think this will have to be WONTFIX as the scrolling, even with smoothScroll
> set to false, seems to be running on a timer

One flaw could be that you're adding the tabs before disabling smoothScroll, which means that the scrollbox could still be scrolling the last tab into view while you're doing other things.
Ok, that's a good point, although the first check was actually working. I can imagine, though, that the timers haven't fully finished whatever they started and may be negatively affecting the test, I'll try this later today. For the load thing, I'll check again if I need it (it made the test more reliable iirc), is there a better way to check if the tab/tabpanel have finished loading?
The tab itself doesn't load, and the tabpanel seems irrelevant.
Right, but it was effectively giving me time after opening all the tabs, I may not need it though, if I make the change to smoothScroll a bit earlier. The loading that I'm talking about is like some sort of notification that the tab is finished (loading about:blank, the relevant observers are called, etc.), may just be my mistake, which incidentally gave me some wrong (passing) results.
Posted patch tests ver. 2 (obsolete) — Splinter Review
Ok, I realized what I was doing wrong :)

1) When the tabstrip goes into overflow tabbrowser.xml does some space correction, so I added a listener for overflow which should get called last, after all the corrections have been made.

2) _startScroll and _stopScroll are called when you click on one of the arrow buttons, so I simulated that better.

All the tests pass now, and I changed the test name to be more accurate.

->re-taking
Assignee: nobody → highmind63
Attachment #359432 - Attachment is obsolete: true
Attachment #359963 - Flags: review?(dao)
Comment on attachment 359963 [details] [diff] [review]
tests ver. 2

>+const tabContainer = gBrowser.tabContainer;
>+const tabStrip = tabContainer.mTabstrip;
>+const tabStripBox = tabStrip.getBoundingClientRect();
>+const tabCountForOverflow = Math.floor(tabStripBox.width / tabContainer.mTabMinWidth);

It would be nice to get rid of these global consts. You can use gBrowser.mTabs instead of tabContainer.childNodes, and otherwise just use gBrowser.tabContainer... directly or define a local var. tabStripBox and tabCountForOverflow are used only once.

>+function runOverflowTests() {
>+
>+  tabStrip.removeEventListener("overflow", runOverflowTests, false);

You should check the overflow orientation (event.detail) and return early if it's not 1.

>+  tabStrip.ensureElementIsVisible(firstTab);
>+  tabStrip.scrollByIndex(1);
>+  tabStrip._startScroll(-1);
>+  tabStrip._stopScroll();
>+  tabStrip._distanceScroll({detail: 3, originalTarget: upButton});

Instead of calling these methods, it would be better to simulate a single/double/triple click on the buttons.

>+function addTabs() {

>+function endTests() {

These functions are used only once -- please move the code there.
Posted patch Nits Addressed (obsolete) — Splinter Review
> It would be nice to get rid of these global consts. You can use gBrowser.mTabs
> instead of tabContainer.childNodes, and otherwise just use
> gBrowser.tabContainer... directly or define a local var. tabStripBox and
> tabCountForOverflow are used only once.

I got rid of those two but left tabStrip and tabContainer as I need to use them in different places and it was convenient. This is just a test though so I hope it's not all that bad.
Attachment #359963 - Attachment is obsolete: true
Attachment #362505 - Flags: review?(dao)
Attachment #359963 - Flags: review?(dao)
Posted patch clickCount fix (obsolete) — Splinter Review
C&P error, fix the clickCount.
Attachment #362505 - Attachment is obsolete: true
Attachment #362506 - Flags: review?(dao)
Attachment #362505 - Flags: review?(dao)
Comment on attachment 362506 [details] [diff] [review]
clickCount fix

>+  var upButtonBox = upButton.getBoundingClientRect();
>+  var downButtonBox = downButton.getBoundingClientRect();

nit: upButtonRect and downButtonRect

>+  EventUtils.synthesizeMouse(upButton, 0, 0, {clickCount: 1});
>+  EventUtils.synthesizeMouse(upButton, 0, 0, {clickCount: 2});
>+  EventUtils.synthesizeMouse(upButton, 0, 0, {clickCount: 3, detail: 3, originalTarget: upButton});

Why three different mouse events without any check in between? Looks like two could be broken and the test would pass.

>+  var tabCountForOverflow = Math.floor(tabStrip.getBoundingClientRect().width / tabContainer.mTabMinWidth);

Reading the pref instead of mTabMinWidth would be better, as it would test the official "API".
Summary: Need regression tests for tabbrowser. → Need regression tests for tabstrip scrolling
Comment on attachment 362506 [details] [diff] [review]
clickCount fix

Ok, gonna fix those and also add a test for bug 475073.
Attachment #362506 - Attachment is obsolete: true
Attachment #362506 - Flags: review?(dao)
OS: Windows Vista → All
Hardware: x86 → All
Posted patch patch (obsolete) — Splinter Review
Ok, here's a patch with the comments addressed, I didn't include the other tests as it doesn't seem appropriate here, I'll try to attach a test for the other bug (new tab button) to one of the toolbar customization tests.
Attachment #366670 - Flags: review?(dao)
Whiteboard: [needs review dao]
I landed this and backed it out, as the second test failed on Mac:

> chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Tab should scroll into view
> TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Tab should scroll into view - Got 929, expected 930
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | One tab should have been scrolled
> TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Tabs should scroll to start
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | _elementFromPoint should recognize the tabs

I'm pretty sure this is because pinstripe adds -moz-margin-end: 1px to the tabs.
Whiteboard: [needs review dao]
Comment on attachment 366670 [details] [diff] [review]
patch

tabStrip.smoothScroll should be set back to the original value before finishing
Attachment #366670 - Flags: review?(dao) → review+
(In reply to comment #24)
> I'm pretty sure this is because pinstripe adds -moz-margin-end: 1px to the
> tabs.

Hmm, I should probably take that test out, now that I'm testing the clicks which are more reliable... I'll post a new patch, and hopefully get the double-clicking tested as well.
Status: NEW → ASSIGNED
Depends on: 482965
(In reply to comment #26)
> (In reply to comment #24)
> > I'm pretty sure this is because pinstripe adds -moz-margin-end: 1px to the
> > tabs.
> 
> Hmm, I should probably take that test out, now that I'm testing the clicks
> which are more reliable...

There's nothing wrong with calling scrollByIndex(1). A click will give you the same result, due to bug 482965.
Posted patch patch v.3 (obsolete) — Splinter Review
This patch removes all direct method calls to tabStrip and instead tests the ui directly through mouse clicks and scrolls. The todo on the mouse scroll is because it fails by 2 pixels for some reason (and I've verified that it's a failure by calling scrollByIndex directly). Let me know if this is better.
Attachment #366670 - Attachment is obsolete: true
Attachment #367441 - Flags: review?(dao)
Posted patch better, v.3 (obsolete) — Splinter Review
This uses the _scrollbox (which should avoid margin or other spacing problems).
Attachment #367441 - Attachment is obsolete: true
Attachment #367447 - Flags: review?(dao)
Attachment #367441 - Flags: review?(dao)
I haven't looked at v.3 yet, but... Which margin problems other than bug 482965?
None that I know of, I just did it this way because of that bug, and this is really more correct. Now the only way this will fail is if it doesn't work (or if some padding is added to the tabstrip, which i don't think will happen). Alternatively, I could test the position of the tab-to-be-moved before the mouse interaction and then test it after, that seems a bit overkill though...
Failing for things like bug 482965 is actually desirable.
Attachment #367447 - Attachment is obsolete: true
Attachment #367447 - Flags: review?(dao)
Comment on attachment 367441 [details] [diff] [review]
patch v.3

In that case, this is probably the one you want. This assumes that the arrows will always be flush perfectly with the first/last tab.
Attachment #367441 - Attachment is obsolete: false
Attachment #367441 - Flags: review?(dao)
Yep, that seems like a good assumption. That said, it's possible that it was failing for other reasons with pinstripe... I can't really tell.
Problem is, I don't really have any other platforms to test this, currently these tests pass (besides the todo_is for DOMMouseScroll) on Vista.
fwiw, I've just verified that (at least when smoothScroll is set to false) when using the mousewheel the tab is off by a couple of pixels, seems like a bug to me, and why this test is todo...

Bug 483552.
Depends on: 483552
Dao, bug 483552 doesn't have much of a chance of getting fixed anytime soon, could we just land this as-is and enable the test once that bug gets fixed (unless you want me to calculate the border and add that value to the calculation, that would mean it would fail once the behavior is correct).
Whiteboard: [needs review dao]
I get this, running the test on linux:

> TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | One tab should have been scrolled - Got -82, expected 16
> TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | One page of tabs should have been scrolled - Got -82, expected 16
> TEST-UNEXPECTED-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | One tab should have been scrolled - Got 986, expected 986

The unexpected pass also happens on the tryserver, on linux.
Hrm, the unexpected pass is a bit tricky, I figured there might be a problem with other themes that don't have the extra margin (or maybe ensureElementIsVisible works right on Linux?). Did the tryserver also have the unexpected-fails? If so, any idea what's going wrong?
Whiteboard: [needs review dao]
Nope, the fails didn't happen on the tryserver.

btw, you can use gPrefService in the test. And smoothScroll shouldn't just be set to true at the end, but to the original value, whatever that was.
Posted patch test with border check (obsolete) — Splinter Review
Throwing this up to see if this works/is acceptable.
Attachment #367441 - Attachment is obsolete: true
Attachment #367441 - Flags: review?(dao)
Attachment #368808 - Attachment description: test with margin check → test with border check
Still the same unexpected pass on linux.
Depends on: 485938
Posted patch test with border check 2 (obsolete) — Splinter Review
I wasn't checking the correct border in the previous test, I verified that with removing the border, this test now passes (instead of KNOWN-FAIL) on windows, can you give it a try on linux? Also, I put some extra checking code in the test output for the KNOWN-FAIL case, can you paste it here if it's still UNEXPECTED-PASS. Thanks.
Attachment #368808 - Attachment is obsolete: true
this passes with the patches in bug 483552
Attachment #370431 - Flags: review?(highmind63)
Comment on attachment 370431 [details] [diff] [review]
added a few more tests and fixed the existing ones

Looks good to me fwiw. The only test that seemed to fail strangely was:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one tab to the right with a single click - Got 918, expected 924

I suspect that's due to bug 485938?

+  tabstrip.addEventListener("overflow", runOverflowTests, false);

Why false? IIRC listeners should be registered as capturing in tests as it speeds them up. Could be wrong though.
Attachment #370431 - Flags: review?(highmind63) → review+
Oh, and you might want to set gBrowser.selectedTab to the first tab at the end, so that you're not changing the selection on each removal, see bug 476928 comment 24. (Not a big deal though...)
Assignee: highmind63 → dao
Status: ASSIGNED → NEW
I don't want to take this, I mostly just reformatted your tests.

(In reply to comment #45)
> (From update of attachment 370431 [details] [diff] [review])
> Looks good to me fwiw. The only test that seemed to fail strangely was:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js
> | Scrolled one tab to the right with a single click - Got 918, expected 924

On Windows?

> +  tabstrip.addEventListener("overflow", runOverflowTests, false);
> 
> Why false? IIRC listeners should be registered as capturing in tests as it
> speeds them up. Could be wrong though.

Because the code we're testing has overflow listeners, and we don't want to run before them.
Assignee: dao → highmind63
(In reply to comment #47)
> On Windows?

Yes. I hadn't imported the other patches from bug 483552 or bug 485938 though.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090331 Minefield/3.6a1pre
Yes, that's bug 485938.
Attachment #370288 - Attachment is obsolete: true
Can this land? Do you want to change or add something?
This is fine. Don't we have to wait for bug 483552 though? (That's the reason I hadn't added checkin-needed...)
The toolkit part has landed, which should be enough to make these tests pass.
http://hg.mozilla.org/mozilla-central/rev/3fb51dc11e1e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #53)
> http://hg.mozilla.org/mozilla-central/rev/3fb51dc11e1e

wOOt! Thanks (this must be the longest attempt at landing a test) :)
Will this tests also go into 1.9.1? Can we set in-testsuite+ now?
Target Milestone: --- → Firefox 3.6a1
(In reply to comment #55)
> Will this tests also go into 1.9.1? Can we set in-testsuite+ now?

The tests cannot land there as they will fail, see the dependent bugs. I've set in-testsuite.
Flags: in-testsuite? → in-testsuite+
Tests have been checked in a long time ago. Everything looks fine here. Marking as verified.
Status: RESOLVED → VERIFIED
Blocks: 575748
Depends on: 582216
You need to log in before you can comment on or make changes to this bug.