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

Make fullscreen API use the top layer #5467

Merged
merged 1 commit into from Dec 7, 2022

Conversation

nt1m
Copy link
Member

@nt1m nt1m commented Oct 17, 2022

c96dd4f

Make fullscreen API use the top layer
https://bugs.webkit.org/show_bug.cgi?id=84798
rdar://11313423

Reviewed by Darin Adler.

The fullscreen API should use the top layer instead of the fullscreen stack. Some notes:
- Pushing to the top layer causes visual changes, so we need to update the DOM methods to follow spec order to prevent unintended effects.
Visual changes can't happen before `willEnterFullscreen` and can't happen before `didExitFullscreen` when fully entering/exiting fullscreen.

- Since top layer handles visual z-ordering, we can remove old style hacks of clobbering the ancestor chain with special styling, and the associated
containsFullscreenElement node flag.

Spec: https://fullscreen.spec.whatwg.org

* LayoutTests/fullscreen/full-screen-enter-while-exiting-expected.txt:
Events are now scheduled and dispatched in didExitFullscreen instead of exitFullscreen(), hence the order change.

* Source/WebCore/css/CSSSelector.cpp:
(WebCore::CSSSelector::selectorText const):
* Source/WebCore/css/CSSSelector.h:
* Source/WebCore/css/SelectorPseudoClassAndCompatibilityElementMap.in:
* Source/WebCore/cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::addPseudoClassType):
Remove temporary backdrop workaround that was landed in https://commits.webkit.org/256226@main .

* Source/WebCore/css/SelectorChecker.cpp:
(WebCore::SelectorChecker::checkOne const):
* Source/WebCore/css/SelectorCheckerTestFunctions.h:
(WebCore::matchesFullScreenPseudoClass):
(WebCore::matchesFullScreenAncestorPseudoClass):
Rewrite selector checker functions to account for removal of containsFullscreenElement flag.
Since :-webkit-fullscreen-ancestor main usage was for the UA stylesheet, we may be able to remove it in a followup clean up.

(WebCore::matchesFullScreenParentPseudoClass): Deleted. (part of the backdrop workaround)
* Source/WebCore/css/fullscreen.css:
(#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API):
(:root:-webkit-full-screen-document:not(:-webkit-full-screen)): Only keep overflow: hidden; since top layer does not make the root element unscrollable.
(iframe:-webkit-full-screen): Match the spec.
(:not(:root):-webkit-full-screen::backdrop): Match the spec by making the fullscreen backdrops black.
(:-webkit-full-screen): Deleted. (not needed with top layer)
(:root:-webkit-full-screen-document:not(:-webkit-full-screen), :root:-webkit-full-screen-ancestor): Deleted.
(:-webkit-full-screen-ancestor:not(iframe)): Deleted. (not needed with top layer)
(video:-webkit-full-screen, audio:-webkit-full-screen): Deleted. (to match the spec)
(:-webkit-full-screen-parent::before): Deleted. (fake backdrop workaround)
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::nodeChildrenWillBeRemoved):
(WebCore::Document::nodeWillBeRemoved):
Move element removal handling to `Element::removedFromAncestor()`

* Source/WebCore/dom/Element.cpp:
(WebCore::Element::insertedIntoAncestor):
(WebCore::Element::removedFromAncestor):
(WebCore::Element::setFullscreenFlag):
(WebCore::Element::setIframeFullscreenFlag):
(WebCore::parentCrossingFrameBoundaries): Deleted.
(WebCore::Element::setContainsFullScreenElement): Deleted.
(WebCore::Element::setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries): Deleted.
Removed the containsFullscreenElement flag and replace it with fullscreen flag and iframe fullscreen flag.

* Source/WebCore/dom/Element.h: Ditto
(WebCore::Element::hasFullscreenFlag const):
(WebCore::Element::hasIframeFullscreenFlag const):
(WebCore::Element::containsFullScreenElement const): Deleted.

* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::fullscreenElement const): Rewrite it to iterate through top layer instead of fullscreen stack.
(WebCore::FullscreenManager::requestFullscreenForElement): Removed all fullscreen stack handling. Top layer handling is now in `willEnterFullscreen` since it performs visual changes.
(WebCore::FullscreenManager::cancelFullscreen): Basically exitFullscreen() but always exits everything.
(WebCore::documentsToUnfullscreen): "collecting documents to unfullscreen" algorithm from spec
(WebCore::FullscreenManager::exitFullscreen): Steps 1 - 8 of exit fullscreen algorithm from spec
(WebCore::FullscreenManager::finishExitFullscreen): Steps 12 - 15 of exit fullscreen algorithm from spec
(WebCore::FullscreenManager::willEnterFullscreen): Push documents to top layer, add fullscreen flag & emit events (Step 12 of request fullscreen in spec)
(WebCore::FullscreenManager::didExitFullscreen): Everything else that is needed when fully exiting fullscreen.
(WebCore::FullscreenManager::exitRemovedFullscreenElementIfNeeded): "removing steps" for element from spec (except for top layer removal which is already done later on in Element::removedFromAncestor)
(WebCore::FullscreenManager::isSimpleFullscreenDocument const): "Simple fullscreen document" as defined by the spec, e.g. a document with only one fullscreen element in the top layer.

(WebCore::FullscreenManager::adjustFullscreenElementOnNodeRemoval): Deleted. Replaced by exitRemovedFullscreenElementIfNeeded called from Element::removedFromAncestor().

(WebCore::FullscreenManager::clearFullscreenElementStack): Deleted.
(WebCore::FullscreenManager::popFullscreenElementStack): Deleted.
(WebCore::FullscreenManager::pushFullscreenElementStack): Deleted.
(WebCore::FullscreenManager::clear):
Remove fullscreen stack remainders.

* Source/WebCore/dom/FullscreenManager.h:

* Source/WebCore/dom/Node.h:
Add IsFullscreen/IsIframeFullscreen flags, remove ContainsFullScreenElement.
Shuffle flags around, since Bit 31 is actually not usable (causes integer overflow when using it).

Canonical link: https://commits.webkit.org/257456@main

2dad342

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim   πŸ›  mac-AS-debug   πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios   πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ merge   πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch-sim

@nt1m nt1m self-assigned this Oct 17, 2022
@nt1m nt1m added 528+ (Nightly build) WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). labels Oct 17, 2022
@nt1m nt1m marked this pull request as draft October 17, 2022 23:41
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 18, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 8d536df to 989d542 Compare October 19, 2022 06:01
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 989d542 to 5905241 Compare October 19, 2022 17:49
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 5905241 to 8369b20 Compare October 19, 2022 20:02
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 8369b20 to 7a43e9e Compare October 19, 2022 20:34
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 19, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Nov 20, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 7a43e9e to b5962fe Compare November 20, 2022 19:34
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 20, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from b5962fe to 820d754 Compare November 29, 2022 03:28
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from fcf9e0b to 5ca9013 Compare December 3, 2022 03:38
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 5ca9013 to 158ab55 Compare December 3, 2022 03:44
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 158ab55. Live statuses available at the PR page, #5467

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 3, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Dec 5, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 158ab55 to 2ca80c5 Compare December 5, 2022 04:03
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 5, 2022
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Dec 5, 2022
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 2ca80c5 to 302d488 Compare December 5, 2022 22:23
@nt1m nt1m requested a review from darinadler December 6, 2022 16:22
Copy link
Member

@darinadler darinadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I’m still not sure we have enough testing of this, we’d want tests that fail if any one line of this code was wrong. Regardless, r=me

Source/WebCore/css/SelectorCheckerTestFunctions.h Outdated Show resolved Hide resolved
Source/WebCore/css/SelectorCheckerTestFunctions.h Outdated Show resolved Hide resolved
Source/WebCore/dom/Element.cpp Outdated Show resolved Hide resolved
Source/WebCore/dom/Node.h Outdated Show resolved Hide resolved
Source/WebCore/dom/Node.h Show resolved Hide resolved
Source/WebCore/dom/FullscreenManager.cpp Outdated Show resolved Hide resolved
Source/WebCore/dom/FullscreenManager.cpp Outdated Show resolved Hide resolved
Source/WebCore/dom/FullscreenManager.cpp Outdated Show resolved Hide resolved
Source/WebCore/dom/FullscreenManager.cpp Outdated Show resolved Hide resolved
Source/WebCore/dom/FullscreenManager.cpp Outdated Show resolved Hide resolved
@nt1m nt1m force-pushed the fullscreen-top-layer-simple branch from 302d488 to 2dad342 Compare December 7, 2022 04:07
@nt1m nt1m added the merge-queue Applied to send a pull request to merge-queue label Dec 7, 2022
@nt1m
Copy link
Member Author

nt1m commented Dec 7, 2022

I’m still not sure we have enough testing of this, we’d want tests that fail if any one line of this code was wrong.

It took me a while to get the whole fullscreen/ directory passing :) I also had failures in media/video-* as I was working on this.

@nt1m
Copy link
Member Author

nt1m commented Dec 7, 2022

Thanks for the review!

https://bugs.webkit.org/show_bug.cgi?id=84798
rdar://11313423

Reviewed by Darin Adler.

The fullscreen API should use the top layer instead of the fullscreen stack. Some notes:
- Pushing to the top layer causes visual changes, so we need to update the DOM methods to follow spec order to prevent unintended effects.
Visual changes can't happen before `willEnterFullscreen` and can't happen before `didExitFullscreen` when fully entering/exiting fullscreen.

- Since top layer handles visual z-ordering, we can remove old style hacks of clobbering the ancestor chain with special styling, and the associated
containsFullscreenElement node flag.

Spec: https://fullscreen.spec.whatwg.org

* LayoutTests/fullscreen/full-screen-enter-while-exiting-expected.txt:
Events are now scheduled and dispatched in didExitFullscreen instead of exitFullscreen(), hence the order change.

* Source/WebCore/css/CSSSelector.cpp:
(WebCore::CSSSelector::selectorText const):
* Source/WebCore/css/CSSSelector.h:
* Source/WebCore/css/SelectorPseudoClassAndCompatibilityElementMap.in:
* Source/WebCore/cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::addPseudoClassType):
Remove temporary backdrop workaround that was landed in https://commits.webkit.org/256226@main .

* Source/WebCore/css/SelectorChecker.cpp:
(WebCore::SelectorChecker::checkOne const):
* Source/WebCore/css/SelectorCheckerTestFunctions.h:
(WebCore::matchesFullScreenPseudoClass):
(WebCore::matchesFullScreenAncestorPseudoClass):
Rewrite selector checker functions to account for removal of containsFullscreenElement flag.
Since :-webkit-fullscreen-ancestor main usage was for the UA stylesheet, we may be able to remove it in a followup clean up.

(WebCore::matchesFullScreenParentPseudoClass): Deleted. (part of the backdrop workaround)
* Source/WebCore/css/fullscreen.css:
(#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API):
(:root:-webkit-full-screen-document:not(:-webkit-full-screen)): Only keep overflow: hidden; since top layer does not make the root element unscrollable.
(iframe:-webkit-full-screen): Match the spec.
(:not(:root):-webkit-full-screen::backdrop): Match the spec by making the fullscreen backdrops black.
(:-webkit-full-screen): Deleted. (not needed with top layer)
(:root:-webkit-full-screen-document:not(:-webkit-full-screen), :root:-webkit-full-screen-ancestor): Deleted.
(:-webkit-full-screen-ancestor:not(iframe)): Deleted. (not needed with top layer)
(video:-webkit-full-screen, audio:-webkit-full-screen): Deleted. (to match the spec)
(:-webkit-full-screen-parent::before): Deleted. (fake backdrop workaround)
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::nodeChildrenWillBeRemoved):
(WebCore::Document::nodeWillBeRemoved):
Move element removal handling to `Element::removedFromAncestor()`

