650694 - browser_scrollbar.js on Android fails four tests and times out

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Phil Ringnalda (:philor)]

The way it goes,

TEST-INFO | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | Testing visibility of scrollbars
TEST-PASS | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The horizontal scrollbar should be hidden
TEST-PASS | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The vertical scrollbar should be hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The horizontal scrollbar should be visible - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The vertical scrollbar should be visible - Got false, expected true
TEST-KNOWN-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | Don't cause the height to grow beyond the window height if it doesn't need to
TEST-PASS | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The horizontal scrollbar should be hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The vertical scrollbar should be visible - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The horizontal scrollbar should be visible - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | The vertical scrollbar should be visible - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | Test timed out
INFO TEST-END | chrome://mochitests/content/browser/mobile/chrome/browser_scrollbar.js | finished in 30038ms

I must be missing several things, because from that output, we load a nearly-blank page and correctly don't have either scrollbar, the two passes, then we fail to have both when we expect to have them (though I don't get why we expect both of them when we loaded a 200px high x 2000px wide document), then we confusingly put the todo about how we didn't really want to expect them both after that instead of before, then we load a 2000px high x 200px wide document, correctly don't have a horizontal scrollbar but incorrectly don't have a vertical one, then we load a 2000px x 2000px document and don't have either one, then we add a PanFinished event listener, which never fires, so we time out.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I don't think there is a need to alter the viewport height when it is already >= to the window size

Comment 2

[Phil Ringnalda (:philor)]

The test failures were fixed by bug 650582, so the bug in the test is apparently that it doesn't make sure that the browser is in the state it expects, whether or not some previous test has failed, and it was checking for scrollbars in an accidentally-left-open preferences window instead of the test's browser window.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #2)
> The test failures were fixed by bug 650582, so the bug in the test is
> apparently that it doesn't make sure that the browser is in the state it
> expects, whether or not some previous test has failed, and it was checking for
> scrollbars in an accidentally-left-open preferences window instead of the
> test's browser window.

Nice.
I still think it worth adding a fix on top of that because by running the test alone I see random orange on the horizontal scrollbars test.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 527018 [details] [diff] [review]
Patch

(In reply to comment #1)
> I don't think there is a need to alter the viewport height when it is already
> >= to the window size

You still need to increase viewportH if it's smaller than the window size *when zoomed out* -- otherwise you will see checkerboards at the bottom of the window when the page first loads or is zoomed out.  We need (viewportH * getPageZoomLevel) >= windowH.

(Sorry, I have a patch in progress to add a browser-chrome test for this case, but it stalled because I was running into some weird bug causing it to fail.)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Add an additional check as suggested and use clampZoomLevel to ensure a zoom level is not used ignoring the metadata of the page.

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 529961 [details] [diff] [review]
Patch v0.2

>+    if (viewportH < screenH || viewportH * this.clampZoomLevel(this.getPageZoomLevel()) < screenH)
>+      viewportH = Math.max(viewportH, screenH * (browser.contentDocumentWidth / screenW));

I think you can remove the part before the "||" in the if statement.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/807ea3a5844f

Comment 8

[Andreea Pod]

Verified fixed according to Tinderboxpushlog.