https://www.w3.org/TR/css-contain-2/#using-cv-hidden
<rdar://problem/89360301>
Created attachment 453398 [details] Initial patch Initial patch
Created attachment 453495 [details] Improved patch This patch keeps track of c-v state even when there is no renderer created (display: none).
Created attachment 453602 [details] Better ChangeLog. Better ChangeLog.
Created attachment 453619 [details] Fix some more test expectations Fix some more test expectations
Created attachment 454326 [details] Build fix
Created attachment 454820 [details] Improve top layers Improve top layers
For some of the top level layer issues we probably need resolution here first: https://github.com/w3c/csswg-drafts/issues/7163
Created attachment 455644 [details] Improve top layer support Improve top layer support
Comment on attachment 455644 [details] Improve top layer support View in context: https://bugs.webkit.org/attachment.cgi?id=455644&action=review > Source/WebCore/dom/Element.cpp:4806 > + child.updateContentVisibilityData(shouldSkip, contentVisibility); How do you update children on new DOM mutations? Also, shouldn't this use the composed / flattened tree?
Comment on attachment 455644 [details] Improve top layer support View in context: https://bugs.webkit.org/attachment.cgi?id=455644&action=review >> Source/WebCore/dom/Element.cpp:4806 >> + child.updateContentVisibilityData(shouldSkip, contentVisibility); > > How do you update children on new DOM mutations? Also, shouldn't this use the composed / flattened tree? What kind of new DOM mutations do you mean? I did some local tests (create whole c-v tree dynamically, just add a text node or element to the c-v subtree) and things seem to work with c-v: hidden. It is possible the state will not be updated (immediately) in such cases, but the painting will stop at a higher level so it should not cause rendering issues. Having said that, feel free to share some tricky test scenarios :) I am sure more WPT tests here are welcome.
Created attachment 456123 [details] Rebase and adjust containsLayout etc. Rebase and adjust containsLayout etc.
Created attachment 457988 [details] Rebase Rebase.
Created attachment 458124 [details] Drop layoutPositionedObject change Drop layoutPositionedObject change
Created attachment 458435 [details] Rebase Rebase
Created attachment 458860 [details] Rebase Rebase
Comment on attachment 458860 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=458860&action=review > Source/WebCore/dom/Element.cpp:3583 > + return false; It looks like we can remove this one, for `!isFocusableStyle(style)` already be true if `isSkippedContent() == ture`.
Comment on attachment 458860 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=458860&action=review > Source/WebCore/dom/Element.cpp:4780 > + shouldSkip |= parent->isSkippedContent(); Looks like we can move this out of the function, and call `updateContentVisibilityData(parent->isSkippedContent(), newContentVisibility)` instead of `updateContentVisibilityData(false, newContentVisibility)` in `Element::updateContentVisibility`
Comment on attachment 458860 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=458860&action=review > LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-with-top-layer-003.tentative.html:14 > +.hidden { ontent-visibility: hidden } What is this change about? > Source/WebCore/dom/Element.cpp:3405 > + if (isSkippedContent()) > + return; This is kinda surprising to me, what bit of the spec says this? > Source/WebCore/dom/Element.cpp:4820 > +void Element::updateDescendantTopLayerElements(ContentVisibility newContentVisibility) > +{ > + auto topLayers = document().topLayerElements(); > + for (auto topLayerElement : topLayers) { > + for (auto* ancestor = topLayerElement.ptr(); ancestor; ancestor = ancestor->parentElementInComposedTree()) { > + if (ancestor == this) { > + if (newContentVisibility == ContentVisibility::Visible) { > + if (!topLayerElement->isInTopLayer()) > + topLayerElement->addToTopLayer(); > + } else if (newContentVisibility == ContentVisibility::Hidden) { > + if (topLayerElement->isInTopLayer()) > + topLayerElement->removeFromTopLayer(); > + } > + break; > + } > + } > + } > +} Not sure I'm understanding how this works? Is this trying to add/remove layers based on the node flag? I'm confused why the DOM side of the top layer logic should be touched here. Isn't this a layout feature?
Comment on attachment 458860 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=458860&action=review >> LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-with-top-layer-003.tentative.html:14 >> +.hidden { ontent-visibility: hidden } > > What is this change about? I wanted to try something local at the time, but clearly this change is unwanted indeed. Thanks for pointing that one out! Note that the patch is not up for review for a reason though :) >> Source/WebCore/dom/Element.cpp:3405 >> + return; > > This is kinda surprising to me, what bit of the spec says this? I think this is https://drafts.csswg.org/css-contain/#content-visibility 4.3.11. >> Source/WebCore/dom/Element.cpp:4820 >> +} > > Not sure I'm understanding how this works? Is this trying to add/remove layers based on the node flag? > > I'm confused why the DOM side of the top layer logic should be touched here. Isn't this a layout feature? I am back to work on this tomorrow, will have a look then, this code is from quite some time ago.
(In reply to Rob Buis from comment #20) > >> Source/WebCore/dom/Element.cpp:3405 > >> + return; > > > > This is kinda surprising to me, what bit of the spec says this? > > I think this is https://drafts.csswg.org/css-contain/#content-visibility > 4.3.11. > > >> Source/WebCore/dom/Element.cpp:4820 > >> +} > > > > Not sure I'm understanding how this works? Is this trying to add/remove layers based on the node flag? > > > > I'm confused why the DOM side of the top layer logic should be touched here. Isn't this a layout feature? > > I am back to work on this tomorrow, will have a look then, this code is from > quite some time ago. Looking at step 4.3.11, we shouldn't touch the DOM side of things at all for the top layer. Doing this will cause the top layer z-order to change (e.g. when stacking multiple dialogs). I think you probably want more fine-grained code targeted towards the renderer/layers parts.
Comment on attachment 458860 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=458860&action=review >> Source/WebCore/dom/Element.cpp:3583 >> + return false; > > It looks like we can remove this one, for `!isFocusableStyle(style)` already be true if `isSkippedContent() == ture`. Correct, will remove.
Created attachment 459160 [details] Fix r293943 regression Fix r293943 regression.
Created attachment 459167 [details] Fix ChangeLog and Tim's comment
Comment on attachment 458860 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=458860&action=review >>> LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-with-top-layer-003.tentative.html:14 >>> +.hidden { ontent-visibility: hidden } >> >> What is this change about? > > I wanted to try something local at the time, but clearly this change is unwanted indeed. Thanks for pointing that one out! Note that the patch is not up for review for a reason though :) I now removed the change, unfortunately we do not pass this test yet.
Created attachment 459532 [details] Remove ChangeLogs Remove ChangeLogs.
Created attachment 459541 [details] New approach to top layers New approach to top layers.
Created attachment 459592 [details] Fix content-visibility-with-top-layer-003.tentative.html
Created attachment 459780 [details] Adapt to https://bugs.webkit.org/show_bug.cgi?id=240906
Created attachment 460086 [details] Rebase
Created attachment 461558 [details] Patch
Created attachment 461856 [details] Patch
Created attachment 462007 [details] Patch
Created attachment 462026 [details] Patch
Created attachment 462027 [details] Patch
Created attachment 462095 [details] Patch
Created attachment 462676 [details] Patch
Created attachment 462677 [details] Patch
Created attachment 462681 [details] Patch
Created attachment 462683 [details] Patch
Created attachment 462691 [details] Patch
Pull request: https://github.com/WebKit/WebKit/pull/4936
Created attachment 463339 [details] Patch
Committed 256186@main (34f748c71e8d): <https://commits.webkit.org/256186@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 463339 [details].