475203 - Need regression tests for tabstrip scrolling

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Nochum Sossonko [:Natch]]

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.

Comment 1

[Nochum Sossonko [:Natch]]

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.

Comment 2

[Nochum Sossonko [:Natch]]

So that wasn't entirely true as you can set smoothScroll to false. I'll post a patch here hopefully tomorrow with some tests.

Comment 3

[Nochum Sossonko [:Natch]]

Attachment: some tests

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.

Comment 4

[Dão Gottwald [:dao]]

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 5

[Nochum Sossonko [:Natch]]

Comment on attachment 359216 [details] [diff] [review]
some tests

Yes, I probably should. I'll also try testing _distanceScroll.

Comment 6

[Nochum Sossonko [:Natch]]

Attachment: tests

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.

Comment 7

[Dão Gottwald [:dao]]

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?

Comment 8

[Nochum Sossonko [:Natch]]

(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.

Comment 9

[Nochum Sossonko [:Natch]]

> >+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.

Comment 10

[Nochum Sossonko [:Natch]]

Attachment: Updated to comments

Ok, I hope this is fine.

Comment 11

[Dão Gottwald [: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.

Comment 12

[Nochum Sossonko [:Natch]]

>+  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

Comment 13

[Dão Gottwald [:dao]]

(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.

Comment 14

[Nochum Sossonko [:Natch]]

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?

Comment 15

[Dão Gottwald [:dao]]

The tab itself doesn't load, and the tabpanel seems irrelevant.

Comment 16

[Nochum Sossonko [:Natch]]

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.

Comment 17

[Nochum Sossonko [:Natch]]

Attachment: tests ver. 2

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

Comment 18

[Dão Gottwald [: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.

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: Nits Addressed

> 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.

Comment 20

[Nochum Sossonko [:Natch]]

Attachment: clickCount fix

C&P error, fix the clickCount.

Comment 21

[Dão Gottwald [: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".

Comment 22

[Nochum Sossonko [:Natch]]

Comment on attachment 362506 [details] [diff] [review]
clickCount fix

Ok, gonna fix those and also add a test for bug 475073.

Comment 23

[Nochum Sossonko [:Natch]]

Attachment: patch

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.

Comment 24

[Dão Gottwald [: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.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 366670 [details] [diff] [review]
patch

tabStrip.smoothScroll should be set back to the original value before finishing

Comment 26

[Nochum Sossonko [:Natch]]

(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.

Comment 27

[Dão Gottwald [:dao]]

(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.

Comment 28

[Nochum Sossonko [:Natch]]

Attachment: patch v.3

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.

Comment 29

[Nochum Sossonko [:Natch]]

Attachment: better, v.3

This uses the _scrollbox (which should avoid margin or other spacing problems).

Comment 30

[Dão Gottwald [:dao]]

I haven't looked at v.3 yet, but... Which margin problems other than bug 482965?

Comment 31

[Nochum Sossonko [:Natch]]

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...

Comment 32

[Dão Gottwald [:dao]]

Failing for things like bug 482965 is actually desirable.

Comment 33

[Nochum Sossonko [:Natch]]

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.

Comment 34

[Dão Gottwald [: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.

Comment 35

[Nochum Sossonko [:Natch]]

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.

Comment 36

[Nochum Sossonko [:Natch]]

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.

Comment 37

[Nochum Sossonko [:Natch]]

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).

Comment 38

[Dão Gottwald [: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.

Comment 39

[Nochum Sossonko [:Natch]]

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?

Comment 40

[Dão Gottwald [: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.

Comment 41

[Nochum Sossonko [:Natch]]

Attachment: test with border check

Throwing this up to see if this works/is acceptable.

Comment 42

[Dão Gottwald [:dao]]

Still the same unexpected pass on linux.

Comment 43

[Nochum Sossonko [:Natch]]

Attachment: test with border check 2

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.

Comment 44

[Dão Gottwald [:dao]]

Attachment: added a few more tests and fixed the existing ones

this passes with the patches in bug 483552

Comment 45

[Nochum Sossonko [:Natch]]

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.

Comment 46

[Nochum Sossonko [:Natch]]

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...)

Comment 47

[Dão Gottwald [:dao]]

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.

Comment 48

[Nochum Sossonko [:Natch]]

(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

Comment 49

[Dão Gottwald [:dao]]

Yes, that's bug 485938.

Comment 50

[Dão Gottwald [:dao]]

Can this land? Do you want to change or add something?

Comment 51

[Nochum Sossonko [:Natch]]

This is fine. Don't we have to wait for bug 483552 though? (That's the reason I hadn't added checkin-needed...)

Comment 52

[Dão Gottwald [:dao]]

The toolkit part has landed, which should be enough to make these tests pass.

Comment 53

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3fb51dc11e1e

Comment 54

[Nochum Sossonko [:Natch]]

(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) :)

Comment 55

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

Will this tests also go into 1.9.1? Can we set in-testsuite+ now?

Comment 56

[Nochum Sossonko [:Natch]]

(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.

Comment 57

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

Tests have been checked in a long time ago. Everything looks fine here. Marking as verified.