Fix wrong backface visibility of composited layer under 3d transforms

In blink: - Change the condition in PaintPropertyTreeBuilder for inheriting parent backface visibility. Now a transform node inherits parent backface visibility if there is the parent doesn't preserve 3d and the current LayoutObject doesn't have 3d transform properties. This is based on dbaron@'s crrev.com/c/4917284. - Let TransformPaintPropertyNode::DelegatesToParentForBackface() reflect the backface visibility inheritance status, instead of the original State::delegates_to_parent_for_backface flag which is for scroll translation node only. In cc: - draw_property_utils functions related to backface visibility are changed to support TransformNode::delegates_to_parent_for_backface set on multiple levels of transform nodes. The blink changes are behind feature BackfaceVisibilityNewInheritance (enabled by default). When the feature is not enabled, the cc change has no effect because TransformNode::delegates_to_parent_for_backface won't be set on both a parent node and a child node. Bug: 954591 Change-Id: I39e8e46ebd053430368d3e6214a970dfce0c4940 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935628 Reviewed-by: David Baron <dbaron@chromium.org> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/main@{#1211704}
chromium · Oct 18, 2023 · cb0abd4 · cb0abd4
1 parent eb0d557
commit cb0abd4
Show file tree

Hide file tree

Showing 16 changed files with 185 additions and 129 deletions.
diff --git a/cc/trees/draw_properties_unittest.cc b/cc/trees/draw_properties_unittest.cc
@@ -3742,15 +3742,12 @@ TEST_F(BackfaceVisibilityInteropTest, BackfaceInvisibleTransform) {
 
   UpdateActiveTreeDrawProperties();
 
-  EXPECT_TRUE(draw_property_utils::IsLayerBackFaceVisible(
-      back_facing, back_facing->transform_tree_index(),
-      host_impl()->active_tree()->property_trees()));
-  EXPECT_TRUE(draw_property_utils::IsLayerBackFaceVisible(
-      back_facing, back_facing_double_sided->transform_tree_index(),
-      host_impl()->active_tree()->property_trees()));
-  EXPECT_FALSE(draw_property_utils::IsLayerBackFaceVisible(
-      front_facing, front_facing->transform_tree_index(),
-      host_impl()->active_tree()->property_trees()));
+  EXPECT_TRUE(draw_property_utils::IsLayerBackFaceVisibleForTesting(
+      back_facing, host_impl()->active_tree()->property_trees()));
+  EXPECT_TRUE(draw_property_utils::IsLayerBackFaceVisibleForTesting(
+      back_facing_double_sided, host_impl()->active_tree()->property_trees()));
+  EXPECT_FALSE(draw_property_utils::IsLayerBackFaceVisibleForTesting(
+      front_facing, host_impl()->active_tree()->property_trees()));
 
   EXPECT_TRUE(back_facing->raster_even_if_not_drawn());
   EXPECT_TRUE(

diff --git a/cc/trees/draw_property_utils.cc b/cc/trees/draw_property_utils.cc
@@ -419,61 +419,43 @@ void ClearRenderSurfaceCommonAncestorClip(LayerImpl* layer) {
 }
 
 template <typename LayerType>
-int TransformTreeIndexForBackfaceVisibility(LayerType* layer,
-                                            const TransformTree& tree) {
-  return layer->transform_tree_index();
-}
-
-bool IsTargetSpaceTransformBackFaceVisible(
-    Layer* layer,
-    int transform_tree_index,
-    const PropertyTrees* property_trees) {
-  // We do not skip back face invisible layers on main thread as target space
-  // transform will not be available here.
-  return false;
+const TransformNode& TransformNodeForBackfaceVisibility(
+    LayerType* layer,
+    const TransformTree& tree) {
+  const TransformNode* node = tree.Node(layer->transform_tree_index());
+  while (node->delegates_to_parent_for_backface) {
+    const TransformNode* parent = tree.Node(node->parent_id);
+    CHECK(node);
+    // Backface visibility inheritance should not cross 3d sorting contexts.
+    DCHECK(!node->sorting_context_id ||
+           parent->sorting_context_id == node->sorting_context_id);
+    node = parent;
+  }
+  return *node;
 }
 
 bool IsTargetSpaceTransformBackFaceVisible(
-    LayerImpl* layer,
-    int transform_tree_index,
+    const LayerImpl* layer,
     const PropertyTrees* property_trees) {
-  const TransformTree& transform_tree = property_trees->transform_tree();
-  const TransformNode& transform_node =
-      *transform_tree.Node(transform_tree_index);
-  if (transform_node.delegates_to_parent_for_backface)
-    transform_tree_index = transform_node.parent_id;
-
+  const TransformNode& transform_node = TransformNodeForBackfaceVisibility(
+      layer, property_trees->transform_tree());
   gfx::Transform to_target;
-  property_trees->GetToTarget(transform_tree_index,
-                              layer->render_target_effect_tree_index(),
-                              &to_target);
-  return to_target.IsBackFaceVisible();
-}
+  property_trees->GetToTarget(
+      transform_node.id, layer->render_target_effect_tree_index(), &to_target);
 
-bool IsTransformToRootOf3DRenderingContextBackFaceVisible(
-    Layer* layer,
-    int transform_tree_index,
-    const PropertyTrees* property_trees) {
-  // We do not skip back face invisible layers on main thread as target space
-  // transform will not be available here.
-  return false;
+  return to_target.IsBackFaceVisible();
 }
 
 bool IsTransformToRootOf3DRenderingContextBackFaceVisible(
-    LayerImpl* layer,
-    int transform_tree_index,
+    const LayerImpl* layer,
     const PropertyTrees* property_trees) {
   const TransformTree& transform_tree = property_trees->transform_tree();
 
   const TransformNode& transform_node =
-      *transform_tree.Node(transform_tree_index);
+      TransformNodeForBackfaceVisibility(layer, transform_tree);
   const TransformNode* root_node = &transform_node;
-  if (transform_node.delegates_to_parent_for_backface) {
-    transform_tree_index = transform_node.parent_id;
-    root_node = transform_tree.Node(transform_tree_index);
-  }
 
-  int root_id = transform_tree_index;
+  int root_id = transform_node.id;
   int sorting_context_id = transform_node.sorting_context_id;
 
   while (root_id > kRootPropertyNodeId) {
@@ -488,24 +470,35 @@ bool IsTransformToRootOf3DRenderingContextBackFaceVisible(
   // TODO(chrishtr): cache this on the transform trees if needed, similar to
   // |to_target| and |to_screen|.
   gfx::Transform to_3d_root;
-  if (transform_tree_index != root_id)
+  if (transform_node.id != root_id) {
     property_trees->transform_tree().CombineTransformsBetween(
-        transform_tree_index, root_id, &to_3d_root);
+        transform_node.id, root_id, &to_3d_root);
+  }
   to_3d_root.PreConcat(root_node->to_parent);
   return to_3d_root.IsBackFaceVisible();
 }
 
-inline bool TransformToScreenIsKnown(Layer* layer,
-                                     int transform_tree_index,
-                                     const TransformTree& tree) {
-  const TransformNode* node = tree.Node(transform_tree_index);
-  return !node->to_screen_is_potentially_animated;
+bool IsLayerBackFaceVisible(const Layer* layer,
+                            const PropertyTrees* property_trees) {
+  // We do not skip back face invisible layers on main thread as target space
+  // transform will not be available here.
+  return false;
 }
 
-inline bool TransformToScreenIsKnown(LayerImpl* layer,
-                                     int transform_tree_index,
-                                     const TransformTree& tree) {
-  return true;
+bool IsLayerBackFaceVisible(const LayerImpl* layer,
+                            const PropertyTrees* property_trees) {
+  // A layer with singular transform is not drawn. So, we can assume that its
+  // backface is not visible.
+  if (HasSingularTransform(layer->transform_tree_index(),
+                           property_trees->transform_tree())) {
+    return false;
+  }
+  if (layer->layer_tree_impl()->settings().enable_backface_visibility_interop) {
+    return IsTransformToRootOf3DRenderingContextBackFaceVisible(layer,
+                                                                property_trees);
+  } else {
+    return IsTargetSpaceTransformBackFaceVisible(layer, property_trees);
+  }
 }
 
 template <typename LayerType>
@@ -536,18 +529,9 @@ bool LayerNeedsUpdate(LayerType* layer,
 
   // The layer should not be drawn if (1) it is not double-sided and (2) the
   // back of the layer is known to be facing the screen.
-  const TransformTree& tree = property_trees->transform_tree();
-  if (layer->should_check_backface_visibility()) {
-    int backface_transform_id =
-        TransformTreeIndexForBackfaceVisibility(layer, tree);
-    // A layer with singular transform is not drawn. So, we can assume that its
-    // backface is not visible.
-    if (TransformToScreenIsKnown(layer, backface_transform_id, tree) &&
-        !HasSingularTransform(backface_transform_id, tree) &&
-        draw_property_utils::IsLayerBackFaceVisible(
-            layer, backface_transform_id, property_trees)) {
-      return false;
-    }
+  if (layer->should_check_backface_visibility() &&
+      IsLayerBackFaceVisible(layer, property_trees)) {
+    return false;
   }
 
   return true;
@@ -1377,7 +1361,7 @@ bool NodeMayContainBackdropBlurFilter(const EffectNode& node) {
 
 }  // namespace
 
-bool CC_EXPORT LayerShouldBeSkippedForDrawPropertiesComputation(
+bool LayerShouldBeSkippedForDrawPropertiesComputation(
     LayerImpl* layer,
     const PropertyTrees* property_trees) {
   const TransformTree& transform_tree = property_trees->transform_tree();
@@ -1403,37 +1387,15 @@ bool CC_EXPORT LayerShouldBeSkippedForDrawPropertiesComputation(
     return true;
   if (layer->layer_tree_impl()->settings().enable_backface_visibility_interop) {
     return layer->should_check_backface_visibility() &&
-           IsLayerBackFaceVisible(layer, layer->transform_tree_index(),
-                                  property_trees);
+           IsLayerBackFaceVisible(layer, property_trees);
   } else {
     return effect_node->hidden_by_backface_visibility;
   }
 }
 
-bool CC_EXPORT IsLayerBackFaceVisible(LayerImpl* layer,
-                                      int transform_tree_index,
-                                      const PropertyTrees* property_trees) {
-  if (layer->layer_tree_impl()->settings().enable_backface_visibility_interop) {
-    return IsTransformToRootOf3DRenderingContextBackFaceVisible(
-        layer, transform_tree_index, property_trees);
-  } else {
-    return IsTargetSpaceTransformBackFaceVisible(layer, transform_tree_index,
-                                                 property_trees);
-  }
-}
-
-bool CC_EXPORT IsLayerBackFaceVisible(Layer* layer,
-                                      int transform_tree_index,
+bool IsLayerBackFaceVisibleForTesting(const LayerImpl* layer,  // IN-TEST
                                       const PropertyTrees* property_trees) {
-  if (layer->layer_tree_host()
-          ->GetSettings()
-          .enable_backface_visibility_interop) {
-    return IsTransformToRootOf3DRenderingContextBackFaceVisible(
-        layer, transform_tree_index, property_trees);
-  } else {
-    return IsTargetSpaceTransformBackFaceVisible(layer, transform_tree_index,
-                                                 property_trees);
-  }
+  return IsLayerBackFaceVisible(layer, property_trees);
 }
 
 void ConcatInverseSurfaceContentsScale(const EffectNode* effect_node,

diff --git a/cc/trees/draw_property_utils.h b/cc/trees/draw_property_utils.h
@@ -75,13 +75,9 @@ bool CC_EXPORT LayerShouldBeSkippedForDrawPropertiesComputation(
     LayerImpl* layer,
     const PropertyTrees* property_trees);
 
-bool CC_EXPORT IsLayerBackFaceVisible(LayerImpl* layer,
-                                      int transform_tree_index,
-                                      const PropertyTrees* property_trees);
-
-bool CC_EXPORT IsLayerBackFaceVisible(Layer* layer,
-                                      int transform_tree_index,
-                                      const PropertyTrees* property_trees);
+bool CC_EXPORT
+IsLayerBackFaceVisibleForTesting(const LayerImpl* layer,
+                                 const PropertyTrees* property_trees);
 
 #if DCHECK_IS_ON()
 // Checks and logs if double background blur exists in any layers. Returns

diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc b/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
@@ -1226,6 +1226,24 @@ void FragmentPaintPropertyTreeBuilder::UpdateIndividualTransform(
         if (object_.HasHiddenBackface()) {
           state.backface_visibility =
               TransformPaintPropertyNode::BackfaceVisibility::kHidden;
+        } else if (RuntimeEnabledFeatures::
+                       BackfaceVisibilityNewInheritanceEnabled()) {
+          if (!context_.can_inherit_backface_visibility ||
+              style.Has3DTransformOperation()) {
+            // We want to set backface-visibility back to visible, if the
+            // parent doesn't allow this element to inherit backface visibility
+            // (e.g. if the parent preserves 3d), or this element has a
+            // syntactically-3D transform in *any* of the transform properties
+            // (not just 'transform'). This means that backface-visibility on
+            // an ancestor element no longer affects this element.
+            state.backface_visibility =
+                TransformPaintPropertyNode::BackfaceVisibility::kVisible;
+          } else {
+            // Otherwise we want to inherit backface-visibility.
+            DCHECK_EQ(
+                state.backface_visibility,
+                TransformPaintPropertyNode::BackfaceVisibility::kInherited);
+          }
         } else if (state.direct_compositing_reasons !=
                    CompositingReason::kNone) {
           // The above condition fixes a CompositeAfterPaint regression
@@ -2663,7 +2681,12 @@ void FragmentPaintPropertyTreeBuilder::UpdateScrollAndScrollTranslation() {
       // scrolling contents.
       if (object_.HasTransform() && object_.StyleRef().BackfaceVisibility() ==
                                         EBackfaceVisibility::kHidden) {
-        state.flags.delegates_to_parent_for_backface = true;
+        if (RuntimeEnabledFeatures::BackfaceVisibilityNewInheritanceEnabled()) {
+          CHECK_EQ(state.backface_visibility,
+                   TransformPaintPropertyNode::BackfaceVisibility::kInherited);
+        } else {
+          state.flags.delegates_to_parent_for_backface = true;
+        }
       }
 
       auto effective_change_type = properties_->UpdateScrollTranslation(
@@ -3109,6 +3132,12 @@ void FragmentPaintPropertyTreeBuilder::UpdateForChildren() {
   }
 #endif
 
+  // Child transform nodes should not inherit backface visibility if the parent
+  // transform node preserves 3d. This is before UpdatePerspective() because
+  // perspective itself doesn't affect backface visibility inheritance.
+  context_.can_inherit_backface_visibility =
+      context_.should_flatten_inherited_transform;
+
   if (properties_) {
     UpdateInnerBorderRadiusClip();
     UpdateOverflowClip();

diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder.h b/third_party/blink/renderer/core/paint/paint_property_tree_builder.h
@@ -146,6 +146,12 @@ struct PaintPropertyTreeBuilderFragmentContext {
   // be updated whenever |transform| is; flattening only needs to happen
   // to immediate children.
   bool should_flatten_inherited_transform = false;
+
+  // Whether newly created child Transform nodes can inherit
+  // backface-visibility from the parent. Some situations (e.g. having 3d
+  // transform operations) of the child can override this flag.
+  bool can_inherit_backface_visibility = false;
+
   // Rendering context for 3D sorting. See
   // TransformPaintPropertyNode::renderingContextId.
   unsigned rendering_context_id = 0;

diff --git a/third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h b/third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h
@@ -17,6 +17,7 @@
 #include "third_party/blink/renderer/platform/graphics/paint/paint_property_node.h"
 #include "third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
+#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "ui/gfx/geometry/point3_f.h"
 #include "ui/gfx/geometry/transform.h"
@@ -76,8 +77,9 @@ class PLATFORM_EXPORT TransformPaintPropertyNode
  public:
   enum class BackfaceVisibility : unsigned char {
     // backface-visibility is not inherited per the css spec. However, for an
-    // element that don't create a new plane, for now we let the element
-    // inherit the parent backface-visibility.
+    // element that don't create a new plane, we let the element inherit the
+    // parent backface-visibility and use the parent's transform to determine
+    // whether the backface is facing forward.
     kInherited,
     // backface-visibility: hidden for the new plane.
     kHidden,
@@ -370,6 +372,9 @@ class PLATFORM_EXPORT TransformPaintPropertyNode
   }
 
   bool DelegatesToParentForBackface() const {
+    if (RuntimeEnabledFeatures::BackfaceVisibilityNewInheritanceEnabled()) {
+      return state_.backface_visibility == BackfaceVisibility::kInherited;
+    }
     return state_.flags.delegates_to_parent_for_backface;
   }
 

diff --git a/third_party/blink/renderer/platform/runtime_enabled_features.json5 b/third_party/blink/renderer/platform/runtime_enabled_features.json5
@@ -406,6 +406,12 @@
     {
       name: "BackfaceVisibilityInterop",
     },
+    // This is a killswitch for the fix for crbug.com/954591 and should be
+    // removable a few weeks after M120 ships.
+    {
+      name: "BackfaceVisibilityNewInheritance",
+      status: "stable",
+    },
     {
       name: "BackForwardCache",
       base_feature: "none",

diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations
@@ -650,12 +650,6 @@ crbug.com/997202 virtual/threaded-no-composited-antialiasing/animations/skew-not
 # Only one pixel difference (187,187,187) vs (188,188,188) occasionally.
 crbug.com/1207960 [ Linux ] compositing/perspective-interest-rect.html [ Failure Pass ]
 
-crbug.com/954591 external/wpt/css/css-transforms/composited-under-rotateY-180deg.html [ Failure ]
-crbug.com/954591 external/wpt/css/css-transforms/composited-under-rotateY-180deg-clip.html [ Failure ]
-
-crbug.com/1261905 external/wpt/css/css-transforms/backface-visibility-hidden-child-will-change-transform.html [ Failure ]
-crbug.com/1261905 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-child-translate.html [ Failure ]
-
 # CompositeAfterPaint remaining failures
 # Outline paints incorrectly with columns. Needs LayoutNGBlockFragmentation.
 # Need to force the video to be composited in this case, or change pre-CAP
@@ -2767,10 +2761,8 @@ crbug.com/626703 [ Win10.20h2 ] virtual/keepalive-in-browser-migration/external/
 crbug.com/626703 [ Win ] virtual/shared-storage-fenced-frame-mparch-selecturl-limit/external/wpt/shared-storage-selecturl-limit/run-url-selection-operation-limit-multiple-origins.tentative.https.sub.html [ Timeout ]
 crbug.com/626703 [ Mac13 ] virtual/third-party-storage-partitioning/external/wpt/IndexedDB/idbobjectstore_getKey.any.html [ Timeout ]
 crbug.com/626703 external/wpt/css/css-transforms/backface-visibility-hidden-003.html [ Failure ]
-crbug.com/626703 external/wpt/css/css-transforms/backface-visibility-hidden-005.html [ Failure ]
 crbug.com/626703 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-003.html [ Failure ]
 crbug.com/626703 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-004.html [ Failure ]
-crbug.com/626703 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-005.html [ Failure ]
 crbug.com/626703 external/wpt/css/motion/offset-path-ray-022.html [ Failure ]
 crbug.com/626703 external/wpt/css/css-text/white-space/white-space-collapse-discard-001.xht [ Failure ]
 crbug.com/626703 external/wpt/css/css-text/white-space/white-space-collapse-preserve-breaks-001.xht [ Failure ]
@@ -4308,10 +4300,6 @@ crbug.com/1405829 [ Mac11-arm64 ] virtual/threaded/external/wpt/css/css-transfor
 crbug.com/1405829 [ Mac12-arm64 ] virtual/threaded/external/wpt/css/css-transforms/animation/canvas-webgl-translate-in-animation.html [ Failure Timeout ]
 crbug.com/1405829 [ Mac13-arm64 ] virtual/threaded/external/wpt/css/css-transforms/animation/canvas-webgl-translate-in-animation.html [ Failure Timeout ]
 
-crbug.com/1008483 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-004.tentative.html [ Pass ]
-crbug.com/1008483 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-005.tentative.html [ Pass ]
-crbug.com/1008483 virtual/backface-visibility-interop/external/wpt/css/css-transforms/backface-visibility-hidden-animated-002.html [ Pass ]
-
 # Tests that expect the DetailsStyling feature to be disabled:
 crbug.com/1469418 external/wpt/html/rendering/the-details-element/details-display-property-is-ignored.html [ Failure ]
 crbug.com/1469418 virtual/details-styling-disabled/external/wpt/html/rendering/the-details-element/details-display-property-is-ignored.html [ Pass ]

diff --git a/...y/blink/web_tests/external/wpt/css/css-transforms/backface-visibility-hidden-005.html.ini b/...y/blink/web_tests/external/wpt/css/css-transforms/backface-visibility-hidden-005.html.ini