1631735 - Make macOS native sizemode transitions asynchronous and uninterruptible

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

Per discussion in bug 1563947 comment 1 forward, we can probably try enable full-screen-api.macos-native-full-screen by default on Nightly to get some feedback before trying to make it better.

I've done a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2870bb48cbf41d3ceceecac037c28115ba9743a

It seems the fullscreen tests aren't very happy with this, so we need to figure out how can we fix all of them (or disable some if appreciate).

Comment 2

[Haik Aftandilian [:haik]]

Here's a try push with native fullscreen turned on - full-screen-api.macos-native-full-screen=true - without any other changes.

https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=cxjOydSJQpCiwx1d-bVnOA.0&revision=c6f3e9517c2c8929e811e46176aea9a29dfdeed1

Comment 13

[Haik Aftandilian [:haik]]

Update: I'm working on getting our tests to pass with native fullscreen enabled.

There are a couple of issues with the current implementation (which is pref’d off). With native fullscreen, we call the nsIWidgetListener callbacks FullscreenWillChange() and FullscreenChanged() from the NSWindow AppKit windowWillEnterFullScreen and windowDidEnterFullScreen callbacks respectively. This is a problem becasue windowWillEnterFullScreen gets called immediately by [NSWindow toggleFullScreen] and windowDidEnterFullScreen is called asynchronously later. This results in the JS fullscreenchange events and promise resolve happening earlier, before Window.fullScreen becomes true. This trips up some tests and is likely to cause webcompat issues so it needs to be fixed. Secondly, if JS attempts to exit fullscreen immediately after getting the fullscreenchange event, this can fail because AppKit ignores the [NSWindow toggleFullScreen] call with no errors apart from a “not in fullscreen state” message logged to the terminal.

I’m working on a fix that calls the listener callbacks FullscreenWillChange() and FullscreenChanged() at the same time like the current non-native implementation. Either they should be called when we first trigger the fullscreen transition and the native transition continues or they are not called until the native transition completes when we receive the windowDidEnterFullScreen callback. Issuing the listener callbacks before the full transition completes is what we do now, but with native it may mean that the fullscreen callbacks fire in JS before the window size reflects the fullscreen change (not confirmed.)

When a new fullscreen request comes in while we are waiting for an existing transition to complete, the fix will maintain the target transition state (fullscreen or normal) and, after a transition completes, start a new transition to the target state.

Comment 14

[Haik Aftandilian [:haik]]

Attachment: WIP: Bug 1631735 Part 0: Logging of macOS fullscreen methods.

Comment 15

[Haik Aftandilian [:haik]]

Attachment: WIP: Bug 1631735 Part 1: Improve macOS native fullscreen behavior.

Depends on D114132

Comment 16

[Haik Aftandilian [:haik]]

Attachment: WIP: Bug 1631735 - WIP - Patch 2 - Make fullscreen resize not trigger a resize event

Does not work.
Make fullscreen resize not trigger a resize event.

Depends on D114133

Comment 17

[Haik Aftandilian [:haik]]

The posted patch 2 needs some work. Explanation below.

Constraint: when transitioning to fullscreen, the window is resized, but the order of events (fullscreen and resize) should be as follows.

DOM fullscreen triggered by content:

1. Window resizes to fullscreen for the native fullscreen animation
2. Element.requestFullscreen (or Document.exitFullscreen) promise resolves and the Document.onfullscreenchange event fires and at this time the window dimensions should reflect fullscreen dimensions and the (obsolete) Document.fullscreen property should be set to true. For leaving fullscreen, the window dimensions should reflect the reverted size.
3. Window resize event fires

With the existing emulated fullscreen, we resize our window in nsCocoaWindow::DoMakeFullscreen() synchronously. In the same function we call the widget listener methods FullscreenWillChange() and FullscreenChanged().

The resize event comes after entering fullscreen because in nsGlobalWindowOuter::SetWidgetFullscreen() we call nsRefreshDriver::SetIsResizeSuppressed() which delays the resize for one refresh driver tick.

—

When entering native fullscreen the order of callbacks is as follows.

1. windowWillEnterFullScreen
2. windowDidResize
3. windowDidChangeOcclusionState (becomes fully occluded)
4. windowDidEnterFullScreen
5. windowDidChangeOcclusionState (no longer fully occluded)

For leaving native fullscreen:

1. windowWillExitFullScreen
2. windowDidResize
3. windowDidChangeOcclusionState (becomes fully occluded)
4. windowDidExitFullScreen
5. windowDidChangeOcclusionState (no longer fully occluded)

The current patch 2 attempts to make the resize in each step 2 take effect such that content is redrawn at the right position, but without delivering a resize event to content until after the fullscreen events are delivered.

The resize in each step 2 draws the window at fullscreen size for the animation.

I’m having trouble making sure the resize events get delivered to content correctly.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Haik, is patch 2 a fix that will help with issues as reported on bug 1705643?

Comment 19

