728893 - Allow mochitest iframe to go fullscreen

Status: RESOLVED WONTFIX

(Testing :: Mochitest, defect)

Description

[Diogo Golovanevsky Monteiro [:diogogmt]]

The majority of mochitests for Bug 633602 need to go fullscreen.
However, the iframe that runs the tests is not allowed to go fullscreen.
For now all the tests are being run inside a harness that opens another window.
What are the drawbacks of allowing the mochitest iframe to go fullscreen?

Comment 1

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: Add mozallowfullscreen to mochitest iframe

With this change the mochitest iframe runs all the tests that require fullscreen mode.
Not sure if there is a specific reason not to allow the iframe to go fullscreen.

Comment 2

[(not currently active) Ted Mielczarek]

The Mochitest harness predates the fullscreen APIs by several years, so I'd imagine it's just that nobody had yet figured this was important.

Comment 3

[cmtalbert]

Comment on attachment 598888 [details] [diff] [review]
Add mozallowfullscreen to mochitest iframe

I think this is fine. That said, I would *definitely* run this through try server because the entire mochitest family is very fragile when it comes to focus and if your test leaves anything in full-screen or doesn't reset the focus properly when you switch back, then you'll probably break all the tests that follow you.

Comment 4

[cmtalbert]

Submitted to try:
https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 5

[Mozilla RelEng Bot]

Try run for 4039e09e0e15 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4039e09e0e15
Results (out of 120 total builds):
    exception: 1
    success: 109
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4039e09e0e15

Comment 6

[cmtalbert]

Diogo, the try run looks good. I suppose adding the attribute really won't break anything. The interesting bit will be when you add your tests to make use of the addon. :)

Happy coding.

Comment 7

[Diogo Golovanevsky Monteiro [:diogogmt]]

(In reply to Clint Talbert ( :ctalbert ) from comment #6)
> Diogo, the try run looks good. I suppose adding the attribute really won't
> break anything. The interesting bit will be when you add your tests to make
> use of the addon. :)
> 
> Happy coding.

Thanks a lot Clint :)
We're making sure to cancel fullscreen before finishing all the tests, but to prevent crashes in the future we could add a check on SimpleTest.finish to cancel fullscreen if for some reason the test fails to cancel it.

I remember seeing that if SimpleTest.finish isn't called and the test hangs for more than 5 mins, SimpleTest.finish will run by default(not sure) so the execution can keep going.

So if one of the tests sets the iframe to fullscreen and doesn't cancel it or fails to finish, when SimpleTest.finish gets called it would reset the fullscreen mode and keep running the other tests.

Comment 8

[cmtalbert]

(In reply to Diogo Golovanevsky Monteiro from comment #7)
> (In reply to Clint Talbert ( :ctalbert ) from comment #6)
> > Diogo, the try run looks good. I suppose adding the attribute really won't
> > break anything. The interesting bit will be when you add your tests to make
> > use of the addon. :)
> > 
> > Happy coding.
> 
> Thanks a lot Clint :)
> We're making sure to cancel fullscreen before finishing all the tests, but
> to prevent crashes in the future we could add a check on SimpleTest.finish
> to cancel fullscreen if for some reason the test fails to cancel it.
> 
That makes a lot of sense to me. Do you want to do that as part of this patch when you add your full screen test to it?

Comment 9

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: v2

When SimpleTest.finish() is called and an element is still in fullscreen mode, fullscreen gets cancelled so the next test can have a fresh start.

Comment 10

[cmtalbert]

Comment on attachment 601652 [details] [diff] [review]
v2

This looks great, but it needs a test. Can you add a simple sanity test for this behavior?  You can see one of the other SimpleTest sanity tests here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_sanity.html?force=1

I would just write a new html file in that directory that ensures that the full screen is canceled if a test calls finish.

Thanks, Clint

Comment 11

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: fix with tests

Mochitest iframe is able to go fullscreen and SimpleTest.finish make sure fullscreen mode is cancelled after every test is finished

Sanity test checks if SimpleTest.finish is cancelling fullscreen.
The test only works when running the sanity harness, as a stand alone test it won't work since the assertion is being made after SimpleTest.finish is called.

Thanks for the help Clint :)

Comment 12

[David Humphrey (:humph)]

Clint, we're getting pretty close on bug 633602, which needs this.  Any chance of another review soon?

Comment 13

[cmtalbert]

Comment on attachment 604600 [details] [diff] [review]
fix with tests

Hey Diogo, 
Sorry it took so long to get to this review. Been consumed with end-of-quarter/beginning-of-new-quarter stuff. Thanks for the patch and the test.

Comment 14

[cmtalbert]

Did this never land?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

It did not. Diogo - please don't resolve bugs unless their patch has actually landed. Otherwise, you're begging for it to slip through the cracks like this one did.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09da30d622c0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, I had to disable the new test due to it making the tree orange for not actually checking anything. Please fix up the test and post a follow-up patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85eb1308f976

