1802193 - Turn on the `full-screen-api.macos-native-full-screen` pref for macOS Nightly

Status: RESOLVED FIXED

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

Description

[Brad Werth [:bradwerth]]

When tests and code are cleaned up in Bug 1802192, we should flip the pref in Nightly to reveal problems encountered with wider usage.

Comment 1

[Brad Werth [:bradwerth]]

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

Comment 4

[Brad Werth [:bradwerth]]

This needs the fixes in Bug 1826645 to be landable without disabling almost all fullscreen tests.

Comment 5

[Brad Werth [:bradwerth]]

This is finally ready to land, I think. Let's try it.

Comment 6

[Brad Werth [:bradwerth]]

Maybe will require some test fixups: browser_fullscreen_newtab.js failing at https://treeherder.mozilla.org/jobs?repo=try&revision=4b22c5afe9ded2125cb365f6a68222837cce6be2&selectedTaskRun=U4vQ2JsvR9mDsGk0VJ3q7g.0

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 2: Make a fullscreen test return window to normal at end of test.

Depends on D162890

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 3: Mark a WPT orientation lock test as failing for macOS.

On macOS this part of the test is currently passing in a trivial way by
returning the expected NotSupportedError. With async fullscreen
transitions, it now may also return a SecurityError due to the issues
noted in Bug 1830143. This other type of error is treated as a test
failure and is timing-dependent, so it means we have to mark the test as
[PASS, FAIL].

Depends on D176572

Comment 9

[Brad Werth [:bradwerth]]

There's still timeouts in test_pointerlock-api.html where part of file_targetOutOfFocus.html requests focus in response to a fulscreenchange event. At that point, the OcclusionStateChanged event hasn't yet been fired, and we presumably don't allow focus on elements in occluded windows. I'll see if I can build a simple fix for the test, rather than try to change the logic in nsFocusManager.

Comment 10

[Brad Werth [:bradwerth]]

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

At that point, the OcclusionStateChanged event hasn't yet been fired, and we presumably don't allow focus on elements in occluded windows. I'll see if I can build a simple fix for the test, rather than try to change the logic in nsFocusManager.

Rather, it appears that the focus call is working as intended, but the call to requestPointerLock is being rejected for various reasons by GetPointerLockError. I've built a patch that makes the tests resilient to an initial error when calling requestPointerLock.

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 4: Make pointerlock tests more resilient to async fullscreen transitions.

Calls to requestPointerLock will fail in GetPointerLockError for windows
that are not "active", which in part requires on their occlusion state to
be marked as visible. Asynchronous fullscreen transitions take time to
update this state, and the tests that call requestPointerLock in response
to a fullscreenchange event may sometimes happen too quickly. This change
makes these tests repeat the first call to requestPointerLock until it is
successful, then continue with the test.

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 5: Skip an already intermittent test which this change makes perma-fail.

The test browser_fullscreen_window_open.js has some kind of
timing-dependent intermittency even with synchronous fullscreen
transitions on macOS. This change to async transitions turns it into a
perma-fail. Since there are already Bugs open to address the
intermittency, this change just disables the test.

Comment 13

[Brad Werth [:bradwerth]]

There's another issue showing up in test-verify where the pointerlock test is failing a fullscreen request with FullscreenDeniedNotFocusedTab. This is happening because the window occlusion is out-of-sync when the fullscreen request arrives. The fixes in Bug 1826645 made overlapping fullscreen requests work better by ensuring that a new request is held up while processing the old one, but there's apparently still a timing window where the request is done but the OcclusionStateChanged event hasn't been sent yet. In that case, a new request will reject with FullscreenDeniedNotFocusedTab. I do not have a good solution to this. I'm going to hand-wave those intermittencies away when requesting this landing.

Truly, we still have really challenging timing issues in our tests that use fullscreen. When fullscreen is async, we have at least these problems:

1. The one mentioned earlier in this comment, that a new request may be made after the previous request is finished but before the occlusion state is updated. It will error in such a case.
2. Orientation locks will fail until the occlusion state is updated, as noted in Bug 1830143 and worked around by the test expectation changes in D176573.
3. Pointer lock requests will fail until the occlusion state is updated, as worked around by the changes in D177200.
4. Whatever is going on with browser_fullscreen_window_open.js which fails sometimes even with synchronous fullscreen transitions.

