Clear previous visual rects when composited PaintLayer goes away.
This avoids spurious layout shift reports when a static-positioned
element loses its PaintLayer and cc::Layer at the same time (a common
scenario at the end of a transition animation).
(cherry picked from commit 9e33cc0699b9c333d04fea46131c446ef0c1407a)
Bug: 960614
Change-Id: I0ab8b693dbb764880090d1b078cb138f4ca1ddb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1636403
Commit-Queue: Steve Kobes <skobes@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#665588}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1652614
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#215}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/third_party/blink/renderer/core/layout/jank_tracker_test.cc b/third_party/blink/renderer/core/layout/jank_tracker_test.cc
index 2df6908..ed95372 100644
--- a/third_party/blink/renderer/core/layout/jank_tracker_test.cc
+++ b/third_party/blink/renderer/core/layout/jank_tracker_test.cc
@@ -331,4 +331,55 @@
EXPECT_FLOAT_EQ(0.15, jank_tracker.WeightedScore());
}
+TEST_F(JankTrackerTest, StableCompositingChanges) {
+ SetBodyInnerHTML(R"HTML(
+ <style>
+ body { margin: 0; }
+ #outer {
+ margin-left: 50px;
+ margin-top: 50px;
+ width: 200px;
+ height: 200px;
+ background: #dde;
+ }
+ .tr {
+ will-change: transform;
+ }
+ .pl {
+ position: relative;
+ z-index: 0;
+ left: 0;
+ top: 0;
+ }
+ #inner {
+ display: inline-block;
+ width: 100px;
+ height: 100px;
+ background: #666;
+ margin-left: 50px;
+ margin-top: 50px;
+ }
+ </style>
+ <div id=outer><div id=inner></div></div>
+ )HTML");
+
+ Element* element = GetDocument().getElementById("outer");
+ size_t state = 0;
+ auto advance = [this, element, &state]() -> bool {
+ //
+ // Test each of the following transitions:
+ // - add/remove a PaintLayer
+ // - add/remove a cc::Layer when there is already a PaintLayer
+ // - add/remove a cc::Layer and a PaintLayer together
+
+ static const char* states[] = {"", "pl", "pl tr", "pl", "", "tr", ""};
+ element->setAttribute(html_names::kClassAttr, AtomicString(states[state]));
+ UpdateAllLifecyclePhases();
+ return ++state < sizeof states / sizeof *states;
+ };
+ while (advance()) {
+ }
+ EXPECT_FLOAT_EQ(0, GetJankTracker().Score());
+}
+
} // namespace blink
diff --git a/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc b/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc
index 8412d3f..4f41fac 100644
--- a/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc
+++ b/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc
@@ -381,8 +381,9 @@
#endif
}
-static void ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
- LayoutObject& layout_object) {
+void PaintLayerCompositor::
+ ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
+ LayoutObject& layout_object) {
// We clear the previous visual rect as it's wrong (paint invalidation
// container changed, ...). Forcing a full invalidation will make us recompute
// it. Also we are not changing the previous position from our paint
diff --git a/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h b/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h
index 7264ea9..bbb7d42 100644
--- a/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h
+++ b/third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h
@@ -167,6 +167,9 @@
compositing_inputs_root_.Update(layer);
}
+ void ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
+ LayoutObject&);
+
private:
#if DCHECK_IS_ON()
void AssertNoUnresolvedDirtyBits();
diff --git a/third_party/blink/renderer/core/paint/paint_layer.cc b/third_party/blink/renderer/core/paint/paint_layer.cc
index c08e7746..ae57e30e 100644
--- a/third_party/blink/renderer/core/paint/paint_layer.cc
+++ b/third_party/blink/renderer/core/paint/paint_layer.cc
@@ -214,8 +214,10 @@
// Child layers will be deleted by their corresponding layout objects, so
// we don't need to delete them ourselves.
-
- ClearCompositedLayerMapping(true);
+ {
+ DisableCompositingQueryAsserts disabler;
+ ClearCompositedLayerMapping(true);
+ }
if (scrollable_area_)
scrollable_area_->Dispose();
@@ -2793,7 +2795,7 @@
}
void PaintLayer::EnsureCompositedLayerMapping() {
- if (rare_data_ && rare_data_->composited_layer_mapping)
+ if (HasCompositedLayerMapping())
return;
EnsureRareData().composited_layer_mapping =
@@ -2803,7 +2805,25 @@
}
void PaintLayer::ClearCompositedLayerMapping(bool layer_being_destroyed) {
- if (!layer_being_destroyed) {
+ if (!HasCompositedLayerMapping())
+ return;
+
+ if (layer_being_destroyed) {
+ if (!RuntimeEnabledFeatures::CompositeAfterPaintEnabled()) {
+ // The visual rects will be in a different coordinate space after losing
+ // their compositing container. Clear them before prepaint to avoid
+ // spurious layout shift reports from JankTracker.
+ // If the PaintLayer were not being destroyed, this would happen during
+ // the compositing update (PaintLayerCompositor::UpdateIfNeeded).
+ // TODO: JankTracker's reliance on having visual rects cleared before
+ // prepaint in the case of compositing changes is not ideal, and will not
+ // work with CompositeAfterPaint. Some transform tree changes may still
+ // produce incorrect behavior from JankTracker (see discussion on review
+ // thread of http://crrev.com/c/1636403).
+ Compositor()->ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
+ layout_object_);
+ }
+ } else {
// We need to make sure our decendants get a geometry update. In principle,
// we could call setNeedsGraphicsLayerUpdate on our children, but that would
// require walking the z-order lists to find them. Instead, we
@@ -2813,9 +2833,8 @@
compositing_parent->GetCompositedLayerMapping()
->SetNeedsGraphicsLayerUpdate(kGraphicsLayerUpdateSubtree);
}
-
- if (rare_data_)
- rare_data_->composited_layer_mapping.reset();
+ DCHECK(rare_data_);
+ rare_data_->composited_layer_mapping.reset();
}
void PaintLayer::SetGroupedMapping(CompositedLayerMapping* grouped_mapping,