Don't use mac fullscreen for dom fullscreen

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: timdream, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(9 attachments, 3 obsolete attachments)

3.87 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
875 bytes, patch
smichaud
: review+
Details | Diff | Splinter Review
4.99 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.89 KB, patch
dao
: review+
Details | Diff | Splinter Review
2.27 KB, patch
mstange
: review+
Details | Diff | Splinter Review
7.70 KB, application/x-gzip
Details
2.52 KB, patch
mstange
: review+
Details | Diff | Splinter Review
12.55 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
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

4 years ago
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?
Component: Untriaged → Widget: Cocoa
Flags: needinfo?(mstange)
Product: Firefox → Core
Summary: [Mac] Transition for HTML5 full screen is annoying → Don't use mac fullscreen for dom fullscreen
Safari doing it means its possible to use a custom animation, but I don't know how much work that would be.
Flags: needinfo?(mstange)

Updated

4 years ago
Duplicate of this bug: 1061917

Updated

4 years ago
Blocks: 1121280
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 1123170
(Assignee)

Updated

4 years ago
Blocks: 1160014

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
I'll fix this in Firefox 41. I almost forgot that I can fix this one when bug 947854 landed...
Depends on: 947854
(Assignee)

Comment 7

4 years ago
If I can fix this in the next week, the chances are that we could uplift it to Firefox 40.
(Assignee)

Updated

4 years ago
Assignee: nobody → quanxunzhen

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
Created attachment 8607849 [details] [diff] [review]
patch 1 - backout bug 740923
Attachment #8607849 - Flags: review?(smichaud)
(Assignee)

Comment 10

4 years ago
Created attachment 8607851 [details] [diff] [review]
patch 2 - suppress animation for hiding chrome
Attachment #8607851 - Flags: review?(smichaud)
(Assignee)

Comment 11

4 years ago
Created attachment 8607853 [details] [diff] [review]
patch 3 - correct fullscreen behavior for new window
Attachment #8607853 - Flags: review?(smichaud)
(Assignee)

Comment 12

4 years ago
Created attachment 8607856 [details] [diff] [review]
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).
Attachment #8607856 - Flags: review?(dao)
(Assignee)

Comment 13

4 years ago
Created attachment 8607857 [details] [diff] [review]
patch 5 - rename a parameter of SetFullScreenInternal
Attachment #8607857 - Flags: review?(bugs)
(Assignee)

Comment 14

4 years ago
Created attachment 8607878 [details] [diff] [review]
patch 6 - add method for widget to distinguish purpose of fullscreen
Attachment #8607878 - Flags: review?(roc)
(Assignee)

Comment 15

4 years ago
Created attachment 8607879 [details] [diff] [review]
patch 7 - use tranditional fullscreen for dom fullscreen
Attachment #8607879 - Flags: review?(smichaud)
(Assignee)

Comment 16

4 years ago
Created attachment 8607880 [details] [diff] [review]
patch 8 - always show fullscreen warning after reactivation
Attachment #8607880 - Flags: review?(dao)
(Assignee)

Comment 17

4 years ago
(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.
Attachment #8607856 - Flags: review?(dao) → review+
Attachment #8607880 - Flags: review?(dao) → review+
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.
Attachment #8607857 - Flags: review?(bugs) → review+
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.
Attachment #8607849 - Flags: review?(smichaud) → review+
(Assignee)

Comment 20

4 years ago
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.
Depends on: 1167890
(Assignee)

Updated

4 years ago
Attachment #8607856 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8610255 [details] [diff] [review]
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.
Attachment #8610255 - Flags: review?(smichaud)
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).
Attachment #8607851 - Flags: review?(smichaud) → review+
(Assignee)

Comment 23

4 years ago
(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 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."
(Assignee)

Comment 25

4 years ago
(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 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 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.
Attachment #8610255 - Flags: review?(smichaud) → review?(mstange)
(Assignee)

Comment 28

4 years ago
(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 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.
Attachment #8610255 - Flags: review?(mstange) → review+
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.
Attachment #8607853 - Flags: review?(smichaud) → review-
> 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.
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.
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.
> 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).
Created attachment 8612989 [details]
Interpose library to hook uname() and lie that we're running on OS X 10.6.8
> 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.
(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?
Flags: needinfo?(smichaud)
(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.
> 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?
Flags: needinfo?(smichaud)
(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.
(Assignee)

Updated

4 years ago
Blocks: 1170369
>>> 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 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)."
Attachment #8607853 - Flags: review- → review+
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 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.
Attachment #8607879 - Flags: review?(smichaud) → review+
> 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.
(Assignee)

Comment 46

4 years ago
(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.
(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.
(Assignee)

Comment 48

4 years ago
(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.
(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?
(Assignee)

Comment 50

4 years ago
Created attachment 8614356 [details] [diff] [review]
patch 3 - save and restore window collection behavior
Attachment #8607853 - Attachment is obsolete: true
Attachment #8614356 - Flags: review?(mstange)
(Assignee)

Comment 51

4 years ago
Created attachment 8614377 [details] [diff] [review]
patch 7 - use tranditional fullscreen for dom fullscreen
Attachment #8607879 - Attachment is obsolete: true
Attachment #8614377 - Flags: review?(smichaud)
Attachment #8614356 - Flags: review?(mstange) → review+
Attachment #8614377 - Flags: review?(smichaud) → review+
(Assignee)

Comment 52

4 years ago
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.
Depends on: 1172601

Updated

4 years ago
Depends on: 1173340
(Assignee)

Updated

4 years ago
Blocks: 900453
(Assignee)

Updated

4 years ago
Blocks: 802504
(Assignee)

Updated

4 years ago
Blocks: 792304

Updated

4 years ago
Depends on: 1181904
(Assignee)

Updated

4 years ago
Depends on: 1188322
You need to log in before you can comment on or make changes to this bug.