Last Comment Bug 726943 - Fullscreen button in HTML video makes the browser go fullscreen but not the video
: Fullscreen button in HTML video makes the browser go fullscreen but not the v...
Status: VERIFIED FIXED
[qa!]
: regression, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Nickolay_Ponomarev
:
Mentors:
: 739358 740398 (view as bug list)
Depends on:
Blocks: 512529
  Show dependency treegraph
 
Reported: 2012-02-14 01:42 PST by Reuben Morais [:reuben]
Modified: 2012-05-17 01:54 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
Reduced testcase. (108 bytes, text/html)
2012-03-13 03:57 PDT, Reuben Morais [:reuben]
no flags Details
Video (302.23 KB, video/webm)
2012-03-14 18:06 PDT, Reuben Morais [:reuben]
no flags Details
annotated callstack demonstrating the problem (1.91 KB, text/plain)
2012-03-24 19:30 PDT, Nickolay_Ponomarev
no flags Details
don't fire extraneous NS_SIZEMODE events from windowDidResize (4.05 KB, patch)
2012-03-24 19:35 PDT, Nickolay_Ponomarev
mstange: review+
Details | Diff | Splinter Review
patch for beta (3.37 KB, patch)
2012-05-10 11:02 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Reuben Morais [:reuben] 2012-02-14 01:42:54 PST
OS X 10.7.3.

