WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
204602
[Web Animations] Layout of children of element with forwards-filling opacity animation may be incorrect after removal
https://bugs.webkit.org/show_bug.cgi?id=204602
Summary
[Web Animations] Layout of children of element with forwards-filling opacity ...
Antoine Quint
Reported
2019-11-26 00:32:46 PST
Consider this markup: <div class="container animates" onAnimationEnd="event.target.classList.remove('animates')"> <div class="text">Hello.</div> </div> With these style rules: .container { margin-left: 100px; margin-top: 100px; } .animates { animation: fade 1s forwards; } .text { position: relative; } @keyframes fade { from { opacity: 0; } to { opacity: 1; } } As the animation runs, container has a layer because it has opacity < 1 and when it’s laid out originally it has a non-0,0 position. Meanwhile .text always has a layer because it has position: relative and it has a 0,0 position. As the animation ends, container still has a layer because its animation has fill: forwards and so we set setUsedZIndex(0) in KeyframeEffect::apply() because the spec tells us to (
https://w3c.github.io/web-animations/#side-effects-section
) since the animation targets opacity, a property which impacts whether an element establishes a stacking context. As the animation is removed, then the old style for container has usedZIndex() == 0 while the new style has hasAutoUsedZIndex() == true, which is expected as the animation is gone. However, when we get in that state RenderStyle::diff() will only return RepaintLayer and not Layout.
Attachments
Patch
(7.11 KB, patch)
2019-11-26 00:43 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(7.19 KB, patch)
2019-11-26 00:56 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(7.18 KB, patch)
2019-11-26 01:05 PST
,
Antoine Quint
simon.fraser
: review-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Antoine Quint
Comment 1
2019-11-26 00:33:11 PST
<
rdar://problem/45311541
>
Antoine Quint
Comment 2
2019-11-26 00:43:23 PST
Created
attachment 384332
[details]
Patch
Antti Koivisto
Comment 3
2019-11-26 00:46:33 PST
Comment on
attachment 384332
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=384332&action=review
> Source/WebCore/rendering/RenderLayerModelObject.cpp:188 > + // If the layer we're about to destroy had a position, then the layout of children will no longer be correct. > + bool layerHadLocation = !layer()->location().isZero();
I wonder if there is some existing way to just update layer positions instead of triggering a layout in render tree...
Antoine Quint
Comment 4
2019-11-26 00:48:28 PST
(In reply to Antti Koivisto from
comment #3
)
> Comment on
attachment 384332
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=384332&action=review
> > > Source/WebCore/rendering/RenderLayerModelObject.cpp:188 > > + // If the layer we're about to destroy had a position, then the layout of children will no longer be correct. > > + bool layerHadLocation = !layer()->location().isZero(); > > I wonder if there is some existing way to just update layer positions > instead of triggering a layout in render tree...
There is RenderLayer::updateLayerPositionsAfterStyleChange() which I could call on all the children I suppose. I'll give it a try.
Antoine Quint
Comment 5
2019-11-26 00:56:13 PST
Created
attachment 384334
[details]
Patch
Antti Koivisto
Comment 6
2019-11-26 01:02:13 PST
Comment on
attachment 384334
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=384334&action=review
> Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > + if (layerHadLocation && parentLayer) > + parentLayer->updateLayerPositionsAfterStyleChange();
Maybe you should also add !needsLayout() check here (like the other call site) as this is likely not needed then.
Antti Koivisto
Comment 7
2019-11-26 01:04:34 PST
I suppose that should be parentLayer->renderer().needsLayout() then.
Antoine Quint
Comment 8
2019-11-26 01:05:26 PST
Created
attachment 384335
[details]
Patch
Antoine Quint
Comment 9
2019-11-26 02:12:05 PST
Committed
r252879
: <
https://trac.webkit.org/changeset/252879
>
Simon Fraser (smfr)
Comment 10
2019-11-26 10:11:20 PST
Comment on
attachment 384335
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=384335&action=review
> Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > + parentLayer->updateLayerPositionsAfterStyleChange();
Nooo, you can't call this here. This needs to happen after the entire style update. This is why we need some new RenderStyle::Diff values.
Simon Fraser (smfr)
Comment 11
2019-11-26 10:13:00 PST
Please revert. Give the bug to me.
Antoine Quint
Comment 12
2019-11-27 04:04:29 PST
(In reply to Simon Fraser (smfr) from
comment #10
)
> Comment on
attachment 384335
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=384335&action=review
> > > Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > > + parentLayer->updateLayerPositionsAfterStyleChange(); > > Nooo, you can't call this here. This needs to happen after the entire style > update. This is why we need some new RenderStyle::Diff values.
Why is not good to call this here? That method is also called in RenderLayerModelObject::styleDidChange() in the case where we destroy a layer, although it's called on a different layer (the destroyed one).
Simon Fraser (smfr)
Comment 13
2019-11-28 07:53:22 PST
This is the same issue as
bug 202505
.
Simon Fraser (smfr)
Comment 14
2019-11-28 07:54:12 PST
(In reply to Antoine Quint from
comment #12
)
> (In reply to Simon Fraser (smfr) from
comment #10
) > > Comment on
attachment 384335
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=384335&action=review
> > > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:192 > > > + parentLayer->updateLayerPositionsAfterStyleChange(); > > > > Nooo, you can't call this here. This needs to happen after the entire style > > update. This is why we need some new RenderStyle::Diff values. > > Why is not good to call this here? That method is also called in > RenderLayerModelObject::styleDidChange() in the case where we destroy a > layer, although it's called on a different layer (the destroyed one).
updateLayerPositionsAfterStyleChange walks all the descendant layers, so if you cal it from here, you risk walking some subtrees multiple times. updateLayerPositionsAfterStyleChange() is intended to called once on the root.
Simon Fraser (smfr)
Comment 15
2019-11-28 22:51:14 PST
I improved on this change in
r252935
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug