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
Add support for css content-visibility: hidden #4936
Add support for css content-visibility: hidden #4936
Conversation
EWS run on previous version of this PR (hash 213fc8a) |
Note that the patch has been reduced a lot and now has only the basics and should be easier to review. The plan is to file bugs like https://bugs.webkit.org/show_bug.cgi?id=245775 for related functionality on top of this. |
213fc8a
to
8ae0db5
Compare
EWS run on previous version of this PR (hash 8ae0db5) |
8ae0db5
to
c9adf21
Compare
EWS run on previous version of this PR (hash c9adf21) |
Source/WebCore/dom/Element.h
Outdated
struct ContentVisibilityData { | ||
WTF_MAKE_STRUCT_FAST_ALLOCATED; | ||
bool isSkippedContent { false }; | ||
ContentVisibility contentVisibility; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is nothing else to put here, this doesn't really need a heap allocated struct. Those fields could just sit directly in the rare data and take less space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we will have more flags here for content-visibility: auto
to indicate if the element is relevant to the user, something like:
bool isIntersectingViewport { false };
bool subtreeHasFocus { false };
bool subtreeHasSelection { false };
See the WIP-patch in https://bugs.webkit.org/show_bug.cgi?id=236711, looks like there isn't complex function there.
I'm fine to put isSkippedContent
and contentVisibility
in ElementRareData
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Cathie, I think the idea was to get rid of the ContentVisibilityData completely, or at least not dynamically allocate it. Anyway I can fix this tomorrow morning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a new version for what I think was the initial suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failing win test has been flaky for a while so I think we can ignore it:
https://ews-build.webkit.org/#/builders/10
So this is ready for review again.
@@ -3389,7 +3389,7 @@ void RenderLayer::paintList(LayerList layerIterator, GraphicsContext& context, c | |||
if (layerIterator.begin() == layerIterator.end()) | |||
return; | |||
|
|||
if (!hasSelfPaintingLayerDescendant()) | |||
if (!hasSelfPaintingLayerDescendant() || renderer().shouldSkipContent()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it determined where these tests are needed? It seems bit random.
The spec says "...to a large extent do not update their styles at all unless explicitly requested by script" and "user agent should additionally avoid as much layout/rendering work as possible for skipped contents" yet I see only tests about skipping painting and hit testing. This won't really help performance much. Is the plan to add those things later?
The spec also says "This is similar to giving the contents display: none". In display:none case we don't actually build a render tree at all for that subtree. Is that the intention of the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll let @rwlbuis explain this, he does most work of this feature:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are added in order to comply with the specification/tests or to make things more efficient, see also the per method comments in the commit message.
A lot of the performance comes from enabling all kinds of containment when content-visibility is hidden, we did that by adjusting shouldApply*Containment methods. But note paint containment mostly works through overflow: clip, so this patch optimises more by avoiding painting of contents/children. I agree for hit testing it may not be a big win but for correctness we have to implement that (whenever the contents are skipped).
Regarding display: none, there are a lot of similarities, and I would not be surprised that its problems was the starting point for content-visibility. Basically display: none changes are heavy while content-visibility changes should be instant ideally. I think the hint was a way to help the reader think about content-visibility: hidden. In the content-visibility case I think we do want to build the render tree, to be less costly when updating the property, and to make content-visibility: auto efficient as well.
Work on top, besides the obvious content-visibility: auto work, will be in the area of editing, resize/intersection observer, top layers/dialogs, accessibility. From the top of my head, except maybe resize/intersection observer, I think it is more about correctness/spec compliance than performance.
c9adf21
to
bc9fcf4
Compare
EWS run on previous version of this PR (hash 7768000) |
7768000
to
d640921
Compare
815a829
to
ea3d9e7
Compare
EWS run on previous version of this PR (hash ea3d9e7) |
ea3d9e7
to
a11cb58
Compare
EWS run on previous version of this PR (hash a11cb58) |
a11cb58
to
1a136ee
Compare
EWS run on previous version of this PR (hash 1a136ee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better!
@@ -521,37 +524,37 @@ inline bool RenderElement::shouldApplySizeOrStyleContainment(bool containsAccord | |||
|
|||
inline bool RenderElement::shouldApplyLayoutContainment() const | |||
{ | |||
return shouldApplyLayoutOrPaintContainment(style().containsLayout()); | |||
return shouldApplyLayoutOrPaintContainment(style().containsLayout() || style().contentVisibility() != ContentVisibility::Visible); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this test could be in containsLayout() function? Same for containsPaint() etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep this as-is for now. Future work on this (forced layout/content-visibility: auto) may need to change/extend these code sections and it is not quite clear yet how.
@@ -2553,6 +2553,16 @@ String RenderObject::debugDescription() const | |||
return builder.toString(); | |||
} | |||
|
|||
bool RenderObject::isSkippedContent() const | |||
{ | |||
return style().isInSkippedContentSubtree() && parent() && parent()->style().isInSkippedContentSubtree(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is sufficient to test the parent bit only (render is skipped if the parent is skipping the content)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thanks!
@@ -550,6 +550,8 @@ class RenderStyle { | |||
|
|||
ContentVisibility contentVisibility() const { return static_cast<ContentVisibility>(m_rareNonInheritedData->contentVisibility); } | |||
|
|||
bool isInSkippedContentSubtree() const { return m_rareInheritedData->isInSkippedContentSubtree; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like effectiveSkipsContent might be less clunky and communicate that this is an effective property (similar to others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
https://bugs.webkit.org/show_bug.cgi?id=236710 Reviewed by Antti Koivisto. This patch implements support for content-visibility: hidden [1]. It introduces the fake inherited property effectiveSkipsContent to keep track for Elements whether content-visibility is hidden and whether elements should be skipped for painting and hit testing. Several points in 4.3. Restrictions and Clarifications [2] are addressed too: - scrollIntoView is adapted [4.3.5] - tab-order navigation and focusing take skipped contents into account [3]. [1] https://drafts.csswg.org/css-contain/#using-cv-hidden [2] https://drafts.csswg.org/css-contain/#cv-notes [3] https://drafts.csswg.org/css-contain/#valdef-content-visibility-hidden * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-015-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-016-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-017-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-018-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-029-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-035-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-038-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-047-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-072-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-img-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-input-image-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-svg-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/contain-intrinsic-size/auto-006-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/contain-intrinsic-size/auto-009-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/contain-intrinsic-size/auto-010-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-focus-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-embed-element/embed-document-under-content-visibility-gbcr-expected.txt: * Source/WebCore/dom/Element.cpp: (WebCore::Element::isFocusable const): skipped contents are not focusable * Source/WebCore/page/FrameView.cpp: (WebCore::FrameView::scrollRectToVisible): do not scroll if the RenderObject is skipped content * Source/WebCore/rendering/RenderBlock.cpp: (WebCore::RenderBlock::paintContents): do not paint contents for c-v: hidden * Source/WebCore/rendering/RenderElement.h: (WebCore::RenderElement::visibleToHitTesting const): mark skipped content as not visible to hit testing (WebCore::RenderElement::shouldApplyLayoutContainment const): c-v: hidden forces containment (WebCore::RenderElement::shouldApplyPaintContainment const): c-v: hidden forces containment (WebCore::RenderElement::shouldApplyLayoutOrPaintContainment const): c-v: hidden forces containment (WebCore::RenderElement::shouldApplySizeContainment const): c-v: hidden forces containment (WebCore::RenderElement::shouldApplyInlineSizeContainment const): c-v: hidden forces containment (WebCore::RenderElement::shouldApplySizeOrInlineSizeContainment const): c-v: hidden forces containment (WebCore::RenderElement::shouldApplyStyleContainment const): c-v: hidden forces containment * Source/WebCore/rendering/RenderLayer.cpp: (WebCore::RenderLayer::paintList): do not paint child layers of a c-v: hidden container * Source/WebCore/rendering/RenderLayerBacking.cpp: (WebCore::RenderLayerBacking::updateAfterDescendants): * Source/WebCore/rendering/RenderObject.cpp: (WebCore::RenderObject::isSkippedContent const): (WebCore::RenderObject::shouldSkipContent const): * Source/WebCore/rendering/RenderObject.h: * Source/WebCore/rendering/RenderReplaced.cpp: (WebCore::RenderReplaced::paint): do not paint contents for content-visibility: hidden * Source/WebCore/rendering/RenderWidget.cpp: (WebCore::RenderWidget::paint): do not paint contents for content-visibility: hidden * Source/WebCore/rendering/style/RenderStyle.h: (WebCore::RenderStyle::effectiveSkipsContent const): (WebCore::RenderStyle::setEffectiveSkipsContent): * Source/WebCore/rendering/style/StyleRareInheritedData.cpp: (WebCore::StyleRareInheritedData::StyleRareInheritedData): (WebCore::StyleRareInheritedData::operator== const): * Source/WebCore/rendering/style/StyleRareInheritedData.h: * Source/WebCore/style/StyleAdjuster.cpp: (WebCore::Style::Adjuster::adjust const):
1a136ee
to
faf9ff8
Compare
EWS run on current version of this PR (hash faf9ff8) |
This system gets confused about multiple people working on a PR, and git-webkit was not able to make me owner of this PR. So I will use good old bugzilla. |
This landed as r256186. |
faf9ff8
faf9ff8
π§ͺ api-iosπ§ͺ api-macπ§ͺ mac-wk1π§ͺ mac-wk2π π§ͺ mergeπ§ͺ mac-AS-debug-wk2