1105939 - Don't use mac fullscreen for dom fullscreen

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. Go to https://vimeo.com/ , find an video
2. play it
2. hit the full screen button on screen

Expected:

1. Video occupies full screen without delay

Actual:

1. The Video freezes so the Mac full screen transition can take place, moving Firefox window to a new Space
2. The confirmation dialog only appears once the transition is complete

Note:

1. This is suboptimal experience compare to Flash video. If we want to tell people HTML5 video is better we should make the UX better as well.

Comment 1

[:Gijs (he/him)]

Safari does something else here as well. I don't know how easy it is to fix this on the platform side - AFAIK we only really support using fullscreen on mac with the platform animations. Markus?

Comment 2

[Markus Stange [:mstange]]

Safari doing it means its possible to use a custom animation, but I don't know how much work that would be.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Markus Stange [:mstange] from comment #2)
> Safari doing it means its possible to use a custom animation, but I don't
> know how much work that would be.

Safari's behavior is very special. It creates another window to place the requesting content, and then make that window fullscreen. You can find that, with some content fullscreen, the original window is still there. I don't think it is actually what we want. (And I think it needs much more work as well.)

But anyway, Mac does support custom animations, and I don't think it need much work to use it. The bad news is that we cannot control how long it takes to enter and exit the fullscreen mode, we can only control how the window transforms. But the good news is that, it passes in the duration of the animation, which means we can move our controls accordingly.

A simple workaround is to make the window fullscreen immediately without the transform animation. It could covers the whole screen, so that user won't be aware of the moving animation happens in the background. But the exit fullscreen part is not that good, then.

Comment 5

[Cory]

I'd really like for this option to be added sooner or later. The fullscreen animation is incredibly frustrating. It's the reason I'm still using the Flash player on Youtube, but I wouldn't be surprised if Google eventually deprecates it.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I'll fix this in Firefox 41. I almost forgot that I can fix this one when bug 947854 landed...

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

If I can fix this in the next week, the chances are that we could uplift it to Firefox 40.

Comment 8

[Cory]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> I'll fix this in Firefox 41. I almost forgot that I can fix this one when
> bug 947854 landed...

Good to hear! I look forward to it.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - backout bug 740923

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - suppress animation for hiding chrome

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - correct fullscreen behavior for new window

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - remove redundant dom fullscreen cleanup

Since MozExitedDomFullscreen will be called anyway, the cleanupDomFullscreen call in cleanup for fullscreen event is redundant.

Further more, if we do not remove this redundant call, and we cancel the warning dialog in the synchronous fullscreen event, there would be some problem with the chrome document (which took me a long time to debug).

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 5 - rename a parameter of SetFullScreenInternal

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 6 - add method for widget to distinguish purpose of fullscreen

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 7 - use tranditional fullscreen for dom fullscreen

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 8 - always show fullscreen warning after reactivation

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #7)
> If I can fix this in the next week, the chances are that we could uplift it
> to Firefox 40.

Since this bug is proven to be far more complicated than I previously thought, I wouldn't request uplifting it.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8607857 [details] [diff] [review]
patch 5 - rename a parameter of SetFullScreenInternal

Even better would be using some enum which would give better hint for the caller.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607849 [details] [diff] [review]
patch 1 - backout bug 740923

I already tested this elsewhere and found there were no problems.

But I've got a couple of urgent things on the fire, so I'll need to postpone my other reviews until next week.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

I steal the code in patch 4 of this bug for bug 1167890. Without bug 1167890, the patch 4 itself will likely cause another regression that exiting fullscreen mode won't exit DOM fullscreen.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - avoid rounded corner on tranditional fullscreen window

Since I moved the code in patch 4 to other bug, reuse the part number for this new patch.

This patch treats a window without titlebar as fullscreen window, so that we don't draw rounded corner on it. The original fullscreen detection was introduced by bug 959570.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607851 [details] [diff] [review]
patch 2 - suppress animation for hiding chrome

This does what it promises -- it suppresses "show window" animation (what you'd normally get whenever a new window is created) during the transitions to and from fullscreen mode.  I think it looks reasonably good (though I'm not the best judge of that).  And it stops any sort of OS-run animation from taking place during these transitions -- which certainly simplifies the process.

But otherwise I'm not entirely sure why you're doing this.  Does the OS-run animation cause timing problems?  Is it that it's at least partially hidden (which you hinted at in comment #4, and which I noticed in my own tests)?

