When going fullscreen, update the sizemode attribute before the OS provided cross-fade animation starts

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: spohl)

Tracking

(Depends on 1 bug)

Trunk
mozilla58
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual][p4][tpi:+])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1372535 +++

from bug 1372535 comment #7:

> (In reply to Johann Hofmann [:johannh] from comment #5)
> > I'm fine with this patch, but it doesn't fix the problem with the
> > double-jumping tabs when switching to fullscreen and therefore wouldn't
> > resolve this bug.
> 
> It sounds like a completely different bug though.
> The problem reported in this bug is: Zooming a window first animates the
> window size and then adjusts the drag space in one discrete step. This
> problem would be solved by not adjusting the drag space on zooming.
> The problem that's now being brought up is: Making a window fullscreen
> animates to the fullscreen state and then adjusts the drag space in *two*
> discrete steps. That's a legitimate problem but I think it should be fixed
> in a different bug.
> 
> > I really think this should have its own animation instead.
> > We can't prevent jumping (though we could try to get rid of the double
> > jumping), so animating the movement sounds best to me.
> 
> The system provides us with a cross-fade animation during the fullscreen
> transition; it fades the non-fullscreen window to the fullscreen window. We
> need to make sure that this fullscreen state rendering already has the
> adjustment applied. The problem is that we only apply it after the
> transition has completed. But if we apply it sooner, then I think the
> system-provided cross-fade animation is all we need.
Flags: qe-verify+
See Also: → 1372535
QA Contact: brindusa.tot
Whiteboard: [photon-visual][p2] → [photon-visual][p2][tpi:+]
Reporter

Updated

2 years ago
Whiteboard: [photon-visual][p2][tpi:+] → [reserve-photon-visual][p4][tpi:+]
Priority: P2 → P3
Priority: P3 → P4
Comment hidden (mozreview-request)
Markus, I have no idea whether this is a sane thing to do or not, this has been bugging me for a while and I thought I'd give it a shot. It improves things for me locally (it's jumping once instead of twice now, we'll still have to look into the second jump).

Let me know what you think about this approach. :)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P4 → P1
Comment on attachment 8907777 [details]
Bug 1373581 - OSX: send fullscreen event on WillEnter/WillExit.

https://reviewboard.mozilla.org/r/179456/#review187090

I think the approach is good. If it passes tests, let's land it.
Attachment #8907777 - Flags: review?(mstange) → review+
Assignee

Comment 5

2 years ago
Comment on attachment 8907777 [details]
Bug 1373581 - OSX: send fullscreen event on WillEnter/WillExit.

