Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing steps on fullscreen element leave a dangling fullscreen flag #217

Open
nt1m opened this issue Feb 28, 2023 · 1 comment
Open

Removing steps on fullscreen element leave a dangling fullscreen flag #217

nt1m opened this issue Feb 28, 2023 · 1 comment

Comments

@nt1m
Copy link
Member

nt1m commented Feb 28, 2023

If you have a fullscreen element, then move it to a different parent, it triggers the removing steps which does at step 3:

  1. If node is document’s fullscreen element, exit fullscreen document.
  2. Otherwise...
  3. If document’s top layer contains node, remove node from document’s top layer.

But in the exit fullscreen algorithm, the fullscreen flag is only removed at step 14.3. This is problematic, because that step runs in parallel, so step 3.3 of the removing steps which removes the fullscreen element from the top layer will run before.

That means we end up stopping the exit fullscreen at step 11, so we end up never removing the fullscreen flag, which is problematic if the removed element is re-appended in the document.

Chromium handles it specifically at step 7. of exit fullscreen, by invoking the "unfullscreen doc's fullscreen element" as a step 7.2.

@marcoscaceres
Copy link
Member

@foolip, do you by chance know the history of "Step 7.2" (see Chrome's implementation) and why it's no longer in the spec?

WebKit will follow Chrome's behavior as otherwise it affects YouTube, but it would be good to figure this out as, right now, the we both don't follow the spec.

webkit-early-warning-system pushed a commit to nt1m/WebKit that referenced this issue Mar 1, 2023
…ayer after transitioning from full screen

https://bugs.webkit.org/show_bug.cgi?id=253121
rdar://105713729

Reviewed by Ryosuke Niwa.

There is a bug with the fullscreen spec that leaves a dangling fullscreen flag when moving elements: whatwg/fullscreen#217
This causes fullscreen styles to unintentionally apply on the YouTube site even though the player element which has moved in the DOM tree, has exited
fullscreen.

To fix this, we follow Chromium's pattern of running an extra "unfullscreen element" step in the synchronous exit fullscreen steps when the element to
be exited is disconnected.

* LayoutTests/imported/w3c/web-platform-tests/fullscreen/model/move-fullscreen-element-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/fullscreen/model/move-fullscreen-element.html: Added.
* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::exitFullscreen):

Canonical link: https://commits.webkit.org/260985@main
aperezdc pushed a commit to WebKit/WebKit that referenced this issue Mar 16, 2023
…gi?id=253121

    REGRESSION(257542@main): Video is misaligned on YouTube site's PiP player after transitioning from full screen
    https://bugs.webkit.org/show_bug.cgi?id=253121
    rdar://105713729

    Reviewed by Ryosuke Niwa.

    There is a bug with the fullscreen spec that leaves a dangling fullscreen flag when moving elements: whatwg/fullscreen#217
    This causes fullscreen styles to unintentionally apply on the YouTube site even though the player element which has moved in the DOM tree, has exited
    fullscreen.

    To fix this, we follow Chromium's pattern of running an extra "unfullscreen element" step in the synchronous exit fullscreen steps when the element to
    be exited is disconnected.

    * LayoutTests/imported/w3c/web-platform-tests/fullscreen/model/move-fullscreen-element-expected.txt: Added.
    * LayoutTests/imported/w3c/web-platform-tests/fullscreen/model/move-fullscreen-element.html: Added.
    * Source/WebCore/dom/FullscreenManager.cpp:
    (WebCore::FullscreenManager::exitFullscreen):

    Canonical link: https://commits.webkit.org/260985@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants