Bug 236710 – [content-visibility] Add support for css content-visibility: hidden

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[cathiechen]

https://www.w3.org/TR/css-contain-2/#using-cv-hidden

[Radar WebKit Bug Importer]

<rdar://problem/89360301>

[Rob Buis]

Created attachment 453398 [details]
Initial patch

Initial patch

[Rob Buis]

Created attachment 453495 [details]
Improved patch

This patch keeps track of c-v state even when there is no renderer created (display: none).

[Rob Buis]

Created attachment 453602 [details]
Better ChangeLog.

Better ChangeLog.

[Rob Buis]

Created attachment 453619 [details]
Fix some more test expectations

Fix some more test expectations

[Rob Buis]

Created attachment 454326 [details]
Build fix

[Rob Buis]

Created attachment 454820 [details]
Improve top layers

Improve top layers

[Rob Buis]

For some of the top level layer issues we probably need resolution here first:
https://github.com/w3c/csswg-drafts/issues/7163

[Rob Buis]

Created attachment 455644 [details]
Improve top layer support

Improve top layer support

[Emilio Cobos Álvarez (:emilio)]

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?

[Rob Buis]

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.

[Rob Buis]

Created attachment 456123 [details]
Rebase and adjust containsLayout etc.

Rebase and adjust containsLayout etc.

[Rob Buis]

Created attachment 457988 [details]
Rebase

Rebase.

[Rob Buis]

Created attachment 458124 [details]
Drop layoutPositionedObject change

Drop layoutPositionedObject change

[Rob Buis]

Created attachment 458435 [details]
Rebase

Rebase

[Rob Buis]

Created attachment 458860 [details]
Rebase

Rebase

[cathiechen]

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`.

[cathiechen]

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`

[Tim Nguyen (:ntim)]

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?

[Rob Buis]

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.

[Tim Nguyen (:ntim)]

(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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 459160 [details]
Fix r293943 regression

Fix r293943 regression.

[Rob Buis]

Created attachment 459167 [details]
Fix ChangeLog and Tim's comment

[Rob Buis]

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.

[Rob Buis]

Created attachment 459532 [details]
Remove ChangeLogs

Remove ChangeLogs.

[Rob Buis]

Created attachment 459541 [details]
New approach to top layers

New approach to top layers.

[Rob Buis]

Created attachment 459592 [details]
Fix content-visibility-with-top-layer-003.tentative.html

[Rob Buis]

Created attachment 459780 [details]
Adapt to https://bugs.webkit.org/show_bug.cgi?id=240906

[Rob Buis]

Created attachment 460086 [details]
Rebase

[Rob Buis]

Created attachment 461558 [details]
Patch

[Rob Buis]

Created attachment 461856 [details]
Patch

[Rob Buis]

Created attachment 462007 [details]
Patch

[Rob Buis]

Created attachment 462026 [details]
Patch

[Rob Buis]

Created attachment 462027 [details]
Patch

[Rob Buis]

Created attachment 462095 [details]
Patch

[Rob Buis]

Created attachment 462676 [details]
Patch

[Rob Buis]

Created attachment 462677 [details]
Patch

[Rob Buis]

Created attachment 462681 [details]
Patch

[Rob Buis]

Created attachment 462683 [details]
Patch

[Rob Buis]

Created attachment 462691 [details]
Patch

[cathiechen]

Pull request: https://github.com/WebKit/WebKit/pull/4936

[Rob Buis]

Created attachment 463339 [details]
Patch

[EWS]

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].