35 INFO TEST-START | /tests/Harness_sanity/test_cancelFullScreen.html
36 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_cancelFullScreen.html | [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once.  Make sure you use SimpleTest.waitForExplicitFinish() if you need it.)
37 INFO TEST-PASS | /tests/Harness_sanity/test_cancelFullScreen.html | SimpleTest.finish should cancel fullscreen mode - false should equal false
38 INFO TEST-END | /tests/Harness_sanity/test_cancelFullScreen.html | finished in 114ms

Comment 17

[Ed Morley [:emorley]]

Ryan, sorry I've backed this out for now, since I'm uncomfortable with just disabling the test and leaving the rest in, especially without review and/or commenting the test out (rather than removing it's entry entirely) so it doesn't get forgotten.

Diogo, once the test is fixed up, we'll get this landed for you as soon as possible. Just ask if you have any questions about how to do so, either here or on IRC in #ateam :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f6c4236bbb

Comment 18

[Diogo Golovanevsky Monteiro [:diogogmt]]

(In reply to Ed Morley [:edmorley] from comment #17)
> Ryan, sorry I've backed this out for now, since I'm uncomfortable with just
> disabling the test and leaving the rest in, especially without review and/or
> commenting the test out (rather than removing it's entry entirely) so it
> doesn't get forgotten.
> 
> Diogo, once the test is fixed up, we'll get this landed for you as soon as
> possible. Just ask if you have any questions about how to do so, either here
> or on IRC in #ateam :-)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f6c4236bbb

Sorry for the trouble of setting the flags on the bug wrongly.

The mochitest should check if the fullscreen is being canceled after SimpleTest.finish is called, that's why the only check in the test happens after SimpleTest.finish is called:

37 INFO TEST-PASS | /tests/Harness_sanity/test_cancelFullScreen.html | SimpleTest.finish should cancel fullscreen mode - false should equal false

I'll add a dummy check before calling SimpleTest.finish to fix the first fail.

Again, sorry for the trouble.

Comment 19

[Ed Morley [:emorley]]

(In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #18)
> Again, sorry for the trouble.

It's no trouble at all - thank you for the patch! :-)

Comment 20

[Diogo Golovanevsky Monteiro [:diogogmt]]

Attachment: v4

I've added a dummy check before calling SimpleTest.finish in the cancelFullScreen sanity test.

That solved the problem of the test failing due to no check being performed.

Comment 21

[cmtalbert]

Comment on attachment 625825 [details] [diff] [review]
v4

looks good.

Comment 22

[cmtalbert]

Ok, this time went ahead and landed on inbound so we don't lose the patch again.

http://hg.mozilla.org/integration/mozilla-inbound/rev/cfac7b6a1f9c

Comment 23

[Ed Morley [:emorley]]

Really sorry, backed out of inbound for pretty frequent Linux failures in test_fullscreen-api.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=885dd2aa83ad&jobname=mochitests-1 (press the down arrow once to see the pushes between the landing and the backout)

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11982530&tree=Mozilla-Inbound

{
86846 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_fullscreen-api.html | Shouldn't be able to set window fullscreen from content - got true, expected false
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/885dd2aa83ad

Comment 24

[Ed Morley [:emorley]]

OS X 10.7 is unfortunately permaorange too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11983555&tree=Mozilla-Inbound

Comment 25

[cmtalbert]

From the logs it looked like the test switched into full screen mode and then never switched back out causing other tests to fail.

Comment 26

[Diogo Golovanevsky Monteiro [:diogogmt]]

(In reply to Clint Talbert ( :ctalbert ) from comment #25)
> From the logs it looked like the test switched into full screen mode and
> then never switched back out causing other tests to fail.

I'll see if I can recreate the failures on my machine.
Maybe SimpleTest.finish is not properly clearing the fullscreen state.


But are this failures related to fullscreen?
It looks like this tests are failing due to some discrepancy in the width/height of the elements.
86846 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_fullscreen-api.html | Shouldn't be able to set window fullscreen from content - got true, expected false
87734 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking doc width - got 1600, expected 400
87735 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking doc height - got 1168, expected 300
87736 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image width - got 800, expected 400
87737 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image height - got 600, expected 300
87746 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image width - got 800, expected 400
87747 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image height - got 600, expected 300
87753 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking scrollLeft - got 0, expected 400
87754 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | Checking scrollTop - got 0, expected 300
87756 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image width - got 800, expected 400
87757 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug369370.html | image height - got 600, expected 300

Comment 27

[Andrew Halberstadt [:ahal]]

Mass closing mochitest bugs that haven't had activity in the past 5 years. Please re-open or file a new bug with modern context if this is still relevant.