The real fix for all of this would be to make a testing-harness-only signal that is sent after the occlusion state is updated, such that fullscreen and other APIs that require updated occlusion state (orientation lock, pointer locks, possibly others) could wait on it after a fullscreen transition. That's beyond the scope of what I want to do in this Bug.

Comment 14

[Brad Werth [:bradwerth]]

Bug 1831967 may have fixed the issues with occlusion state changing during a fullscreen transition. That will simplify this patch stack and allow us to finally land it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b023b0c3f03
Part 1: Turn macOS native fullscreen pref on for Nightly. r=mac-reviewers,spohl
https://hg.mozilla.org/integration/autoland/rev/e5d64c41cf0b
Part 2: Make a fullscreen test return window to normal at end of test. r=mac-reviewers,mstange

Comment 16

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures in dom/tests/mochitest/pointerlock/test_pointerlock-api.html

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

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | dom/tests/mochitest/pointerlock/test_pointerlock-api.html | Test timed out

Comment 17

[Cosmin Sabou [:CosminS]]

Also these started from that land: https://treeherder.mozilla.org/jobs?repo=autoland&revision=6752303f266a1222f277dd00c46c3486b5d9c26e&test_paths=browser%2Fbase%2Fcontent%2Ftest%2Ffullscreen&searchStr=osx&selectedTaskRun=aYUY1RrGQVuf5pExXWjXGw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=415411692&repo=autoland

Comment 18

[Brad Werth [:bradwerth]]

The failure in the pointerlockchange event tests is due to the fact that a transition to fullscreen is failing. The NSWindowDelegate method windowDidFailToEnterFullScreen is getting called. That's really unusual and it's not well-documented what might cause this to occur. I'm working backward from our calling pattern to see if I can do something about it. One known difference about the behavior on the try server is that the window is already at fullscreen dimensions when the fullscreen transition is requested. This makes us take a different path in AppWindow::FullscreenWillChange, but I'm not sure how that could cause the OS call to fail. In local testing, setting the window to fullscreen dimensions and then transitioning to fullscreen works fine.

Comment 19

[Brad Werth [:bradwerth]]

I've discovered that the file_infiniteMovement.html subtest has some odd behavior that might be the source of the problem. That subtest triggers fullscreen, then adds mousemove listeners and calls synthesizeMouse to try to trigger those listeners. But for whatever reason, the listeners are activating immediately, before the spoofed mousemove events are sent. I'm going to fix that and see if it coincidentally prevents the interruption of the macOS native fullscreen transition. If that works, it's an indicator that our call to synthesizeMouse might be the problematic trigger. That would be good news because it's not something that could be triggered by web authors. It would be better, of course, to understand why and how it affects the fullscreen transition...

Comment 20

[Brad Werth [:bradwerth]]

The timing of the mousemove listeners did not affect or prevent the call to windowDidFailToEnterFullScreen. So now my focus is on the one confirmed difference between the try server (where the error occurs) and my local run (where it does not). On the try server, the content window is already at fullscreen size when AppWindow::FullscreenWillChange is called, which triggers a different code path. That code is comparing scaled sizes from GetSize with unscaled sizes from GetAvailScreenSize, which is likely why it's not being hit on the high DPI screen in my testing. I'll fix that code, try to match the state of the try server, and see if I can replicate the error.

Comment 21

[Brad Werth [:bradwerth]]

Fixing the size comparison in AppWindow::FullscreenWillChange better aligned the behavior between try server and local runs, but it did not prevent windowDidFailToEnterFullScreen from being called on the try server run. Wild! Still looking...

Comment 22

[Brad Werth [:bradwerth]]

I'll force a crash during windowDidFailToEnterFullScreen to get a stack from the try server run. That's one of the few things I could imagine obstructing fullscreen -- if we are in some callback or something that shouldn't be requesting fullscreen in the first place.

Comment 23

[Brad Werth [:bradwerth]]

Okay, I now understand this problem. It is a problem where two macOS windows are attempting a native fullscreen transition at the same time. Here's how it happens for this test:

1. Subtest file_escapeKey.html starts, enters native fullscreen, checks some things, and exits fullscreen.
2. The exit fullscreen transition begins.
3. Test harness starts subtest file_infiniteMovement.html, creates a new window to host it, and it attempts to enter native fullscreen.
4. The request to enter fullscreen is rejected by the OS.
5. The exit fullscreen transition ends.

Since we don't have any notification when native fullscreen transitions end, the test harness can't wait on that notification. We've side-stepped this problem for macOS as long as the same window is making the requests, but we don't handle it when multiple windows are making the requests. I'll create a new Bug to fix that and use it as a blocker for this Bug.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1d9884cb815
Part 1: Turn macOS native fullscreen pref on for Nightly. r=mac-reviewers,spohl

Comment 25

[Cristian Tuns]

Backed out for causing bc failures in browser_fullscreen_window_focus.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/base/content/test/fullscreen/browser_fullscreen_window_focus.js | Test timed out -

Comment 26

[Brad Werth [:bradwerth]]

I'm starting to understand the failure in browser_fullscreen_window_focus.js. It's something like this:

1. Test opens a new tab (Tab 1), and then that tab opens a new tab (Tab 2).
2. Tab 1 requests to enter fullscreen.
3. An actor in Tab 1 waits for a MozAfterPaint event, which it receives at the start of the fullscreen transition.
4. Tab 1 decides it has reached fullscreen (though the transition is still going), and it focuses Tab 2, which unfocuses Tab 1. A MozAfterPaint event is sent, but nobody is listening for it.
5. The change in tab focus causes an exit fullscreen to be queued.
6. The enter fullscreen transition completes, and the window immediately starts the exit fullscreen transition.
7. The actor in Tab 1 waits for a MozAfterPaint event, which has already been fired. Tab 1 is unfocused and a new event won't be sent until it's focused again.
8. The exit fullscreen transition completes.
9. The test times out waiting for the actor in Tab 1 to receive a MozAfterPaint to determine that fullscreen has been exited.

It's something like that. The fix will be one of these:

• Make Tab 1 wait for the completion of the fullscreen transition in Step 4.
• Make the actor in Tab 1 listen earlier for the MozAfterPaint from the exit fullscreen transition.
• Make the test use something other than MozAfterPaint to determine that fullscreen is complete.

Comment 27

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 2: Make a test of focus exiting fullscreen work with asynchronous fullscreen transitions.

This adds an additional action that is triggered when the fullscreen
transition event is received. That interim action is used to re-focus the
original tab. Without this change, an asynchronous fullscreen transition
will silently swallow the MozAfterPaint event that is necessary to detect
the end of fullscreen.

Before this change, with asynchronous transitions, here's the flow:

1. The test opens Tab 1 which opens Tab 2.
2. Tab 1 enters fullscreen. This transition takes awhile but signals
   success early. This is intentional because we want web content to be able
   to be fully layed out when the fullscreen transition is complete.
3. Thinking that it has reached fullscreen, the test focuses Tab 2, which
   causes a fullscreen exit transition to be queued up but not yet run.
   Then Tab 1 is unfocused and a MozAfterPaint message is sent, but nobody is
   listening for it so it has no effect. If the fullscreen transition was
   synchronous, the actions of Step 6 (below) would happen before the change
   in focus and there would be an event listener ready for the MozAfterPaint
   event.
4. The test waits for the DOMFullscreenChild to send the
   DOMFullscreen:Painted message, which it will do when it receives a
   MozAfterPaint event.
5. Enter fullscreen transition completes, exit fullscreen transition
   starts.
6. The MozDOMFullscreen:Exited event is sent to the DOMFullscreenChild,
   which starts listening for the MozAfterPaint event. This event has
   already been sent.
7. Test times out.

This change adds an additional action to take place between steps 6 and 7.
That additional action refocuses Tab 1. This makes it send another
MozAfterPaint event which the DOMFullscreenChild is ready to receive.

Comment 28

[Brad Werth [:bradwerth]]

