[content-visibility] Support content-visibility for resize/intersection observer #6973
Conversation
|
EWS run on previous version of this PR (hash 2d844de)
|
rwlbuis
requested review from
darinadler
and removed request for
rniwa and
cdumez
Source/WebCore/dom/Document.cpp
Outdated
| if (targetRenderer->isSkippedContent()) | ||
| intersectionState.isIntersecting = false; | ||
| else | ||
| intersectionState.isIntersecting = rootLocalTargetRect && rootLocalIntersectionRect.edgeInclusiveIntersect(*rootLocalTargetRect); |
There was a problem hiding this comment.
I might have just added to the boolean expression.
| @@ -52,24 +52,26 @@ void ResizeObservation::updateObservationSize(const BoxSizes& boxSizes) | |||
| m_lastObservationSizes = boxSizes; | |||
| } | |||
|
|
|||
| auto ResizeObservation::computeObservedSizes() const -> BoxSizes | |||
| std::optional<ResizeObservation::BoxSizes> ResizeObservation::computeObservedSizes() const | |||
There was a problem hiding this comment.
You can write this for greater brevity:
auto ResizeObservation::computeObservedSizes() const -> std::optional<BoxSizes>
I suppose it’s a matter of taste which one is better.
There was a problem hiding this comment.
I do like that notation better.
| { | ||
| if (m_target->isSVGElement()) { | ||
| if (auto svgRect = downcast<SVGElement>(*m_target).getBoundingBox()) { | ||
| auto size = LayoutSize(svgRect->width(), svgRect->height()); | ||
| return { size, size, size }; | ||
| return BoxSizes { size, size, size }; |
There was a problem hiding this comment.
Another alternative is:
return { { size, size, size } };
| return { | ||
| if (box->isSkippedContent()) | ||
| return std::nullopt; | ||
| return BoxSizes { |
There was a problem hiding this comment.
Can use the two sets of braces here.
| } | ||
| } | ||
| auto* box = m_target->renderBox(); | ||
| if (box) { | ||
| return { | ||
| if (box->isSkippedContent()) | ||
| return std::nullopt; |
There was a problem hiding this comment.
Antti told me elsewhere that we use { } in cases like this. Of course, here it is subtle and risky to do that!
There was a problem hiding this comment.
Right, I kept it as-is.
| adjustLayoutSizeForAbsoluteZoom(box->contentSize(), *box), | ||
| adjustLayoutSizeForAbsoluteZoom(box->contentLogicalSize(), *box), | ||
| adjustLayoutSizeForAbsoluteZoom(box->borderBoxLogicalSize(), *box) | ||
| }; | ||
| } | ||
|
|
||
| return { }; | ||
| return BoxSizes { }; |
There was a problem hiding this comment.
Probably could even use the two sets of braces here, but probably not better!
| if (!currentSizes) | ||
| return currentSizes; |
There was a problem hiding this comment.
I think it’s better to return { } or std::nullopt rather than currentSizes here.
There was a problem hiding this comment.
I like std::nullopt best to make it clear what is happening.
| if (m_lastObservationSizes.borderBoxLogicalSize != currentSizes.borderBoxLogicalSize) | ||
| return currentSizes; | ||
| if (m_lastObservationSizes.borderBoxLogicalSize != currentSizes->borderBoxLogicalSize) | ||
| return *currentSizes; |
There was a problem hiding this comment.
The change on this line isn’t needed.
| if (m_lastObservationSizes.contentBoxLogicalSize != currentSizes.contentBoxLogicalSize) | ||
| return currentSizes; | ||
| if (m_lastObservationSizes.contentBoxLogicalSize != currentSizes->contentBoxLogicalSize) | ||
| return *currentSizes; |
There was a problem hiding this comment.
The change on this line isn’t needed.
There was a problem hiding this comment.
I could have sworn it gave compile errors without the change at the time, but indeed now I try again it builds fine.
rwlbuis
force-pushed
the
eng/content-visibility-Support-content-visibility-for-resizeintersection-observer
branch
from
2d844de
to
30ba836
Compare
|
EWS run on current version of this PR (hash 30ba836)
|
…on observer https://bugs.webkit.org/show_bug.cgi?id=245776 Reviewed by Darin Adler. Implement restrictions to resize and intersection observers as specified in the specification: https://w3c.github.io/csswg-drafts/css-contain/#cv-notes (Items 1 and 2) * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-030-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-031-expected.txt: * Source/WebCore/dom/Document.cpp: (WebCore::computeIntersectionState): * Source/WebCore/page/ResizeObservation.cpp: (WebCore::ResizeObservation::computeObservedSizes const): (WebCore::ResizeObservation::elementSizeChanged const): * Source/WebCore/page/ResizeObservation.h: Canonical link: https://commits.webkit.org/257256@main
webkit-commit-queue
force-pushed
the
eng/content-visibility-Support-content-visibility-for-resizeintersection-observer
branch
from
30ba836
to
56fda58
Compare
|
Committed 257256@main (56fda58): https://commits.webkit.org/257256@main Reviewed commits have been landed. Closing PR #6973 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
rwlbuis
deleted the
eng/content-visibility-Support-content-visibility-for-resizeintersection-observer
branch
56fda58
30ba836
🧪 ios-wk2🧪 api-mac🧪 api-gtk🧪 mac-wk1🧪 mac-wk2🧪 mac-AS-debug-wk2