1096209 - Add automated tests for Firefox scrolling & zooming

Status: RESOLVED INVALID

(Testing :: Mochitest, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

https://moztrap.mozilla.org/manage/case/11497/

1 Load www.cnn.com and perform scrolling (up/down) using: mouse wheel, trackpad, keyboard. Then enter full screen (F11) and retry all the scrolling methods.
 > All actions perform fast and smooth, without any jerkiness or rendering issues
2Load www.bbc.com and perform zooming in/out on text/images using the CTRL ++/ CTRL -- shortcut
 > Zooming in/out is smooth, without any jerkiness or rendering issues
3 While the page from step 2 is zoomed in/out, perform some more scrolling on the page
 > The scrolling is smooth, without any jerkiness or rendering issues

The new test should be chrome, and should live under:
https://github.com/mozilla/gecko-dev/tree/master/browser/base/content/test/general



The key for entering fullscreen is: VK_F11

Comment 1

[Teodor Druta]

Attachment: browser_bug1096209.patch

I have attached a new patch with the test.
Drew, can you please make a review ?

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 8523917 [details] [diff] [review]
browser_bug1096209.patch

Review of attachment 8523917 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure this test is necessary.  We already have tests that cover zooming and scrolling.  I don't see why it's important to test those things in full-screen mode.  Should we test everything in full-screen mode?

And anyway, I tried running this under e10s and it fails (using mach's --e10s option).  That's to be expected because in a few places it accesses browser.contentWindow and contentDocument, which aren't available to chrome for remote browsers.  (See the definitions of those properties here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml?rev=36e3a60e3dc6#126)  It's also not possible to use EventUtils to synthesize wheel events from chrome in the content window.  You'd need to use a content script I think.

Comment 3

[Andreea Matei [:AndreeaMatei]]

We need this test as it's part of the regression and esr smoke testsuite for QA and we want to have as much coverage on automation as possible.

Comment 4

[Teodor Druta]

Drew, I've tried using window.fullScreen property in e10s fullscreen mode and it always returns false, same with document.mozFullScreen, can you please specify if this is the correct behaviour ? Or is there another method to find out if the browser window is in fullscreen mode ?

window.fullScreen works with e10s disabled.
document.mozFullScreen always returns false, with e10s disabled and with it enabled.

Comment 5

[Drew Willcoxon :adw]

(In reply to Andreea Matei [:AndreeaMatei] from comment #3)
> We need this test as it's part of the regression and esr smoke testsuite for
> QA and we want to have as much coverage on automation as possible.

This patch adds a test to the browser front-end, browser/base/content/test/general.  It's fine for you all to have whatever tests you think you need as part of your QA toolchain -- that's none of my business.  But I don't think we need this test in browser.

Comment 6

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I think this is a good test for us since it also covers the fullscreen bahaviour. The more coverage we get, the better. I would like to have this in.

Comment 7

[Teodor Druta]

(In reply to Teodor Druta from comment #4)
> Drew, I've tried using window.fullScreen property in e10s fullscreen mode
> and it always returns false, same with document.mozFullScreen, can you
> please specify if this is the correct behaviour ? Or is there another method
> to find out if the browser window is in fullscreen mode ?
> 
> window.fullScreen works with e10s disabled.
> document.mozFullScreen always returns false, with e10s disabled and with it
> enabled.

Gijs, can you please give us some info on this ?

Comment 8

[:Gijs (he/him)]

(In reply to Teodor Druta from comment #7)
> (In reply to Teodor Druta from comment #4)
> > Drew, I've tried using window.fullScreen property in e10s fullscreen mode
> > and it always returns false, same with document.mozFullScreen, can you
> > please specify if this is the correct behaviour ? Or is there another method
> > to find out if the browser window is in fullscreen mode ?
> > 
> > window.fullScreen works with e10s disabled.
> > document.mozFullScreen always returns false, with e10s disabled and with it
> > enabled.
> 
> Gijs, can you please give us some info on this ?

I can't reproduce this. window.fullScreen wfm, with and without e10s, on current Nightly.

I'm a little bit miffed that when Drew told you "I don't think this is a good idea", you went to ask someone else instead. It feels like "Mommy said no, so run and ask Daddy".

I agree with Drew that this test doesn't look like it is really helpful. The test also uses API calls to scroll the window in one go, which doesn't really verify what's in comment #0 as that means scrolling with mouse events.

In fact, none of the tests seem to really test for "smoothness" in any way. They just check for APIs being sync, which isn't actually helpful and doesn't "improve our test coverage", unless we have no DOM tests for this at all (unlikely - see http://mxr.mozilla.org/mozilla-central/search?string=scrollBy&find=test_&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central for some of them) in which case we should be adding mochitest-plain tests under dom/ somewhere, not cluttering up browser/base/content/test/general even more.

If we really think this smoothness checking is important, we should add a talos test instead of a browser-chrome test.

Who prioritized this to be converted to automated testing, and why?