726943 - Fullscreen button in HTML video makes the browser go fullscreen but not the video

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Reuben Morais [:reuben]]

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 2

[Reuben Morais [:reuben]]

Actually, this still happens here.

Comment 3

[Paul Silaghi, QA [:pauly]]

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

[Chris Pearce [:cpearce (Not reading bugmail)]]

WFM on Win7x64 and OSX10.7 20120312...

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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]]

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]]

Attachment: Reduced testcase.

Reduced testcase.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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]]

Attachment: 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

[Chris Pearce [:cpearce (Not reading bugmail)]]

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]]

(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]]

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

[Chris Pearce [:cpearce (Not reading bugmail)]]

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]]

(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

[Chris Pearce [:cpearce (Not reading bugmail)]]

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)]

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)]

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 bugmail, email directly)]

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]

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]]

(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]]

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 bugmail, email directly)]

(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]

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]]

(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]

Attachment: 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]

Attachment: 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 bugmail, email directly)]

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 29

[Nickolay_Ponomarev]

Paul, thanks for your comments! As I said, I'd appreciate it if someone took over from here.

Comment 30

[Nickolay_Ponomarev]

(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 bugmail, email directly)]

(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 bugmail, email directly)]

Markus, r+ as is or should we do what I was thinking comment 27? (s/mInFullScreenTranstion/mSuppressSizeModeEvents/)

Comment 33

[Markus Stange [:mstange]]

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 bugmail, email directly)]

(In reply to Markus Stange from comment #33)
> I prefer mInFullScreenTransition.

Fair enough.

Comment 35

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79e4805da723

Comment 36

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

> 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 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/79e4805da723

Comment 39

[Paul Silaghi, QA [:pauly]]

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]

Please nominate for beta approval and we'll get this in.

Comment 41

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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]

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]]

(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]]

(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]

> 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]]

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]]

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 bugmail, email directly)]

Attachment: 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]

Comment on attachment 622792 [details] [diff] [review]
patch for beta

low risk, fixed regression - approved for beta.

Comment 50

[Alex Keybl [:akeybl]]

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 bugmail, email directly)]

Landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/0cb005e477fc

Comment 52

[Paul Silaghi, QA [:pauly]]

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