Element jumps to wrong position after perspective change on ancestor
https://bugs.webkit.org/show_bug.cgi?id=202505
<rdar://problem/55930710>
Reviewed by Antti Koivisto.
Source/WebCore:
This modifies the fix in r252879 to be better-performing and to avoid a new call site for updateLayerPositions*.
Style can change in a way that creates or destroys RenderLayers, but does not result in a layout; this can happen
with changes of properties like opacity or perspective. When this happens, something needs to trigger a call to
RenderLayer::updateLayerPositions() on the root of the changed subtree. This is best done after the style update,
to avoid multiple updateLayerPositions traversals.
Implement this by storing on RenderView the rootmost changed layer, and having FrameView::styleDidChange()
call updateLayerPositionsAfterStyleChange() if we're after a style change with no pending layout.
Test: compositing/geometry/layer-position-after-removing-perspective.html
* page/FrameView.cpp:
(WebCore::FrameView::styleDidChange):
* page/FrameView.h:
* platform/ScrollView.h:
* rendering/RenderElement.cpp:
(WebCore::RenderElement::didAttachChild):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::insertOnlyThisLayer):
(WebCore::RenderLayer::removeOnlyThisLayer):
(WebCore::findCommonAncestor):
(WebCore::RenderLayer::commonAncestorWithLayer const):
* rendering/RenderLayer.h:
* rendering/RenderLayerModelObject.cpp:
(WebCore::RenderLayerModelObject::createLayer):
(WebCore::RenderLayerModelObject::styleDidChange):
* rendering/RenderView.cpp:
(WebCore::RenderView::layerChildrenChangedDuringStyleChange):
(WebCore::RenderView::takeStyleChangeLayerTreeMutationRoot):
* rendering/RenderView.h:
LayoutTests:
* compositing/geometry/layer-position-after-removing-perspective-expected.html: Added.
* compositing/geometry/layer-position-after-removing-perspective.html: Added.
* css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt: Rebaselined.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@252935 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 49246ac..84f922d 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2019-11-28 Simon Fraser <simon.fraser@apple.com>
+
+ Element jumps to wrong position after perspective change on ancestor
+ https://bugs.webkit.org/show_bug.cgi?id=202505
+ <rdar://problem/55930710>
+
+ Reviewed by Antti Koivisto.
+
+ * compositing/geometry/layer-position-after-removing-perspective-expected.html: Added.
+ * compositing/geometry/layer-position-after-removing-perspective.html: Added.
+ * css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt: Rebaselined.
+
2019-11-28 Jonathan Bedard <jbedard@apple.com>
[WebGL] Garden dedicated queue (Part 10)
diff --git a/LayoutTests/compositing/geometry/layer-position-after-removing-perspective-expected.html b/LayoutTests/compositing/geometry/layer-position-after-removing-perspective-expected.html
new file mode 100644
index 0000000..771c03e
--- /dev/null
+++ b/LayoutTests/compositing/geometry/layer-position-after-removing-perspective-expected.html
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <style>
+ div {
+ height: 200px;
+ width: 200px;
+ margin: 100px 10px;
+ background-color: blue;
+ }
+
+ .rotated {
+ background-color: green;
+ margin: 0;
+ transform: rotateY(45deg);
+ }
+
+ #target {
+ perspective: none;
+ }
+ </style>
+</head>
+<body>
+ <div id="target" class="container">
+ <div class="rotated"></div>
+ </div>
+</body>
+</html>
+
diff --git a/LayoutTests/compositing/geometry/layer-position-after-removing-perspective.html b/LayoutTests/compositing/geometry/layer-position-after-removing-perspective.html
new file mode 100644
index 0000000..b30ee96
--- /dev/null
+++ b/LayoutTests/compositing/geometry/layer-position-after-removing-perspective.html
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <style>
+ div {
+ height: 200px;
+ width: 200px;
+ margin: 100px 10px;
+ background-color: blue;
+ }
+
+ .rotated {
+ background-color: green;
+ margin: 0;
+ transform: rotateY(45deg);
+ }
+
+ #target {
+ perspective: 500px;
+ }
+
+ #target.changed {
+ perspective: none;
+ }
+ </style>
+ <script>
+ if (window.testRunner)
+ testRunner.waitUntilDone();
+
+ window.addEventListener('load', () => {
+ setTimeout(() => {
+ document.getElementById('target').classList.add('changed');
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 0);
+ }, false);
+ </script>
+</head>
+<body>
+ <div id="target" class="container">
+ <div class="rotated"></div>
+ </div>
+</body>
+</html>
+
diff --git a/LayoutTests/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt b/LayoutTests/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt
index ebeb13e..6960647 100644
--- a/LayoutTests/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt
+++ b/LayoutTests/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt
@@ -14,7 +14,6 @@
(rect 48 54 60 60)
(rect 48 54 60 60)
(rect 48 54 60 60)
- (rect 48 54 60 60)
(rect 48 172 60 60)
(rect 48 172 60 60)
(rect 28 290 60 60)
@@ -29,6 +28,7 @@
(rect 48 644 60 60)
(rect 28 290 60 60)
(rect 48 644 60 60)
+ (rect 48 54 60 60)
(rect 48 290 60 60)
(rect 28 526 60 60)
(rect 88 644 20 60)
diff --git a/LayoutTests/platform/ios/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt b/LayoutTests/platform/ios/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt
index c2efb39..211da52 100644
--- a/LayoutTests/platform/ios/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt
+++ b/LayoutTests/platform/ios/css3/blending/repaint/blend-mode-isolate-stacking-context-expected.txt
@@ -14,7 +14,6 @@
(rect 48 56 60 60)
(rect 48 56 60 60)
(rect 48 56 60 60)
- (rect 48 56 60 60)
(rect 48 176 60 60)
(rect 48 176 60 60)
(rect 28 296 60 60)
@@ -29,6 +28,7 @@
(rect 48 656 60 60)
(rect 28 296 60 60)
(rect 48 656 60 60)
+ (rect 48 56 60 60)
(rect 48 296 60 60)
(rect 28 536 60 60)
(rect 88 656 20 60)
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 6127bf8..f1083a7 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,43 @@
+2019-11-28 Simon Fraser <simon.fraser@apple.com>
+
+ Element jumps to wrong position after perspective change on ancestor
+ https://bugs.webkit.org/show_bug.cgi?id=202505
+ <rdar://problem/55930710>
+
+ Reviewed by Antti Koivisto.
+
+ This modifies the fix in r252879 to be better-performing and to avoid a new call site for updateLayerPositions*.
+
+ Style can change in a way that creates or destroys RenderLayers, but does not result in a layout; this can happen
+ with changes of properties like opacity or perspective. When this happens, something needs to trigger a call to
+ RenderLayer::updateLayerPositions() on the root of the changed subtree. This is best done after the style update,
+ to avoid multiple updateLayerPositions traversals.
+
+ Implement this by storing on RenderView the rootmost changed layer, and having FrameView::styleDidChange()
+ call updateLayerPositionsAfterStyleChange() if we're after a style change with no pending layout.
+
+ Test: compositing/geometry/layer-position-after-removing-perspective.html
+
+ * page/FrameView.cpp:
+ (WebCore::FrameView::styleDidChange):
+ * page/FrameView.h:
+ * platform/ScrollView.h:
+ * rendering/RenderElement.cpp:
+ (WebCore::RenderElement::didAttachChild):
+ * rendering/RenderLayer.cpp:
+ (WebCore::RenderLayer::insertOnlyThisLayer):
+ (WebCore::RenderLayer::removeOnlyThisLayer):
+ (WebCore::findCommonAncestor):
+ (WebCore::RenderLayer::commonAncestorWithLayer const):
+ * rendering/RenderLayer.h:
+ * rendering/RenderLayerModelObject.cpp:
+ (WebCore::RenderLayerModelObject::createLayer):
+ (WebCore::RenderLayerModelObject::styleDidChange):
+ * rendering/RenderView.cpp:
+ (WebCore::RenderView::layerChildrenChangedDuringStyleChange):
+ (WebCore::RenderView::takeStyleChangeLayerTreeMutationRoot):
+ * rendering/RenderView.h:
+
2019-11-28 Antti Koivisto <antti@apple.com>
[LFC][IFC] Remove m_inlineRunToLineMap
diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
index 0f70ac7..04012f9 100644
--- a/Source/WebCore/page/FrameView.cpp
+++ b/Source/WebCore/page/FrameView.cpp
@@ -799,6 +799,18 @@
renderView->compositor().willRecalcStyle();
}
+void FrameView::styleDidChange()
+{
+ ScrollView::styleDidChange();
+ RenderView* renderView = this->renderView();
+ if (!renderView)
+ return;
+
+ RenderLayer* layerTreeMutationRoot = renderView->takeStyleChangeLayerTreeMutationRoot();
+ if (layerTreeMutationRoot && !needsLayout())
+ layerTreeMutationRoot->updateLayerPositionsAfterStyleChange();
+}
+
bool FrameView::updateCompositingLayersAfterStyleChange()
{
// If we expect to update compositing after an incipient layout, don't do so here.
diff --git a/Source/WebCore/page/FrameView.h b/Source/WebCore/page/FrameView.h
index ded7a5b..e36f996 100644
--- a/Source/WebCore/page/FrameView.h
+++ b/Source/WebCore/page/FrameView.h
@@ -138,6 +138,7 @@
#endif
void willRecalcStyle();
+ void styleDidChange() override;
bool updateCompositingLayersAfterStyleChange();
void updateCompositingLayersAfterLayout();
diff --git a/Source/WebCore/platform/ScrollView.h b/Source/WebCore/platform/ScrollView.h
index 03eb5d1..430160d 100644
--- a/Source/WebCore/platform/ScrollView.h
+++ b/Source/WebCore/platform/ScrollView.h
@@ -140,7 +140,7 @@
// Overridden by FrameView to create custom CSS scrollbars if applicable.
virtual Ref<Scrollbar> createScrollbar(ScrollbarOrientation);
- void styleDidChange();
+ virtual void styleDidChange();
// If the prohibits scrolling flag is set, then all scrolling in the view (even programmatic scrolling) is turned off.
void setProhibitsScrolling(bool b) { m_prohibitsScrolling = b; }
diff --git a/Source/WebCore/rendering/RenderElement.cpp b/Source/WebCore/rendering/RenderElement.cpp
index e7f903a..242f177 100644
--- a/Source/WebCore/rendering/RenderElement.cpp
+++ b/Source/WebCore/rendering/RenderElement.cpp
@@ -470,7 +470,7 @@
// To avoid the problem alltogether, detect early if we're inside a hidden SVG subtree
// and stop creating layers at all for these cases - they're not used anyways.
if (child.hasLayer() && !layerCreationAllowedForSubtree())
- downcast<RenderLayerModelObject>(child).layer()->removeOnlyThisLayer();
+ downcast<RenderLayerModelObject>(child).layer()->removeOnlyThisLayer(RenderLayer::LayerChangeTiming::RenderTreeConstruction);
}
RenderObject* RenderElement::attachRendererInternal(RenderPtr<RenderObject> child, RenderObject* beforeChild)
diff --git a/Source/WebCore/rendering/RenderLayer.cpp b/Source/WebCore/rendering/RenderLayer.cpp
index 22876b7..588498c 100644
--- a/Source/WebCore/rendering/RenderLayer.cpp
+++ b/Source/WebCore/rendering/RenderLayer.cpp
@@ -496,7 +496,7 @@
}
}
-void RenderLayer::insertOnlyThisLayer()
+void RenderLayer::insertOnlyThisLayer(LayerChangeTiming timing)
{
if (!m_parent && renderer().parent()) {
// We need to connect ourselves when our renderer() has a parent.
@@ -511,15 +511,23 @@
for (auto& child : childrenOfType<RenderElement>(renderer()))
child.moveLayers(m_parent, this);
+ if (parent()) {
+ if (timing == LayerChangeTiming::StyleChange)
+ renderer().view().layerChildrenChangedDuringStyleChange(*parent());
+ }
+
// Clear out all the clip rects.
clearClipRectsIncludingDescendants();
}
-void RenderLayer::removeOnlyThisLayer()
+void RenderLayer::removeOnlyThisLayer(LayerChangeTiming timing)
{
if (!m_parent)
return;
+ if (timing == LayerChangeTiming::StyleChange)
+ renderer().view().layerChildrenChangedDuringStyleChange(*parent());
+
// Mark that we are about to lose our layer. This makes render tree
// walks ignore this layer while we're removing it.
renderer().setHasLayer(false);
@@ -2250,6 +2258,27 @@
return false;
}
+static RenderLayer* findCommonAncestor(const RenderLayer& firstLayer, const RenderLayer& secondLayer)
+{
+ if (&firstLayer == &secondLayer)
+ return const_cast<RenderLayer*>(&firstLayer);
+
+ HashSet<const RenderLayer*> ancestorChain;
+ for (auto* currLayer = &firstLayer; currLayer; currLayer = currLayer->parent())
+ ancestorChain.add(currLayer);
+
+ for (auto* currLayer = &secondLayer; currLayer; currLayer = currLayer->parent()) {
+ if (ancestorChain.contains(currLayer))
+ return const_cast<RenderLayer*>(currLayer);
+ }
+ return nullptr;
+}
+
+RenderLayer* RenderLayer::commonAncestorWithLayer(const RenderLayer& layer) const
+{
+ return findCommonAncestor(*this, layer);
+}
+
void RenderLayer::convertToPixelSnappedLayerCoords(const RenderLayer* ancestorLayer, IntPoint& roundedLocation, ColumnOffsetAdjustment adjustForColumns) const
{
LayoutPoint location = convertToLayerCoords(ancestorLayer, roundedLocation, adjustForColumns);
diff --git a/Source/WebCore/rendering/RenderLayer.h b/Source/WebCore/rendering/RenderLayer.h
index 49ee9d4..6ca422b 100644
--- a/Source/WebCore/rendering/RenderLayer.h
+++ b/Source/WebCore/rendering/RenderLayer.h
@@ -163,6 +163,7 @@
RenderLayer* firstChild() const { return m_first; }
RenderLayer* lastChild() const { return m_last; }
bool isDescendantOf(const RenderLayer&) const;
+ RenderLayer* commonAncestorWithLayer(const RenderLayer&) const;
// This does an ancestor tree walk. Avoid it!
const RenderLayer* root() const
@@ -176,8 +177,12 @@
void addChild(RenderLayer& newChild, RenderLayer* beforeChild = nullptr);
void removeChild(RenderLayer&);
- void insertOnlyThisLayer();
- void removeOnlyThisLayer();
+ enum class LayerChangeTiming {
+ StyleChange,
+ RenderTreeConstruction,
+ };
+ void insertOnlyThisLayer(LayerChangeTiming);
+ void removeOnlyThisLayer(LayerChangeTiming);
bool isNormalFlowOnly() const { return m_isNormalFlowOnly; }
diff --git a/Source/WebCore/rendering/RenderLayerModelObject.cpp b/Source/WebCore/rendering/RenderLayerModelObject.cpp
index fd4c3df..6e68f62 100644
--- a/Source/WebCore/rendering/RenderLayerModelObject.cpp
+++ b/Source/WebCore/rendering/RenderLayerModelObject.cpp
@@ -97,7 +97,7 @@
ASSERT(!m_layer);
m_layer = makeUnique<RenderLayer>(*this);
setHasLayer(true);
- m_layer->insertOnlyThisLayer();
+ m_layer->insertOnlyThisLayer(RenderLayer::LayerChangeTiming::StyleChange);
}
bool RenderLayerModelObject::hasSelfPaintingLayer() const
@@ -169,27 +169,21 @@
if (s_wasFloating && isFloating())
setChildNeedsLayout();
createLayer();
- if (parent() && !needsLayout() && containingBlock()) {
+ if (parent() && !needsLayout() && containingBlock())
layer()->setRepaintStatus(NeedsFullRepaint);
- layer()->updateLayerPositionsAfterStyleChange();
- }
}
} else if (layer() && layer()->parent()) {
#if ENABLE(CSS_COMPOSITING)
if (oldStyle->hasBlendMode())
layer()->willRemoveChildWithBlendMode();
#endif
- setHasTransformRelatedProperty(false); // All transform-related propeties force layers, so we know we don't have one or the object doesn't support them.
+ setHasTransformRelatedProperty(false); // All transform-related properties force layers, so we know we don't have one or the object doesn't support them.
setHasReflection(false);
// Repaint the about to be destroyed self-painting layer when style change also triggers repaint.
if (layer()->isSelfPaintingLayer() && layer()->repaintStatus() == NeedsFullRepaint && hasRepaintLayoutRects())
repaintUsingContainer(containerForRepaint(), repaintLayoutRects().m_repaintRect);
- // If the layer we're about to destroy had a position, then the positions of the current children will no longer be correct.
- auto* parentLayer = layer()->parent();
- bool layerHadLocation = !layer()->location().isZero();
- layer()->removeOnlyThisLayer(); // calls destroyLayer() which clears m_layer
- if (layerHadLocation && parentLayer && !parentLayer->renderer().needsLayout())
- parentLayer->updateLayerPositionsAfterStyleChange();
+
+ layer()->removeOnlyThisLayer(RenderLayer::LayerChangeTiming::StyleChange); // calls destroyLayer() which clears m_layer
if (s_wasFloating && isFloating())
setChildNeedsLayout();
if (s_hadTransform)
diff --git a/Source/WebCore/rendering/RenderView.cpp b/Source/WebCore/rendering/RenderView.cpp
index e565960..f648614 100644
--- a/Source/WebCore/rendering/RenderView.cpp
+++ b/Source/WebCore/rendering/RenderView.cpp
@@ -877,6 +877,24 @@
return 0;
}
+void RenderView::layerChildrenChangedDuringStyleChange(RenderLayer& layer)
+{
+ if (!m_styleChangeLayerMutationRoot) {
+ m_styleChangeLayerMutationRoot = makeWeakPtr(layer);
+ return;
+ }
+
+ RenderLayer* commonAncestor = m_styleChangeLayerMutationRoot->commonAncestorWithLayer(layer);
+ m_styleChangeLayerMutationRoot = makeWeakPtr(commonAncestor);
+}
+
+RenderLayer* RenderView::takeStyleChangeLayerTreeMutationRoot()
+{
+ auto* result = m_styleChangeLayerMutationRoot.get();
+ m_styleChangeLayerMutationRoot.clear();
+ return result;
+}
+
#if ENABLE(CSS_SCROLL_SNAP)
void RenderView::registerBoxWithScrollSnapPositions(const RenderBox& box)
{
diff --git a/Source/WebCore/rendering/RenderView.h b/Source/WebCore/rendering/RenderView.h
index b69df0f..1842b30 100644
--- a/Source/WebCore/rendering/RenderView.h
+++ b/Source/WebCore/rendering/RenderView.h
@@ -184,6 +184,9 @@
void scheduleLazyRepaint(RenderBox&);
void unscheduleLazyRepaint(RenderBox&);
+
+ void layerChildrenChangedDuringStyleChange(RenderLayer&);
+ RenderLayer* takeStyleChangeLayerTreeMutationRoot();
void protectRenderWidgetUntilLayoutIsDone(RenderWidget& widget) { m_protectedRenderWidgets.append(&widget); }
void releaseProtectedRenderWidgets() { m_protectedRenderWidgets.clear(); }
@@ -219,6 +222,8 @@
mutable std::unique_ptr<Region> m_accumulatedRepaintRegion;
SelectionRangeData m_selection;
+ WeakPtr<RenderLayer> m_styleChangeLayerMutationRoot;
+
// FIXME: Only used by embedded WebViews inside AppKit NSViews. Find a way to remove.
struct LegacyPrinting {
int m_bestTruncatedAt { 0 };
