[Fission] Full screen button for YouTube video embedded on Reddit doesn't work
Categories
(Core :: Widget: Cocoa, defect, P2)
Tracking
()
People
(Reporter: mccr8, Assigned: haik)
References
Details
Attachments
(1 file, 3 obsolete files)
Bug 1708324 - [Fission] Full screen button for YouTube video embedded on Reddit doesn't work r?spohl
48 bytes,
text/x-phabricator-request
|
Details | Review |
- Visit this page: https://www.reddit.com/r/movies/comments/n0e9x2/the_tomorrow_war_july_2nd_prime_video/
- Click on the video so that it starts playing.
- Click on the square in the right side of the video controls.
Expected behavior (seen in a non-Fission window): The video goes full-screen.
Actual behavior: The screen goes black, like it is transitioning into full-screen, but then I'm just back on the page like I hadn't clicked on anything.
I see the message in the browser console with Fission enabled (which I don't see in a non-Fission window):
Error: TelemetryStopwatch: key "FULLSCREEN_CHANGE_MS" was already initialized DOMFullscreenParent.jsm:136:28
handleEvent resource:///actors/DOMFullscreenParent.jsm:136
I haven't noticed issues with embedded full screen videos for a while, so I'm not sure what the difference is here. Trying to make it go full screen a bunch of times, it did work like maybe 1 out of 5 times.
Oddly enough, if I follow the same steps on this page, which also has a YouTube embed, it works just fine (including no error message): https://www.reddit.com/r/netflix/comments/n0frru/hit_run_official_teaser_netflix/
Reporter | ||
Comment 1•4 years ago
|
||
Here's another page where the full screen doesn't work if you want a more wholesome trailer to watch the first 4 seconds of over and over:
https://www.reddit.com/r/movies/comments/n0ea1j/disney_and_pixars_luca_official_trailer_disney/
Reporter | ||
Comment 2•4 years ago
|
||
Looking at the code, I'm guessing the TelemetryStopwatch error is because we got MozDOMFullscreen:Entered and MozDOMFullscreen:Exited without an intervening DOMFullscreen:Painted (because we popped in and out without actually showing anything full screen?), so maybe that's just a symptom rather than the cause of the error.
Comment 3•4 years ago
|
||
Edgar, can you please debug this embedded fullscreen issue? I was able to reproduce this using MacOS.
Updated•4 years ago
|
Comment 4•4 years ago
|
||
Edgar, this is a new Fission fullscreen bug that sounds pretty serious.
This might be macOS-specific. In the Fission bug triage meeting, people running macOS can reproduce and people Windows and Linux can't. Perhaps this is related to the new native fullscreen code for macOS?
Comment 5•4 years ago
|
||
It's also weird that when the the screen goes black, like it is transitioning into full-screen, and then it returns to the same page (non-fullscreen), it also makes the whole browser window fullscreen. So, maybe the embedded video fullscreen is somehow wrongly triggering the window fullscreen.
Comment 6•4 years ago
|
||
I can easily reproduce this on Nightly build, but the reproduce rate is not that high on my local build (even on non-debug build).
When issue happens, we run into https://searchfox.org/mozilla-central/rev/49e0a928390a8013a4eb08b7b3bbff025f01913a/browser/actors/DOMFullscreenChild.jsm#28-37 where DOMFullscreenChild exit the fullscreen right after receiving DOMFullscreen:Entered
, it seems the fullscreen state is somehow cleared, need to check why.
Comment 7•4 years ago
|
||
Yeah, I think this is macOS-specific. Only macOS updates the occlusion state for now, https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2039-2055.
When issue happens, the occlusion state is flipped back and forth, and if the occlusion state is not yet stable and the embedded get the response from it's parent to contintue the fullscreen steps which will check the active state again, then it fails at https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/dom/base/Document.cpp#14554-14557 and exit the fullscreen.
It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth. But I think we could do something like what pointerlock does currently: only check the active state on initial check.
Comment 8•4 years ago
|
||
(In reply to Edgar Chen [:edgar] from comment #7)
It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth.
Hi Haik, do you have any idea what would possible cause occlusion state filping while widget goes into fullscreen mode.
The occlusion changes is from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2909-2913.
Thanks!
Comment 9•4 years ago
|
||
On Mac OS, when wiget switch to fullscreen mode, the occlusion state might be
flipped back and forth. And checking the active state of BrowsingContext might
fail if it happends when the occlusion state isn't flipped back yet. To avoid
this, we only check if document is in active tab on initial check, like what
we do for PointerLock.
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Edgar Chen [:edgar] from comment #8)
(In reply to Edgar Chen [:edgar] from comment #7)
It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth.
Hi Haik, do you have any idea what would possible cause occlusion state filping while widget goes into fullscreen mode.
The occlusion changes is from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2909-2913.
Thanks!
Hi, I don't know what is triggering the occlusion event. I have some tracing code setup to help debug fullscreen so I will take a look at this case.
I ran into a similar problem with bug 1563947 to use the native Mac fullscreen API for DOM fullscreen support (which I'm working on now). In that case, the Mac window NSWindow instance is getting occlusion callbacks during the transition to fullscreen causing the tab-is-focused check to fail. Depending on when the occlusion events are firing, you may be able to check nsCocoaWindow::mInFullScreenTransition
and change the nsCocoaWindow callback to ignore the occlusion change during the transition. That is how I worked around the problem in my WIP code.
I thought the occlusion events were driven by macOS as a quirk during native fullscreen transition, but if it is occurring for non-native too, it could be caused by something Firefox is doing.
Assignee | ||
Comment 11•4 years ago
•
|
||
(In reply to Haik Aftandilian [:haik] from comment #10)
(In reply to Edgar Chen [:edgar] from comment #8)
(In reply to Edgar Chen [:edgar] from comment #7)
It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth.
Hi Haik, do you have any idea what would possible cause occlusion state filping while widget goes into fullscreen mode.
The occlusion changes is from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2909-2913.
Thanks!Hi, I don't know what is triggering the occlusion event. I have some tracing code setup to help debug fullscreen so I will take a look at this case.
When we transition to fullscreen we create a new window for the animation in nsCocoaWindow::PrepareForFullscreenTransition()
for the black screen effect (i.e., fill the screen with black temporarily when going in/out of fullscreen)[1]. So it follows that we would receive occlusion events because the existing window is fully occluded for some period of time.
If it makes sense to solve this at the widget layer, having Mac widget code ignore the occlusion events during that time sounds reasonable to me. And I think it's appropriate to fix there because the occlusion event is a side effect of the Mac-specific implementation.
- This might change when we transition to using native fullscreen in bug 1563947.
Comment 12•4 years ago
|
||
(In reply to Haik Aftandilian [:haik] from comment #11)
If it makes sense to solve this at the widget layer, having Mac widget code ignore the occlusion events during that time sounds reasonable to me.
Yeah, I think it makes sense to solve this at widget layer, I will prepare patch for that. Thanks for your help!
Comment 13•4 years ago
|
||
I tried to add a check for mInFullScreenTransition
in https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/widget/cocoa/nsCocoaWindow.mm#2047, something like
if (mInFullScreenTransition || mIsFullyOccluded == newOcclusionState) {
return;
}
But it doesn't work since the occlusion events happens after nsCocoaWindow::DoMakeFullScreen
call, maybe the event is asynchronous? Hi Haik, any suggestion? Or something I could try? Thanks!
Assignee | ||
Comment 14•4 years ago
|
||
WIP - ignore occlusion events during the fullscreen transition.
Assignee | ||
Comment 15•4 years ago
|
||
I added an implementation of CleanupFullscreenTransition()
in nsCocoaWindow and tested with this using PrepareForFullscreenTransition()
and CleanupFullscreenTransition()
to start/stop ignoring occlusion events. I can't reproduce the problem with the fix but it should be tested more.
Comment 16•4 years ago
|
||
Nice, after applying the patch I could not reproduce (tried over 20 times), I used to reproduce this problem very easy with https://www.reddit.com/r/movies/comments/n0ea1j/disney_and_pixars_luca_official_trailer_disney/. Thanks!!!
Haik, since your patch works, would you mind taking this bug then? :)
Updated•4 years ago
|
Assignee | ||
Comment 18•4 years ago
|
||
(In reply to Edgar Chen [:edgar] (OOO ~ 05/19) from comment #16)
Haik, since your patch works, would you mind taking this bug then? :)
No problem.
Assignee | ||
Comment 19•4 years ago
|
||
To add a bit more to Edgar's evaluation on comment 7, the reason the problem is intermittent is that Document::FullscreenElementReadyCheck()
runs in the child process racing with the parent process executing the FullscreenTransitionTask
.
In the child, first nsDOMWindowUtils::RemoteFrameFullscreenChanged()
runs which sends an async message to the parent which eventually runs nsDOMWindowUtils::RemoteFrameFullscreenChanged()
and ends up in MakeWidgetFullscreen()
which enqueues the FullscreenTransitionTask
runnable.
In the failure case, we have a tracing log like this:
CPU FUNCTION PID
6 -> nsCocoaWindow::DispatchOcclusionEvent() 2990
6 -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990 Occluded
6 <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990
6 <- nsCocoaWindow::DispatchOcclusionEvent() 2990
4 -> mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&) 3000
4 <- mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&) 3000
4 -> nsCocoaWindow::DoMakeFullScreen(bool, bool) 2990
4 <- nsCocoaWindow::DoMakeFullScreen(bool, bool) 2990
7 -> nsCocoaWindow::DispatchOcclusionEvent() 2990
7 -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990 Unoccluded
7 <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990
7 <- nsCocoaWindow::DispatchOcclusionEvent() 2990
Good case:
CPU FUNCTION PID
5 -> nsCocoaWindow::DispatchOcclusionEvent() 2990
5 -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990 Occluded
5 <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990
5 <- nsCocoaWindow::DispatchOcclusionEvent() 2990
5 -> nsCocoaWindow::DispatchOcclusionEvent() 2990
5 -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990 Unoccluded
5 <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool) 2990
5 <- nsCocoaWindow::DispatchOcclusionEvent() 2990
4 -> nsCocoaWindow::DoMakeFullScreen(bool, bool) 2990
4 <- nsCocoaWindow::DoMakeFullScreen(bool, bool) 2990
4 -> mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&) 3000
4 <- mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&) 3000
Assignee | ||
Comment 20•4 years ago
|
||
During fullscreen transitions on Mac, ignore occlusion events caused by the widget DOM fullscreen transition effect which uses a temporary window.
Add a test that attempts to enter fullscreen from a non-focused window. This test is to ensure the fix (and future fixes) do not regress the focus requirement for fullscreen.
Comment 21•3 years ago
|
||
Comment 22•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 23•3 years ago
|
||
Comment 24•3 years ago
|
||
Comment on attachment 9223893 [details]
Bug 1708324 - Exit fullscreen only when the focus change will raise the window;
Revision D116182 was moved to bug 1712038. Setting attachment 9223893 [details] to obsolete.
Description
•