STR:
1. Open a video (https://www.youtube.com/watch?v=iRZ2Sh5-XuM)
2. Press fullscreen button

Actual results:
Browser goes fullscreen, video is still contained. If you press the fullscreen button again after that it works.

Expected results:
Video goes fullscreen, browser doesn't.
Comment 1 Reuben Morais [:reuben] 2012-02-14 08:10:51 PST

*** This bug has been marked as a duplicate of bug 694690 ***
Comment 2 Reuben Morais [:reuben] 2012-02-14 08:13:18 PST
Actually, this still happens here.
Comment 3 Paul Silaghi, QA [:pauly] 2012-03-12 06:41:43 PDT
The issue is reproducible on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120311 Firefox/13.0a1
Comment 4 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-12 15:24:37 PDT
WFM on Win7x64 and OSX10.7 20120312...
Comment 5 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-12 15:25:22 PDT
Paul, do you have hardware acceleration enabled on your 10.6 machine? I don't on my 10.7 machine...
Comment 6 Paul Silaghi, QA [:pauly] 2012-03-13 01:11:14 PDT
I can reproduce this both with hardware acceleration ON/OFF also on
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20120312 Firefox/13.0a1.
Chris, be sure not to forget to enable html5 before.
Comment 7 Reuben Morais [:reuben] 2012-03-13 03:57:47 PDT
Created attachment 605333 [details]
Reduced testcase.

Reduced testcase.
Comment 8 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-13 14:39:49 PDT
(In reply to Reuben Morais [:reuben] from comment #7)
> Created attachment 605333 [details]
> Reduced testcase.
> 
> Reduced testcase.

Can you upload a screen shot showing this testcase failing please, it works fine for me.
Comment 9 Reuben Morais [:reuben] 2012-03-14 18:06:12 PDT
Created attachment 606046 [details]
Video

(In reply to Chris Pearce (:cpearce) from comment #8)
> Can you upload a screen shot showing this testcase failing please, it works
> fine for me.

Not sure how a screenshot would demonstrate it so here's a screen recording instead.
Comment 10 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-14 18:19:25 PDT
Looks like your system is entering browser fullscreen mode rather than HTML fullscreen mode. Does this bug still occur when you run Firefox in safe mode? (I'm wondering if addons are at fault).

If running in safe mode doesn't fix this, could you try to find a regression range in nightly builds please?
Comment 11 Reuben Morais [:reuben] 2012-03-14 21:18:00 PDT
(In reply to Chris Pearce (:cpearce) from comment #10)
> Looks like your system is entering browser fullscreen mode rather than HTML
> fullscreen mode. Does this bug still occur when you run Firefox in safe
> mode? (I'm wondering if addons are at fault).
> 

Yes.

>
> If running in safe mode doesn't fix this, could you try to find a regression
> range in nightly builds please?
>

Will do, as soon as I have some time.
Comment 12 Reuben Morais [:reuben] 2012-03-18 18:32:15 PDT
Feb 07 2012 - works
Feb 08 2012 - fails (doesn't trigger browser fullscreen either)

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-02-07&enddate=2012-02-08

Bug 512529 looks suspicious.
Comment 13 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-18 18:49:22 PDT
You could try building locally with 02babc3139502171e5740 backed out, that's the only changeset from bug 512529 that should affect behaviour on MacOSX.
Comment 14 Reuben Morais [:reuben] 2012-03-22 18:08:17 PDT
(In reply to Chris Pearce (:cpearce) from comment #13)
> You could try building locally with 02babc3139502171e5740 backed out, that's
> the only changeset from bug 512529 that should affect behaviour on MacOSX.

I assume you meant 1b0294c7b952, which fixes the problem indeed. (And it looks so nice with the new Lion fullscreen).
Comment 15 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-22 18:19:42 PDT
Yes, I meant 1b0294c7b952. Philipp Hartwig, it looks like your changes from bug 512529 are breaking MacOSX fullscreen, can you take a look at this? Or can we back out 1b0294c7b952 safely?
Comment 16 Karl Tomlinson (:karlt) 2012-03-22 21:17:20 PDT
It was Neil that requested that change in bug 512529 comment 22.

If MacOSX doesn't suffer from bug 512529, then the new code can be ifndef XP_MACOSX, but it smells like there is some other bug at the root cause that is not understood.
Comment 17 Karl Tomlinson (:karlt) 2012-03-22 21:41:55 PDT
I wonder whether windowDidResize might be dispatching a non-fullscreen SizeMode
http://hg.mozilla.org/mozilla-central/annotate/996b89200406/widget/cocoa/nsCocoaWindow.mm#l1913
before 
http://hg.mozilla.org/mozilla-central/annotate/996b89200406/widget/cocoa/nsCocoaWindow.mm#l1187
Comment 18 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-22 23:06:47 PDT
Are we blaming 1b0294c7b952? I actually needed that to make Lion full screen work (and had essentially that as part of my patch before bug 512529 landed: https://bugzilla.mozilla.org/attachment.cgi?id=594868&action=diff#a/xpfe/appshell/src/nsWebShellWindow.cpp_sec1)

The test case here works for me (and I also ran this same sort of test _a lot_)... am I missing something?
Comment 19 Philipp Hartwig 2012-03-23 03:07:06 PDT
1b0294c7b952 will break things whenever the nsSizeMode_Fullscreen flag is not set correctly by the full screen code. For this reason full screen on OS X was broken between the landing of 1b0294c7b952 and 5f033ecdf2aa and it is therefore not a surprise that you observe a regression when looking at 1b0294c7b952 alone. (To be honest it looks like neither Neil nor I did think of OS X, very sorry about that. Luckily Paul's 5f033ecdf2aa landed only two days later.) 2012-02-10 is the first nightly to contain both changes.

Looking at your attachment 606046 [details] the behavior looks similar to a behavior I've observed myself under Linux. It's a wild guess but could you try if setting "full-screen-api.exit-on-deactivate" to "false" in about:config makes a difference? This option only exists since Firefox 13.
Comment 20 Reuben Morais [:reuben] 2012-03-23 04:06:28 PDT
(In reply to Philipp Hartwig from comment #19)
> It's a wild guess but could you try if setting "full-screen-api.exit-on-deactivate"
> to "false" in about:config makes a difference? This option only exists since
> Firefox 13.

It doesn't (at least not for this bug).
Comment 21 Reuben Morais [:reuben] 2012-03-23 04:16:18 PDT
By the way, backing out 1b0294c7b952 (https://tbpl.mozilla.org/?tree=Try&rev=0585136e6bbd) fixes the problem on Lion, and I can't see any side effects on Linux.
Comment 22 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-23 13:54:21 PDT
(In reply to Philipp Hartwig from comment #19)
> It's a wild guess but could you
> try if setting "full-screen-api.exit-on-deactivate" to "false" in
> about:config makes a difference? This option only exists since Firefox 13.

Just FYI, exit-on-deactivate is ignored with Lion fullscreen because of the way it moves the window to a standalone desktop.

(In reply to Reuben Morais [:reuben] from comment #21)
> By the way, backing out 1b0294c7b952
> (https://tbpl.mozilla.org/?tree=Try&rev=0585136e6bbd) fixes the problem on
> Lion, and I can't see any side effects on Linux.

3 things...

1. I still don't see the problem with latest nightly & a new profile. Reuben, is there anything in your profile that might be causing this to be a local issue? Has anybody else actually seen it recently? Or do I have the unique machine where it works?

2. Without that changeset, if you exit fullscreen via the menubar button on Lion, we don't fire the fullscreen event. With your try build, open the error console from the window with the test case and run the following code. You won't see the message when exiting full screen via the menubar button.
top.opener.addEventListener("fullscreen", function(){ top.opener.Services.console.logStringMessage("FS toggle"); }, false);

(it's worse when using domfullscreen because none of the toolbars get unhidden)

3. That was actually the problem on Linux too, so there should be (virtually the same) issues (I don't have a machine handy right now to test though).
Comment 23 Nickolay_Ponomarev 2012-03-23 19:31:52 PDT
I can also reproduce this (clean profile), and it just occurred to me that the browser window must be *maximized* for the bug to happen. Reuben, do you see that too?

If the browser window is not maximized, the video correctly goes full-screen the first time you try (although it first plays the animation of the browser window going full screen -- do you see that?).
Comment 24 Reuben Morais [:reuben] 2012-03-23 19:33:31 PDT
(In reply to Nickolay_Ponomarev from comment #23)
> I can also reproduce this (clean profile), and it just occurred to me that
> the browser window must be *maximized* for the bug to happen. Reuben, do you
> see that too?
> 

Yes.
Comment 25 Nickolay_Ponomarev 2012-03-24 19:30:35 PDT
Created attachment 609055 [details]
annotated callstack demonstrating the problem

Going fullscreen (both via Lion API and our older code) happens in two steps:
1) The window is resized, causing windowDidResize to be called.
2) Full screen is activated.

The issue here is that after the first step the sizemode (as determined by GetWindowSizeMode <http://hg.mozilla.org/mozilla-central/annotate/20a01901480f/widget/cocoa/nsCocoaWindow.mm#l1427>) is nsSizeMode_Normal, which is a change from nsSizeMode_Maximized in the case discussed here. So a NS_SIZEMODE event is fired, in response to which https://hg.mozilla.org/mozilla-central/rev/1b0294c7b952 from bug 512529 cancels the fullscreen mode.

Cancelling widget-level fullscreen also cancels the DOM fullscreen (see nsGlobalWindow::SetFullScreenInternal). The widget-level fullscreen is re-entered at a later stage, leading to the observed effect.

I think the most reasonable fix is to prevent windowDidResize from firing a sizemode event if we know we're transitioning to fullscreen. (A simpler one would be to just set mFullScreen earlier on, but that will send a fullscreen notification before windowDidEnterFullScreen).
Comment 26 Nickolay_Ponomarev 2012-03-24 19:35:51 PDT
Created attachment 609056 [details] [diff] [review]
don't fire extraneous NS_SIZEMODE events from windowDidResize

Here's a simple patch adding the flag. I tested it manually on 10.7 (going to/from fullscreen using Cmd+Shift+F, the Lion's full-screen controls, and the <video> fullscreen button, with iniial sizemode=maximized/normal, with/without fullscreenbutton="true" in browser.xul), but I won't get to writing a test for it or pushing it to try, hoping someone else will take over.
Comment 27 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-26 13:27:56 PDT
Comment on attachment 609056 [details] [diff] [review]
don't fire extraneous NS_SIZEMODE events from windowDidResize

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

Thanks for figuring this out! Apart from my comment about the variable name and the nit, I think this is pretty reasonable.

::: widget/cocoa/nsCocoaWindow.h
@@ +367,5 @@
>    bool                 mSheetNeedsShow; // if this is a sheet, are we waiting to be shown?
>                                          // this is used for sibling sheet contention only
>    bool                 mFullScreen;
> +  bool                 mInFullScreenTransition; // true from the request to enter/exit fullscreen
> +                                                // (MakeFullScreen() call) to EnteredFullScreen()

I'm wondering if it might make sense to make this a bit more general and obvious - something like mSuppressSizeModeEvents

::: widget/cocoa/nsCocoaWindow.mm
@@ +1475,5 @@
>  {
>    nsSizeMode newMode = GetWindowSizeMode(mWindow, mFullScreen);
> +  if (mInFullScreenTransition || // avoid firing sizemode from windowDidResize,
> +                                 // which fires before windowDidEnterFullScreen
> +      mSizeMode == newMode)

Nit: I would just add the comment on it's own line(s) above the if - this looks messy.
Comment 28 Nickolay_Ponomarev 2012-03-26 15:46:09 PDT
*** Bug 739358 has been marked as a duplicate of this bug. ***
Comment 29 Nickolay_Ponomarev 2012-03-26 15:52:06 PDT
Paul, thanks for your comments! As I said, I'd appreciate it if someone took over from here.
Comment 30 Nickolay_Ponomarev 2012-03-26 15:56:27 PDT
(In reply to Nickolay_Ponomarev from comment #23)
> If the browser window is not maximized, the video correctly goes full-screen
> the first time you try (although it first plays the animation of the browser
> window going full screen -- do you see that?).

BTW, I filed bug 739012 on that issue.
Comment 31 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-26 16:13:14 PDT
(In reply to Nickolay_Ponomarev from comment #29)
> Paul, thanks for your comments! As I said, I'd appreciate it if someone took
> over from here.

I can take over from here. Thanks again for getting it to this point!
Comment 32 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-28 14:42:56 PDT
Markus, r+ as is or should we do what I was thinking comment 27? (s/mInFullScreenTranstion/mSuppressSizeModeEvents/)
Comment 33 Markus Stange [:mstange] 2012-03-29 01:31:14 PDT
I prefer mInFullScreenTransition. As long as there are no other sources of size mode event suppression, I don't think we should name it that way, because then it looks like there could be multiple sources of suppression active at the same time. But if that were the case, EnteredFullScreen couldn't just set mSuppressSizeModeEvents = false unconditionally, and the reader of that line would wonder about handling that case.
Comment 34 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-29 10:40:23 PDT
(In reply to Markus Stange from comment #33)
> I prefer mInFullScreenTransition.

Fair enough.
Comment 35 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-29 13:36:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/79e4805da723
Comment 36 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-29 13:40:09 PDT
> https://hg.mozilla.org/integration/mozilla-inbound/rev/79e4805da723

This changeset included a comment change and I also made sure mInFullScreenTransition was initialized to false.
Comment 37 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-29 13:43:57 PDT
*** Bug 740398 has been marked as a duplicate of this bug. ***
Comment 38 Ed Morley [:emorley] 2012-03-30 13:02:33 PDT
https://hg.mozilla.org/mozilla-central/rev/79e4805da723
Comment 39 Paul Silaghi, QA [:pauly] 2012-04-03 00:33:47 PDT
Still an issue on Aurora: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120402 Firefox/13.0a2
Comment 40 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-01 10:53:44 PDT
Please nominate for beta approval and we'll get this in.
Comment 41 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-01 12:01:55 PDT
This patch and affected code are only on 14, so I don't think this needed to track 13.

(In reply to Paul Silaghi [QA] from comment #39)
> Still an issue on Aurora: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6;
> rv:13.0) Gecko/20120402 Firefox/13.0a2

Paul, can you expand on your steps to reproduce with 13?
Comment 42 Nickolay_Ponomarev 2012-05-01 13:39:09 PDT
Paul: this was a regression from bug 512529 which landed on 13, which is why I asked for tracking that release.

But you're right that the patch can't be landed on 13 as is, since the code being patched changed in 14 to support Lion full-screen: windowDidFailTo{Enter/ Exit}FullScreen must be removed from the patch and it should be tested on 13 if you decide to backport it.
Comment 43 Paul Silaghi, QA [:pauly] 2012-05-03 01:23:09 PDT
(In reply to Paul O'Shannessy [:zpao] from comment #41)
> Paul, can you expand on your steps to reproduce with 13?

STR:
1. Enable html5 (www.youtube.com/html5)
2. Open a video (https://www.youtube.com/watch?v=iRZ2Sh5-XuM)
3. Press fullscreen button

Actual results:
Browser goes fullscreen, video is still contained. If you press the fullscreen button again after that it works.

Expected results:
Video goes fullscreen, browser doesn't.

Tested on FF 13b2: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
Comment 44 Alex Keybl [:akeybl] 2012-05-08 13:13:11 PDT
(In reply to Nickolay_Ponomarev from comment #42)
> But you're right that the patch can't be landed on 13 as is, since the code
> being patched changed in 14 to support Lion full-screen:
> windowDidFailTo{Enter/ Exit}FullScreen must be removed from the patch and it
> should be tested on 13 if you decide to backport it.

Do you know when you expect to have a fix prepared for Firefox 13? We're going to build with our third of six betas today, so we'd like to get a feel for where we stand with this bug.
Comment 45 Nickolay_Ponomarev 2012-05-09 16:35:06 PDT
> Do you know when you expect to have a fix prepared for Firefox 13?
Just in case this question was directed at me: I'm not planning to work on this.
Comment 46 Vlad [QA] 2012-05-10 05:22:15 PDT
I've encountered a similar issue regarding ogg videos:
http://videos.mozilla.org/firefox4beta/Firefox_4_beta.theora.ogv

If I press the full screen button on the video, the browser enters in full screen and not the video.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0 beta 3
Comment 47 Vlad [QA] 2012-05-10 06:33:11 PDT
The same thing is happening on html5 videos on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20100101 Firefox/13.0 beta 3 and also on MacOS X 10.6.8
Comment 48 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-10 11:02:52 PDT
Created attachment 622792 [details] [diff] [review]
patch for beta

Nickolay, I left you as the author of the patch because it's still really your code. I carried over Markus's review as well. The only difference from what landed on m-c is minor - just moves the resetting of mInFullScreenTransition to MakeFullScreen.

[Approval Request Comment]
Regression caused by (bug #): bug 512529
User impact if declined: DOM fullscreen won't work for maximized windows on OS X
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Comment 49 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-11 15:47:06 PDT
Comment on attachment 622792 [details] [diff] [review]
patch for beta

low risk, fixed regression - approved for beta.
Comment 50 Alex Keybl [:akeybl] 2012-05-14 09:05:24 PDT
Please land today or tomorrow by the afternoon PT to make it into our next beta build. Thanks!
Comment 51 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-14 14:10:35 PDT
Landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/0cb005e477fc
Comment 52 Paul Silaghi, QA [:pauly] 2012-05-17 01:54:14 PDT
HTML5 video fullscreen is working fine now on Mac. Verified fixed on FF 13b4:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0

Note You need to log in before you can comment on or make changes to this bug.