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

[motion-path] Check if we are mid layout when setting containing block rect for ray #16633

Conversation

nmoucht
Copy link
Contributor

@nmoucht nmoucht commented Aug 12, 2023

acfa97d

[motion-path] Check if we are mid layout when setting containing block rect for ray
https://bugs.webkit.org/show_bug.cgi?id=260110
rdar://113780201

Reviewed by Simon Fraser.

On the first RenderLayer::updateTransform call the parent is mid layout, so it is
inorrect to query it about its rect size. If we have a motion path transform, set
that this RenderObject needs a transform update on the parent through the FrameView's
layout context, and have the parent call updateTransform after it has completed layout.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-019-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-019.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-020-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-020.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-021-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-021.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-022-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-022.html: Added.

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

653d25c

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@nmoucht nmoucht self-assigned this Aug 12, 2023
@nmoucht nmoucht added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Aug 12, 2023
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from dcfc057 to 06e6453 Compare August 12, 2023 01:04
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 12, 2023
@nmoucht nmoucht removed the merging-blocked Applied to prevent a change from being merged label Aug 14, 2023
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from 06e6453 to a6edd43 Compare August 14, 2023 20:23
@nmoucht nmoucht requested a review from smfr August 14, 2023 23:49
Source/WebCore/rendering/RenderBlockFlow.h Outdated Show resolved Hide resolved
Source/WebCore/rendering/RenderLayer.cpp Outdated Show resolved Hide resolved
Source/WebCore/rendering/RenderLayer.cpp Outdated Show resolved Hide resolved
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from a6edd43 to 5022957 Compare August 15, 2023 02:45
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 15, 2023
@nmoucht nmoucht removed the merging-blocked Applied to prevent a change from being merged label Aug 15, 2023
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from 5022957 to 3b896cc Compare August 15, 2023 19:31
@nmoucht nmoucht requested a review from smfr August 15, 2023 21:14
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from 3b896cc to d903dfd Compare August 17, 2023 00:02
Source/WebCore/rendering/RenderLayerModelObject.cpp Outdated Show resolved Hide resolved
Source/WebCore/rendering/RenderLayerModelObject.cpp Outdated Show resolved Hide resolved
auto& rayPathOperation = downcast<RayPathOperation>(*pathOperation);
auto pathReferenceBoxRect = snapRectToDevicePixelsIfNeeded(parentBlock->transformReferenceBoxRect(parentBlock->style()), *this);
if (auto* containingBlockFlow = dynamicDowncast<RenderBlockFlow>(parentBlock)) {
if (parentBlock->needsLayout() && !containingBlockFlow->wasMarkedNeedingTransformUpdate(this)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit gross that this sets some state on the containingBlockFlow. Why not just change the code to always update motion path things after the containing block has finished layout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to set this state in RenderLayer::updateTransform and moved the rest to MotionPathHelper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if RenderLayer::updateTransform() is the right place to set this state. We can either set it here or we have to set in every RenderObject subclass where we call updateLayerTransform() in the individual layout() functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the marking of needing a transform update to RenderLayerModelObject::updateLayerTransform.

Source/WebCore/rendering/RenderBlockFlow.h Outdated Show resolved Hide resolved
Source/WebCore/rendering/RenderBlockFlow.cpp Outdated Show resolved Hide resolved
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 17, 2023
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Aug 18, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 18, 2023
@nmoucht nmoucht removed the merging-blocked Applied to prevent a change from being merged label Aug 19, 2023
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from d903dfd to df4af25 Compare August 19, 2023 03:10
@nmoucht nmoucht requested a review from cdumez as a code owner August 19, 2023 03:10
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 19, 2023
@nmoucht nmoucht removed the merging-blocked Applied to prevent a change from being merged label Aug 19, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 25, 2023
@nmoucht nmoucht removed the merging-blocked Applied to prevent a change from being merged label Aug 28, 2023
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch 2 times, most recently from a423034 to 0f378d6 Compare August 28, 2023 21:21
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from 0f378d6 to a716265 Compare August 28, 2023 21:26
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from a716265 to 1cb7aaa Compare August 29, 2023 20:59
Source/WebCore/page/LocalFrameViewLayoutContext.h Outdated Show resolved Hide resolved
Source/WebCore/page/LocalFrameViewLayoutContext.h Outdated Show resolved Hide resolved
Source/WebCore/page/LocalFrameViewLayoutContext.h Outdated Show resolved Hide resolved
@@ -3548,4 +3548,14 @@ LayoutUnit RenderBlock::layoutOverflowLogicalBottom(const RenderBlock& renderer)
return std::max(renderer.clientLogicalBottom(), maxChildLogicalBottom + renderer.paddingAfter());
}

void RenderBlock::updateDescendantTransformsAfterLayout()
{
auto descendants = view().frameView().layoutContext().descendantsNeedingTransformUpdate(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

It could still be takeBoxesNeedingTransformUpdateAfterContainerLayout() which would extract them from the hash map, and remove the need for finishedUpdatingDescendants().

Source/WebCore/page/LocalFrameViewLayoutContext.cpp Outdated Show resolved Hide resolved
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch 2 times, most recently from 6db36a2 to af3d612 Compare August 30, 2023 18:23
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from af3d612 to e32fb19 Compare August 30, 2023 19:40
@nmoucht nmoucht requested a review from smfr August 30, 2023 19:44
@nmoucht nmoucht force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from e32fb19 to 653d25c Compare August 31, 2023 00:08
@nmoucht nmoucht added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 31, 2023
…k rect for ray

https://bugs.webkit.org/show_bug.cgi?id=260110
rdar://113780201

Reviewed by Simon Fraser.

On the first RenderLayer::updateTransform call the parent is mid layout, so it is
inorrect to query it about its rect size. If we have a motion path transform, set
that this RenderObject needs a transform update on the parent through the FrameView's
layout context, and have the parent call updateTransform after it has completed layout.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-019-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-019.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-020-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-020.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-021-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-021.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-022-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/offset-path-ray-022.html: Added.

Canonical link: https://commits.webkit.org/267479@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/motion-path-Check-if-we-are-mid-layout-when-setting-containing-block-rect-for-ray branch from 653d25c to acfa97d Compare August 31, 2023 02:10
@webkit-commit-queue
Copy link
Collaborator

Committed 267479@main (acfa97d): https://commits.webkit.org/267479@main

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

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 31, 2023
@webkit-commit-queue webkit-commit-queue merged commit acfa97d into WebKit:main Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
7 participants