Please hold off on landing this for now. I was just looking at this code recently and may be able to find a fix for both jumps.
Attachment #8907777 - Flags: review-
(In reply to Stephen A Pohl [:spohl] from comment #5)
> Comment on attachment 8907777 [details]
> Bug 1373581 - OSX: send fullscreen event on WillEnter/WillExit.
> 
> Please hold off on landing this for now. I was just looking at this code
> recently and may be able to find a fix for both jumps.

Sure, no problem, I was about to look at the failing tests first anyway, but I'll hold off for now. Please keep us updated, it would be great if we could uplift this to 57.
Iteration: 57.3 - Sep 19 → ---
(In reply to Stephen A Pohl [:spohl] from comment #5)
> Comment on attachment 8907777 [details]
> Bug 1373581 - OSX: send fullscreen event on WillEnter/WillExit.
> 
> Please hold off on landing this for now. I was just looking at this code
> recently and may be able to find a fix for both jumps.

Any news?
Flags: needinfo?(spohl.mozilla.bugs)
Assignee

Comment 8

2 years ago
Sorry, my patch isn't working 100% yet and I've had to look into regressions from our SDK switch and macOS 10.13. I should have mentioned that part of the reason why I asked to hold off on landing this patch was because of the failures on try. They seemed related to fullscreen mode. If you're able to resolve the failures on try, feel free to land this patch. I will back it out once I'm able to land a complete patch.
Flags: needinfo?(spohl.mozilla.bugs)
Duplicate of this bug: 1402352

Updated

2 years ago
Duplicate of this bug: 1402817
Assignee

Comment 12

2 years ago
Posted patch PatchSplinter Review
Here is a patch that fixes both jumps when transitioning in and out of fullscreen. If this approach is sensible, I had two questions/remarks:

1. I'm not sure about the names for the new events. Please let me know if they could be named better.
2. On macOS, we're now basically setting and removing the "inFullscreen" attribute twice. Once in willToggle() and once in toggle(). I don't *think* that's a problem, but would appreciate confirmation.

Try is green (comment 11).
Attachment #8915835 - Flags: review?(mstange)
Reporter

Comment 13

2 years ago
Do we really need new events for this? Can you at least make it clear in browser-fullScreenAndPointerLock.js that they don't work across platforms?
Thanks, I'll try this patch later today! I'm also not a big fan of an additional event, can we prefix it with "MacOSX" or something?

FWIW, I think one of the two jumps was already fixed by bug 1390025 (at least on my machine), so my patch would theoretically also solve both, afaiu. I still haven't been able to wrap my head around the test failures (I didn't spend much time on it), I just wanted to mention it.
Comment on attachment 8915835 [details] [diff] [review]
Patch

I have the same question as Dão - why is the new event necessary? What breaks if you just change when the old event is fired, and how does the new event fix that?
Assignee

Comment 17

2 years ago
(In reply to Markus Stange [:mstange] from comment #15)
> Comment on attachment 8915835 [details] [diff] [review]
> Patch
> 
> I have the same question as Dão - why is the new event necessary? What
> breaks if you just change when the old event is fired, and how does the new
> event fix that?

I have finally been able to isolate this: The reason that this breaks on try is because we're toggling fullscreen faster internally than we do natively. In many of our fullscreen tests that toggle fullscreen, the sequence used to be as follows:

1. JS: BrowserFullScreen() (we want to enter fullscreen)
2. Cocoa: windowWillEnterFullScreen
3. Cocoa: windowDidEnterFullScreen
4. JS: sizemodechange event
5. JS: BrowserFullScreen() (we want to exit fullscreen)
6. Cocoa: windowWillExitFullScreen
7. Cocoa: windowDidExitFullScreen
8. JS: sizemodechange event

By dispatching the fullscreen event in windowWillEnterFullscreen instead of windowDidEnterFullScreen, the sequence becomes:

1. JS: BrowserFullScreen() (we want to enter fullscreen)
2. Cocoa: windowWillEnterFullScreen
3. JS: sizemodechange event
4. JS: BrowserFullScreen() (we want to exit fullscreen)
5. Cocoa: windowDidEnterFullScreen

So, we're trying to exit fullscreen before windowDidEnterFullScreen was called. The tests time out because we never get a second sizemodechange event, because we never exit fullscreen.

The new events in my patch maintain the previous sequence, but allows us to set and remove the "inFullscreen" attribute on the document earlier to allow for a smoother fullscreen transition without tab jumps.

I haven't used a macOS-specific event name prefix because other platforms could potentially implement these events as well. But I'm happy to change this if desired.
Flags: needinfo?(mstange)
Assignee

Comment 18

2 years ago
Comment on attachment 8915835 [details] [diff] [review]
Patch

(see comment 17)
Attachment #8915835 - Flags: review?(mstange)
Reassigning to Stephen. FWIW, this sounds like an okay solution to me.
Assignee: jhofmann → spohl.mozilla.bugs
Comment on attachment 8915835 [details] [diff] [review]
Patch

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

Makes sense. I think this is a good solution. Thanks for digging into it!
Attachment #8915835 - Flags: review?(mstange) → review+
Flags: needinfo?(mstange)
Assignee

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a94c21fb2d184b93e0b2b9dd01254d79ad4f6491
Bug 1373581: Make transition into and out of native fullscreen smoother on macOS. r=mstange
Assignee

Updated

2 years ago
Attachment #8907777 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a94c21fb2d18
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 23

2 years ago
Comment on attachment 8915835 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Photon
[User impact if declined]: Bad UX when going fullscreen on macOS.
[Is this code covered by automated tests?]: Yes, fullscreen is covered by automated tests.
[Has the fix been verified in Nightly?]: Yes, in a local build off of Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Click on the green fullscreen button in the top left corner of the browser window. Watch tabs jump after fullscreen has been entered before this fix. Tabs are already in the right place when fullscreen is entered after this fix. 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This doesn't touch any existing code and simply adds two new events on macOS to move the tabs slightly earlier than before.
[String changes made/needed]: none
Attachment #8915835 - Flags: approval-mozilla-beta?
Assignee

Comment 24

2 years ago
(In reply to Stephen A Pohl [:spohl] from comment #23)
> [Has the fix been verified in Nightly?]: Yes, in a local build off of
> Nightly.

... and also verified with the latest official Nightly just now.
Comment on attachment 8915835 [details] [diff] [review]
Patch

Seems like a must, fix was verified on Nightly, Beta57+
Attachment #8915835 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 27

2 years ago
Posted video Fullscreen_Bug.mov
(In reply to Stephen A Pohl [:spohl] from comment #24)
> (In reply to Stephen A Pohl [:spohl] from comment #23)
> > [Has the fix been verified in Nightly?]: Yes, in a local build off of
> > Nightly.
> 
> ... and also verified with the latest official Nightly just now.

Hello Stephen,

thank you for fixing this bug.

But I noticed an issue with this change:

Steps to reproduce:

1.) Open a new window
2.) Click on the green Window Control Button, so that the window goes into Fullscreen Mode
3.) Don't move the mouse cursor, so that the cursor keep stays at the place where you have clicked the green Window Control Button
4.) Now press SHIFT-CMD-F to leave the Fullscreen Mode

Actual: The Window Control Buttons do not appear, until you move the cursor out of the Window Control Buttons area.

Expected: The Window Control Buttons should appear immediately.

This is may be caused, because the buttons do not appear until the Exit-Fullscreen-Animation is finished. Normally on OSX they appear during the Exit animation.

Please find attached a screencast which demonstrates the issue.

Could you please take a look at it? Please let me now, if I file a separate report for it.

Thank you in advance.
Flags: needinfo?(spohl.mozilla.bugs)

Comment 28

2 years ago
Additional information to my comment 27:

I tried to reproduce it after I relaunched Nightly, but I wasn't able to do it.

So, I noticed, that this issue only happens, when Firefox 57.0b10 was running before.

So the steps should be:

0.) Start Firefox 57.0b10 and quit it.
1.) Start Nightly (and open a new window)
2.) Click on the green Window Control Button, so that the window goes into Fullscreen Mode
3.) Don't move the mouse cursor, so that the cursor keep stays at the place where you have clicked the green Window Control Button
4.) Now press SHIFT-CMD-F to leave the Fullscreen Mode

Actual: The Window Control Buttons do not appear, until you move the cursor out of the Window Control Buttons area.

Expected: The Window Control Buttons should appear immediately.

May be this issue can be ignored, since it isn't reproducible after relaunch of Nightly.

What do you think? Thank you.
Assignee

Comment 29

2 years ago
This seems like less of a priority if Firefox 57.0b10 has to be run before Nightly to reproduce the issue. Either way, it would be good to capture this in a separate bug. Thanks!
Flags: needinfo?(spohl.mozilla.bugs)

Updated

2 years ago
Depends on: 1410698

Comment 30

2 years ago
(In reply to Stephen A Pohl [:spohl] from comment #29)
> This seems like less of a priority if Firefox 57.0b10 has to be run before
> Nightly to reproduce the issue. Either way, it would be good to capture this
> in a separate bug. Thanks!

Hello Stephen,

It seems that Firefox 57.0b10 should not have to run before. I found the reproducing steps and is 100% reproducible. I filed bug 1410698. May be you can have a look at it. Thank you very much.
I verified this issue using Nightly 58.0a1 and Firefox Beta with Build ID 20171019140425 on Mac OS 10.12 and Mac OS 10.13 and during the verification I found another issue and I filled a new bug 1410878 for this.  
I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.