Closed Bug 650694 Opened 9 years ago Closed 9 years ago

browser_scrollbar.js on Android fails four tests and times out

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: philor, Assigned: vingtetun)

References

Details

(Whiteboard: [mobile_unittests] [mobile_dev_needed])

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [mobile_unittests]
Whiteboard: [mobile_unittests] → [mobile_unittests] [mobile_dev_needed]
Attached patch Patch (obsolete) — Splinter Review
I don't think there is a need to alter the viewport height when it is already >= to the window size
Attachment #527018 - Flags: review?(mbrubeck)
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.
(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 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.)
Attachment #527018 - Flags: review?(mbrubeck) → review-
Attached patch Patch v0.2Splinter Review
Add an additional check as suggested and use clampZoomLevel to ensure a zoom level is not used ignoring the metadata of the page.
Assignee: nobody → 21
Attachment #527018 - Attachment is obsolete: true
Attachment #529961 - Flags: review?(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.
Attachment #529961 - Flags: review?(mbrubeck) → review+
http://hg.mozilla.org/mozilla-central/rev/807ea3a5844f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified fixed according to Tinderboxpushlog.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.