[Haik Aftandilian [:haik]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #18)

Haik, is patch 2 a fix that will help with issues as reported on bug 1705643?

For now, I expect the transition will look the same as it does now with full-screen-api.macos-native-full-screen=true. Specifically, the fade to black and then back out to content will not be used.

Comment 20

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #17)

I’m having trouble making sure the resize events get delivered to content correctly.

Instead of the approach in patch 2, calling the nsIWidgetListener callbacks from the windowDidResize callback may solve this. Needs more testing.

Comment 23

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #20)

(In reply to Haik Aftandilian [:haik] from comment #17)

I’m having trouble making sure the resize events get delivered to content correctly.

Instead of the approach in patch 2, calling the nsIWidgetListener callbacks from the windowDidResize callback may solve this. Needs more testing.

This approach didn't work out well. Due to the transition being reported as done in windowDidResize, there is a period of time after this before the NSWindow is fully transitioned and during that time some operations such as starting a new FS transition on another window, trying to close the window, or hide the window don't work and deferring all those operations until after the NSWindow transition fully completes was very complicated.

The think the way forward is to (instead of the approach is patch 2) change windowDidResize to ignore the resize during a DOM FS transition and issue the resize events from windowDidEnterFullScreen instead. However, with that approach I'm running into some focus issues where after coming out of FS leaves the window unfocused that I have not tracked down the root cause of.

Comment 24

[Haik Aftandilian [:haik]]

The updated patch finishes the fullscreen transition early in the windowDidResize, but needs more work to properly handling the window being closed while the NSWindow transition is still ongoing.

https://treeherder.mozilla.org/jobs?repo=try&revision=b6b492f30b9d8671e74214a4d604558acccd93c5

Comment 26

[Stephen A Pohl [:spohl]]

The notch on the new MacBooks raises the priority of landing this.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:spohl, since the bug has high priority, high severity and recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 33

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 34

[Brad Werth [:bradwerth]]

I'll try to complete this.

Comment 35

[Brad Werth [:bradwerth]]

Attachment: WIP: Bug 1631735 Part 0: Logging of macOS fullscreen methods.

This is a rebased version of Haik's logging patch.

Comment 36

[Brad Werth [:bradwerth]]

Attachment: WIP: Bug 1631735 Part 2: Logging of macOS fullscreen methods.

This is a rebased version of Haik's logging patch D114132.

Comment 37

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 1: Improve macOS native fullscreen behavior.

This is a rebased version of Haik's patch D114133. It aims to fix the
issues that cause our macOS fullscreen tests to fail.

Depends on D160096

Comment 38

[Phabricator Automation]

Comment on attachment 9299947 [details]
WIP: Bug 1631735 Part 0: Logging of macOS fullscreen methods.

Revision D157015 was moved to bug 1790164. Setting attachment 9299947 [details] to obsolete.

Comment 39

[Brad Werth [:bradwerth]]

Attachment: WIP: Bug 1631735 Part 1: Turn macOS native fullscreen pref on for Nightly.

Comment 40

[Brad Werth [:bradwerth]]

Haik's original patch seems to work well interactively, both with and without the native fullscreen pref set, but there are still test failures. It's time to get some of this landed. The cleanest way to do this is to land this code without the pref flip, disable some of the failing tests (which seem to run fine in local testing and/or do not reflect problems with interactive fullscreen behavior), and file bugs for both these issues as follow-ups.

Comment 41

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 2: Disable some macOS fullscreen tests.

Generally, these tests either run successfully in local automated testing,
or the failures they show are not replicable in interactive testing.
Fixing these tests will be handled in follow-up Bug 1802192.

Depends on D160097

Comment 43

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 2: Remove waiting for visibilitychange events from fullscreen tests.

fullscreen tests

Since macOS fullscreen can handle incoming fullscreen requests during a
fullscreen transition, it is no longer necessary to check for
visibilitychange events, which are also no longer fired.

Depends on D160097

Comment 44

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 2: Add a fullscreen test that checks event order, and a test of fs->minimize->fs.

Depends on D164247

Comment 45

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 4: Disable tests and portions of tests that resize fullscreen windows on macOS.

Depends on D164248

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe09769d7d5a
Part 1: Improve macOS native fullscreen behavior. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4b8cc0917731
Part 2: Remove waiting for visibilitychange events from fullscreen tests. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a6d345a47718
Part 3: Add a fullscreen test that checks event order. r=mstange
https://hg.mozilla.org/integration/autoland/rev/781c1031c032
Part 4: Disable tests and portions of tests that resize fullscreen windows on macOS. r=webdriver-reviewers,whimboo

Comment 47

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/fe09769d7d5a
https://hg.mozilla.org/mozilla-central/rev/4b8cc0917731
https://hg.mozilla.org/mozilla-central/rev/a6d345a47718
https://hg.mozilla.org/mozilla-central/rev/781c1031c032

Comment 48

[Sandor Molnar[:smolnar]]

Backed out for causing bug 1806355

Backout link: https://hg.mozilla.org/integration/autoland/rev/08ef61194a1e6d5022afb99c2ead79e0ed712eb9

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sandor, with the backout of this patch we should also backout the patch from 1806262 which otherwise would cause test failures.

Comment 50

[Sandor Molnar[:smolnar]]

@whimboo, already did that. had to because it caused conflict when backing out

Comment 51

[Markus Stange [:mstange]]

When these patches land again, they should probably land in a different bug, because these patches don't change the pref default so they don't fix this bug.

Comment 52

[Sandor Molnar[:smolnar]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/08ef61194a1e6d5022afb99c2ead79e0ed712eb9

Comment 54

[Markus Stange [:mstange]]

When these patches land again, we should land them in a different bug because they don't actually flip the default and, on their own, don't fix the bugs we've been duping against this one.

Comment 55

[Brad Werth [:bradwerth]]

I'm revising this to have a more general queue of pending transitions (fullscreen, minimize, hide, etc.)

Comment 56

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 1: Make nsCocoaWindow animated transitions asynchronous and atomic.

This patch attempts to make nsCocoaWindow transitions (fullscreen,
windowed, minimize, zoom) atomic operations that can't be disrupted by
user action or by programmatic triggers. To accomplish this, it defines a
transition queue which ensures that when a transition is submitted, it is
always run to completion before another transition is started.
Additionally, it will add additional transitions in to handle programmatic
requests which aren't allowed by macOS, which include:

1. Attempting to minimize a fullscreen window will first transition to
   windowed state.
2. Any operation on a minimized window will first transition to windowed
   state.

Since some of these transitions are animated, this patch also makes clear
when the size mode event is sent for each type of transition:

Enter or exit fullscreen: beginning of animation.
Minimize or Deminimize: end of animation.
Enter or Zoom: end of transition, which is not animated.

This matches existing test expectations, with the exception of a few tests
which assume that window.minimize is synchronous. In all cases, the next
transition must wait for the previous transition (including any animation) to
complete.

Comment 57

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 2: Make tests that trigger minimize wait for the animation to complete.

Depends on D166450

Comment 58

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 3: Update test expectations to note macOS transitions are async.

Depends on D164248

Comment 59

[Brad Werth [:bradwerth]]

Per review feedback, we'll make the native sizemode transitions asynchronous (fullscreen, minimize) and the other transitions synchronous (normal/windowed, maximized/zoom).

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae84866d35d
Part 1: Make nsCocoaWindow animated transitions asynchronous and atomic. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4a7049e0f50d
Part 2: Add a fullscreen test that checks event order, and a test of fs->minimize->fs. r=mstange,emilio
https://hg.mozilla.org/integration/autoland/rev/1c7dffec620f
Part 3: Update test expectations to note macOS transitions are async. r=mstange

Comment 61

[Sandor Molnar[:smolnar]]

Backed out for causing bc assertion failures in xpfe/appshell/AppWindow.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/c0fa92102e0a51f82c04fdc3f3008cb2b5a55747

Push with failures

Failure log -> Assertion failure: mFullscreenChangeState == FullscreenChangeState::NotChanging, at /builds/worker/checkouts/gecko/xpfe/appshell/AppWindow.cpp

Comment 62

[Brad Werth [:bradwerth]]

Hmm... I had thought that the new test browser_fullscreen_from_minimize.js would cleanly fail on Windows, but since it fails with an assert, it'll need to be skipped instead.

Comment 63

[Brad Werth [:bradwerth]]

There's a second problem in Part 1 where an inserted transition that is asynchronous will not correctly break out of the ProcessTransitions function, because there's still another transition in the queue. That needs a fix.

Comment 64

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #63)

There's a second problem in Part 1 where an inserted transition that is asynchronous will not correctly break out of the ProcessTransitions function, because there's still another transition in the queue. That needs a fix.

Actually, that looks fine, but an inserted transition to windowed mode may send an unwanted sizemodechange event if the window is currently in emulated fullscreen. I'll put a patch together.

Comment 65

[Brad Werth [:bradwerth]]

Attachment: Bug 1631735 Part 4: Make tests that minimize windows wait for size mode change events.

Depends on D170841

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9995ff92253
Part 1: Make nsCocoaWindow animated transitions asynchronous and atomic. r=mstange
https://hg.mozilla.org/integration/autoland/rev/98df31ea18e2
Part 2: Add a fullscreen test that checks event order, and a test of fs->minimize->fs. r=mstange,emilio
https://hg.mozilla.org/integration/autoland/rev/8201fa6c6156
Part 3: Update test expectations to note macOS transitions are async. r=mstange
https://hg.mozilla.org/integration/autoland/rev/168ec9dca708
Part 4: Make tests that minimize windows wait for size mode change events. r=mstange

Comment 67

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c9995ff92253
https://hg.mozilla.org/mozilla-central/rev/98df31ea18e2
https://hg.mozilla.org/mozilla-central/rev/8201fa6c6156
https://hg.mozilla.org/mozilla-central/rev/168ec9dca708