* Source/WebCore/dom/Element.cpp:
(WebCore::Element::insertedIntoAncestor):
(WebCore::Element::removedFromAncestor):
(WebCore::Element::setFullscreenFlag):
(WebCore::Element::setIframeFullscreenFlag):
(WebCore::parentCrossingFrameBoundaries): Deleted.
(WebCore::Element::setContainsFullScreenElement): Deleted.
(WebCore::Element::setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries): Deleted.
Removed the containsFullscreenElement flag and replace it with fullscreen flag and iframe fullscreen flag.

* Source/WebCore/dom/Element.h: Ditto
(WebCore::Element::hasFullscreenFlag const):
(WebCore::Element::hasIframeFullscreenFlag const):
(WebCore::Element::containsFullScreenElement const): Deleted.

* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::fullscreenElement const): Rewrite it to iterate through top layer instead of fullscreen stack.
(WebCore::FullscreenManager::requestFullscreenForElement): Removed all fullscreen stack handling. Top layer handling is now in `willEnterFullscreen` since it performs visual changes.
(WebCore::FullscreenManager::cancelFullscreen): Basically exitFullscreen() but always exits everything.
(WebCore::documentsToUnfullscreen): "collecting documents to unfullscreen" algorithm from spec
(WebCore::FullscreenManager::exitFullscreen): Steps 1 - 8 of exit fullscreen algorithm from spec
(WebCore::FullscreenManager::finishExitFullscreen): Steps 12 - 15 of exit fullscreen algorithm from spec
(WebCore::FullscreenManager::willEnterFullscreen): Push documents to top layer, add fullscreen flag & emit events (Step 12 of request fullscreen in spec)
(WebCore::FullscreenManager::didExitFullscreen): Everything else that is needed when fully exiting fullscreen.
(WebCore::FullscreenManager::exitRemovedFullscreenElementIfNeeded): "removing steps" for element from spec (except for top layer removal which is already done later on in Element::removedFromAncestor)
(WebCore::FullscreenManager::isSimpleFullscreenDocument const): "Simple fullscreen document" as defined by the spec, e.g. a document with only one fullscreen element in the top layer.

(WebCore::FullscreenManager::adjustFullscreenElementOnNodeRemoval): Deleted. Replaced by exitRemovedFullscreenElementIfNeeded called from Element::removedFromAncestor().

(WebCore::FullscreenManager::clearFullscreenElementStack): Deleted.
(WebCore::FullscreenManager::popFullscreenElementStack): Deleted.
(WebCore::FullscreenManager::pushFullscreenElementStack): Deleted.
(WebCore::FullscreenManager::clear):
Remove fullscreen stack remainders.

* Source/WebCore/dom/FullscreenManager.h:

* Source/WebCore/dom/Node.h:
Add IsFullscreen/IsIframeFullscreen flags, remove ContainsFullScreenElement.
Shuffle flags around, since Bit 31 is actually not usable (causes integer overflow when using it).

Canonical link: https://commits.webkit.org/257456@main
@webkit-commit-queue
Copy link
Collaborator

Committed 257456@main (c96dd4f): https://commits.webkit.org/257456@main

Reviewed commits have been landed. Closing PR #5467 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 7, 2022
@webkit-commit-queue webkit-commit-queue merged commit c96dd4f into WebKit:main Dec 7, 2022
@nt1m nt1m deleted the fullscreen-top-layer-simple branch December 7, 2022 05:24
webkit-commit-queue pushed a commit to nt1m/WebKit that referenced this pull request Dec 22, 2023
https://bugs.webkit.org/show_bug.cgi?id=266820
rdar://120052751

Reviewed by Ryosuke Niwa.

Remove this code to match the spec closer: https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen

It was originally to address a review comment:
WebKit#5467 (comment)

But this review comment no longer applies now that `Element::setFullscreenFlag` only sets a single flag.

* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::clearFullscreenFlags): rename variable to be more consistent with other iframe downcasts in the file.
(WebCore::FullscreenManager::willEnterFullscreen):

Canonical link: https://commits.webkit.org/272462@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
7 participants