Now working on a test failure in toolkit/components/pictureinpicture/tests/browser_fullscreen.js. I don't understand it yet, but the PiP toggle button doesn't have the expected visibility. A helper function used by the test has an existing macOS-specific setTimeout that indicates that there is some intermittency inherent in the design of the test. That setTimeout is only in place on the call to exit fullscreen, and my current theory is that the same problem exists for entering fullscreen, but hasn't been hit yet with synchronous fullscreen transitions. I'll see what I can do for a fix.

Comment 29

[Brad Werth [:bradwerth]]

By comparing logs of the try server funs (failing) against the local runs (succeeding), it looks like difference is that in the failing run, the nsRefreshDriver needs time to send the fullscreenchange event to whatever is listening for it in the PiP controls. At least, I don't see a way that the event could be swallowed by the nsRefreshDriver -- just that it might take some time to be dispatched. That may be what the timeout noted in comment 28 is giving time to provide on fullscreen exit. I'll add the same fake timeout to the enter fullscreen request.

Comment 30

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 3: Make a PiP test work with asynchronous fullscreen transitions.

This test is checking the state of PiP UI elements in the video window,
and how they change in response to fullscreen. The logic in the test waits
on the fullscreenchange events it receives from the parent process, and
the PiP UI changes depending on the fullscreenchange events it receives
from the content process. The changes in this patch ensure that the PiP UI
elements have time to receive their fullscreenchange events from the
refresh driver. This was already in place for fullscreen exits, presumably
because it was causing problems when transitioning from test-to-test. This
change adds that same pause to fullscreen enters, which gives time for the
UI elements to receive their event and get updated.

Prior to this change, this test was using the pattern of defining async
functions which await other Promises. But the last Promise in the chain
was never awaited. This changes the test to use the pattern of returning
the Promises, which works becauses the calling functions are awaiting
the results of each step in the chain.

It also adds more logging to the test.

Depends on D180884

Comment 31

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 4: Make test_fullscreen_modal.html work with asynchronous fullscreen transitions.

This ensures the fullscreen div has focus before requesting fullscreen.

Depends on D181123

Comment 32

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 5: Disable a synchronous fullscreen test on macOS.

The function supplied as the third parameter to SpecialPowers.spawn needs
to return a Promise.

Depends on D181124

Comment 33

[Brad Werth [:bradwerth]]

Attachment: Bug 1802193 Part 6: Make browser_fullscreen-tab-close-race work with asynchronous fullscreen transitions.

This ensures that the calls to SpecialPowers.spawn are always awaited. It
also changes setupFn to setupAndCompleteFn to clarify that that function
returns a promise, which needs to be setup before the click and awaited
afterwards. It also removes the "async" keyword from functions that return
Promises, but don't await them.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0932b37c9a3
Part 1: Turn macOS native fullscreen pref on for Nightly. r=mac-reviewers,spohl
https://hg.mozilla.org/integration/autoland/rev/3842e3e30ee6
Part 2: Make a test of focus exiting fullscreen work with asynchronous fullscreen transitions. r=edgar
https://hg.mozilla.org/integration/autoland/rev/3146a51a44d1
Part 3: Make a PiP test work with asynchronous fullscreen transitions. r=pip-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/0d5cd7d1f7df
Part 4: Make test_fullscreen_modal.html work with asynchronous fullscreen transitions. r=edgar
https://hg.mozilla.org/integration/autoland/rev/ef7efeb46e78
Part 5: Disable a synchronous fullscreen test on macOS. r=edgar
https://hg.mozilla.org/integration/autoland/rev/b4e5d64ec56a
Part 6: Make browser_fullscreen-tab-close-race work with asynchronous fullscreen transitions. r=edgar

Comment 35

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/b0932b37c9a3
https://hg.mozilla.org/mozilla-central/rev/3842e3e30ee6
https://hg.mozilla.org/mozilla-central/rev/3146a51a44d1
https://hg.mozilla.org/mozilla-central/rev/0d5cd7d1f7df
https://hg.mozilla.org/mozilla-central/rev/ef7efeb46e78
https://hg.mozilla.org/mozilla-central/rev/b4e5d64ec56a