In any case, though, I strongly sympathize with the idea of suppressing any sort of OS-run animation during the transition to/from fullscreen mode.  The OS-run stuff lives in black boxes, and is hard to control directly.  Your patches don't currently have any kind of transition animation.  But if we decide we want it, we'll probably be happier with the results if we do it entirely in Gecko (and don't use the OS-run stuff at all).

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Steven Michaud from comment #22)
> Comment on attachment 8607851 [details] [diff] [review]
> patch 2 - suppress animation for hiding chrome
> 
> But otherwise I'm not entirely sure why you're doing this.  Does the OS-run
> animation cause timing problems?  Is it that it's at least partially hidden
> (which you hinted at in comment #4, and which I noticed in my own tests)?

No, it doesn't cause timing problem at least at present. But I feel it looks awkward to have a "create new window" transition for entering fullscreen, doesn't it?

Also that transition is not compatible with our target to have a unified transition among all desktop platform in bug 1160014.

> In any case, though, I strongly sympathize with the idea of suppressing any
> sort of OS-run animation during the transition to/from fullscreen mode.  The
> OS-run stuff lives in black boxes, and is hard to control directly.  Your
> patches don't currently have any kind of transition animation.  But if we
> decide we want it, we'll probably be happier with the results if we do it
> entirely in Gecko (and don't use the OS-run stuff at all).

We are indeed going to do that.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607853 [details] [diff] [review]
patch 3 - correct fullscreen behavior for new window

The fullscreen button (and NSWindowCollectionBehaviorFullScreenPrimary) are only supported on OS X 10.7 and up.  So this block should probably also have a nsCocoaFeatures::OnLionOrLater() condition.  (I realize that we can probably assume that mUsesNativeFullScreen is only ever true on 10.7 and up, but I'd prefer that we played it safe.)

+  // If we are displaying chrome, and we could use native fullscreen,
+  // we need to set this behavior here for the new window.

This comment is unclear.  It should be something like:

"If we are unhiding chrome and our window supports Lion-style native fullscreen mode, make sure our window's titlebar has a fullscreen button."

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Steven Michaud from comment #24)
> Comment on attachment 8607853 [details] [diff] [review]
> patch 3 - correct fullscreen behavior for new window
> 
> The fullscreen button (and NSWindowCollectionBehaviorFullScreenPrimary) are
> only supported on OS X 10.7 and up.  So this block should probably also have
> a nsCocoaFeatures::OnLionOrLater() condition.  (I realize that we can
> probably assume that mUsesNativeFullScreen is only ever true on 10.7 and up,
> but I'd prefer that we played it safe.)

I think we use that assertion in a wide range of places. If you want to be safer, I guess we probably could just add a MOZ_ASSERT(whatever check makes you comfortable) there.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607853 [details] [diff] [review]
patch 3 - correct fullscreen behavior for new window

> So this block should probably also have a nsCocoaFeatures::OnLionOrLater()
> condition.

On second thought don't bother with this, or even with an assertion.  There's at least one other place where we use Lion-specific APIs when mUsesNativeFullScreen is true, without checking that we're on 10.7 and up.

That's not how I would have written the code ... but there's such a thing as being too picky :-)

Do change the comment, though.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8610255 [details] [diff] [review]
patch 4 - avoid rounded corner on tranditional fullscreen window

I'm going to pass this to Markus since he knows the corner-rounding code much better than I do.

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Steven Michaud from comment #26)
> Do change the comment, though.

OK, I'll change that comment. So, does this patch otherwise look good?

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607853 [details] [diff] [review]
patch 3 - correct fullscreen behavior for new window

> So, does this patch otherwise look good?

Actually, now that I look at this again, I wonder why nsCocoaWindow::SetShowsFullScreenButton() doesn't take care of things, and make this code unnecessary.  So my answer is "no", at least for the time being.

Sorry for the confusion.  It's late in the day and I'm getting tired.

I'll pick this up again tomorrow.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607853 [details] [diff] [review]
patch 3 - correct fullscreen behavior for new window

This patch is wrong.  In fact the collection behavior is already correctly set for this window (so that it should display a fullscreen button on the right of the titlebar).

But (without this part of your patch) the fullscreen button isn't displayed when you come out of dom fullscreen mode.  I'll be trying to figure out why.

Note that even with this part of your patch, the fullscreen button often gets displayed slightly offset from its correct position.  We do need to intervene to fix this problem (of the disappearing fullscreen button), but we need to do it in another way.

I'll be working on this.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

> But (without this part of your patch) the fullscreen button isn't
> displayed when you come out of dom fullscreen mode.

In fact none of the titlebar buttons are displayed.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Just for grins, I tried altering nsCocoaWindow::MakeFullScreen() (in current trunk code) so that it always uses the non-native codepath (not just on SnowLeopard, but on all versions of OS X).  That also doesn't work properly (on OS X 10.7.5), and also makes the fullscreen button disappear (when you use cmd-shift-f to go into fullscreen mode) -- though the other titlebar buttons don't disappear.

So what works fine on SnowLeopard doesn't work on other versions of OS X.  I'm going to try to find out why.

Comment 33

[Markus Stange [:mstange]]

I wonder whether attachment 803077 [details] [diff] [review] (which simplifies the 10.6-style full screen code to just call setStyleMask instead of creating a new NSWindow) would help with the full screen button problem.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

> So what works fine on SnowLeopard doesn't work on other versions of OS X.

Actually this isn't true.  The SnowLeopard (Gecko only) fullscreen mode works just fine (using cmd-shift-f) on Lion if I use an interpose library to hook uname() and lie that the utsname.release is "10.8.0" (i.e. OS X 10.6.8).  uname() is used by _MD_getsysinfo(), which is in turn used to help initialize the Services.sysinfo object, which is used by most high-level (cross-platform) code to determine what version of the OS we're running on.

I'll try your idea, Markus.  But I really want to get to the bottom of this, which is going to take several days (well into next week).

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library to hook uname() and lie that we're running on OS X 10.6.8

Comment 36

[Steven Michaud [:smichaud] (Retired)]

> The SnowLeopard (Gecko only) fullscreen mode works just fine (using cmd-shift-f) on Lion

Also on Yosemite (10.10.3) and Mavericks (10.9.5).  Though note that we don't have a fullscreen button on Yosemite, so it can't disappear.

Comment 37

[Jet Villegas (inactive)]

(In reply to Steven Michaud from comment #30)
> Comment on attachment 8607853 [details] [diff] [review]
> patch 3 - correct fullscreen behavior for new window
> 
> This patch is wrong.  In fact the collection behavior is already correctly
> set for this window (so that it should display a fullscreen button on the
> right of the titlebar).
> 
> But (without this part of your patch) the fullscreen button isn't displayed
> when you come out of dom fullscreen mode.  I'll be trying to figure out why.
> 
> Note that even with this part of your patch, the fullscreen button often
> gets displayed slightly offset from its correct position.  We do need to
> intervene to fix this problem (of the disappearing fullscreen button), but
> we need to do it in another way.
> 
> I'll be working on this.

Steven: Did you spin off a new bug for this work?

Comment 38

[Jet Villegas (inactive)]

(In reply to Steven Michaud from comment #36)
> > The SnowLeopard (Gecko only) fullscreen mode works just fine (using cmd-shift-f) on Lion
> 
> Also on Yosemite (10.10.3) and Mavericks (10.9.5).  Though note that we
> don't have a fullscreen button on Yosemite, so it can't disappear.

Exiting fullscreen in Yosemite requires clicking on the green system button again. At least that's what I see for both Firefox and Chrome now.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

> Steven: Did you spin off a new bug for this work?

Nope.  The problem I found needs to be figured out before any of this bug's patches can land.

> the green system button

You mean the zoom button, I suppose.  Were you testing with my interpose library?

Comment 40

[Jet Villegas (inactive)]

(In reply to Steven Michaud from comment #39)
> > Steven: Did you spin off a new bug for this work?
> 
> Nope.  The problem I found needs to be figured out before any of this bug's
> patches can land.

OK, I was just clarifying if it's you or Xidorn who's hacking on that part.

> > the green system button
> 
> You mean the zoom button, I suppose.  Were you testing with my interpose
> library?

No, just using standard Nightly. It just seems like entering/exiting app fullscreen on Yosemite is done with that control now.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

>>> the green system button
>>
>> You mean the zoom button, I suppose.  Were you testing with my
>> interpose library?
>
> No, just using standard Nightly. It just seems like entering/exiting
> app fullscreen on Yosemite is done with that control now.

Yes.  On Yosemite the zoom button as been repurposed to enter or exit
"normal" fullscreen mode.  That's why Yosemite no longer needs what
I've been calling a fullscreen button (the double arrow that used to
appear on the right side of the titlebar).

Comment 42

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607853 [details] [diff] [review]
patch 3 - correct fullscreen behavior for new window

When I r-ed this patch, I didn't fully understand its context.  Since we're creating a new native window here, we also need to do something to ensure that its titlebar displays a fullscreen button (if that's appropriate).  nsCocoaWindow::SetShowsFullScreenButton() operated on a native window that has since been destroyed (in the call to DestroyNativeWindow() a few lines above).

Furthermore, the disappearing/misplaced titlebar buttons problem seems to have disappeared in current trunk code (when I applied this bug's patches to it).  Well *almost* disappeared -- the fullscreen button is still displayed a little too high.  But I think we can just open another bug to deal with that.  (And if the much worse problem I saw comes back later, we can open yet another bug to deal with that.)

So I'm going to r+ this patch in its current form (though I'll request a change to the comment, below).  But please don't land any of this bug's patches until I review part 7, where I'll ask for some variable-name changes.

+  // If we are displaying chrome, and we could use native fullscreen,
+  // we need to set this behavior here for the new window.

Please replace this with something like:

"If we are unhiding chrome and we support Lion-style native fullscreen mode, make sure our new window's titlebar has a fullscreen button (where appropriate)."

Comment 43

[Markus Stange [:mstange]]

I'd prefer if this was handled in importState / exportState since that's where we're handling the other window properties that need to be preserved throughout window recreation.

Comment 44

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8607879 [details] [diff] [review]
patch 7 - use tranditional fullscreen for dom fullscreen

This basically looks fine to me, but some of the variables are now horribly misnamed:

  bool                 mFullScreen;
  bool                 mUsingNativeFullScreen;

These both indicate that we're in a (DOM or "regular") fullscreen mode -- either *any* fullscreen mode (mFullScreen) or specifically a native fullscreen mode (mUsingNativeFullScreen).  Their names need to do a better job of indicating this.

How about mInFullScreenMode and mInNativeFullScreenMode?

We may also eventually need a mInDOMFullScreenMode.

  bool                 mUsesNativeFullScreen;

This really means whether or not we support "regular" native fullscreen mode.  So it should be renamed to something like mSupportsNativeFullScreenMode.

Comment 45

[Steven Michaud [:smichaud] (Retired)]

> I'd prefer if this was handled in importState / exportState ...

That's also fine with me.

Markus, you should probably explain in more detail what you want.

Comment 46

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Markus Stange [:mstange] from comment #43)
> I'd prefer if this was handled in importState / exportState since that's
> where we're handling the other window properties that need to be preserved
> throughout window recreation.

I don't think this is a state we're preserving. When we hide the chrome, the fullscreen behavior is fine to be stripped. We only need to restore it when we restore from fullscreen to normal window. Hence using importState / exportState could probably be confusing.

But after checking the code of those functions, it seems to me that we should restore the behavior bit before calling importState. I suppose it could solve the problem that the fullscreen button is higher than where it should be.

Comment 47

[Markus Stange [:mstange]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #46)
> (In reply to Markus Stange [:mstange] from comment #43)
> > I'd prefer if this was handled in importState / exportState since that's
> > where we're handling the other window properties that need to be preserved
> > throughout window recreation.
> 
> I don't think this is a state we're preserving. When we hide the chrome, the
> fullscreen behavior is fine to be stripped.

Hmm, true. But does it absolutely need to be stripped? There are other parts of the state that don't make much sense for borderless windows but we still import / export those parts of the state, so I think it would still fit in well in those functions.

Comment 48

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Markus Stange [:mstange] from comment #47)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #46)
> > I don't think this is a state we're preserving. When we hide the chrome, the
> > fullscreen behavior is fine to be stripped.
> 
> Hmm, true. But does it absolutely need to be stripped? There are other parts
> of the state that don't make much sense for borderless windows but we still
> import / export those parts of the state, so I think it would still fit in
> well in those functions.

No, it doesn't absolutely need to. If we can store the whole window collection behavior, I guess that would be fine to do it in importState / exportState. But if we only save and restore the fullscreen bit, I suspect that would simply make things more complicated without adding value.

Comment 49

[Markus Stange [:mstange]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #48)
> (In reply to Markus Stange [:mstange] from comment #47)
> > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #46)
> > > I don't think this is a state we're preserving. When we hide the chrome, the
> > > fullscreen behavior is fine to be stripped.
> > 
> > Hmm, true. But does it absolutely need to be stripped? There are other parts
> > of the state that don't make much sense for borderless windows but we still
> > import / export those parts of the state, so I think it would still fit in
> > well in those functions.
> 
> No, it doesn't absolutely need to. If we can store the whole window
> collection behavior,

Is there a reason we can't?

Comment 50

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - save and restore window collection behavior

Comment 51

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 7 - use tranditional fullscreen for dom fullscreen

Comment 52

[Xidorn Quan [:xidorn] UTC+11]

Looks like the new patchset works fine with the current testset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=069b1b271a9c

IIRC, there are a bunch of fullscreen tests disabled on 10.7+. We probably can enable them now. I've filed bug 1170369 for that.

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c75bb91474
https://hg.mozilla.org/integration/mozilla-inbound/rev/237eb57fc917
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aeee224c758
https://hg.mozilla.org/integration/mozilla-inbound/rev/70ab2a1ad75e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b801a65cc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6175e10def
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcf7e5dc2d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ffe2b5199f

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2c75bb91474
https://hg.mozilla.org/mozilla-central/rev/237eb57fc917
https://hg.mozilla.org/mozilla-central/rev/8aeee224c758
https://hg.mozilla.org/mozilla-central/rev/70ab2a1ad75e
https://hg.mozilla.org/mozilla-central/rev/c6b801a65cc6
https://hg.mozilla.org/mozilla-central/rev/af6175e10def
https://hg.mozilla.org/mozilla-central/rev/ffcf7e5dc2d2
https://hg.mozilla.org/mozilla-central/rev/00ffe2b5199f