Commit 396fda95 authored by Sam Fortiner's avatar Sam Fortiner Committed by Commit Bot

Update fixed-pos bounds expansion for overlap

The max overlap for fixed feature (crrev.com/766618) had an assumption
that the overlap testing rect, as computed by
BoundingBoxForCompositingOverlapTest, is in viewport space and includes
the area that a fixed-pos layer can occupy given any valid scroll
offset. This expansion was limited to just the fixed-pos layer and did
not include out-of-flow composited children of the fixed-pos layer. This
decision to use viewport space for the expanded rect caused an
unrecognized embedding of the scroll offset into the rect position at
compositing inputs time when the expanded bounding rect was computed.
The dependency on scroll offset could cause incorrect overlap test
results when the scroll offset changed and overlap testing re-ran
without a compositing inputs update. This was addressed by moving the
fixed-pos layer's expanded bounds to absolute space in crrev.com/807955.
However, this later revision also caused the children of a fixed-pos
element to change from storing their unexpanded overlap test bounding
rects in viewport space to storing them in absolute space without
expanding their bounds or otherwise compensating for no compositing
inputs update on scroll offset. This effectively introduced a dependency
on the scroll offset at bounds computation time for children of
fixed-pos, leading to crbug.com/1137974 due to incorrect overlap testing
results. This was addressed in m87 by reverting revision
crrev.com/807955.

This change addresses the issue in a different way. The new approach is
as follows:

1. Fixed-pos (and their children) compute non-expanded bounds instead of
expanded bounds in BoundingBoxForCompositingOverlapTest, now renamed to
LocalBoundingBoxForCompositingOverlapTest. These bounds, for fixed-pos
layers, are kept in viewport coords when stored on
AncestorDependentCompositingInputs, keeping them scrolloffset agnostics
as cached. Additionally, by avoiding expanding the bounds, the return
values from (un)ClippedAbsoluteBoundingBox are no longer expanded. These
return values are used by layer squashing sparsity calculations, meaning
this change reverts that calculation to its previous behavior before the
MaxOverlapBoundsForFixed feature was introduced.

2. Conversion to absolute coords and expansion happens at overlap
testing time, instead of compositing inputs update time, avoiding the
problem that was attempted to be addressed via crrev.com/807955.
Additionally, children of fixed-pos elements are also converted from
viewport to absolute space during overlap testing, avoiding the issue
found in crbug.com/1137974.

3. Only the top-most fixed-pos element expands its bounds. Without this
limitation, children of a fixed-pos element would also expand their
bounds and could detect overlap with siblings that they couldn't
actually overlap with. E.g. two absolutely positioned siblings with a
common fixed-pos ancestor do not need to expand their bounds to
determine if they can overlap.

4. Because of the change in number 3, it becomes important that the
fixed-pos layer's bound for overlap includes all that area covered by
the fixed-pos layer and any of its children that may have their own
composition layer. Without this consideration, it's possible for a child
to extend beyond the fixed-pos expanded bounds and not cause something
to composite due to overlap even though it intersects when scroll
offsets change. This would be fixed on a lifecycle update that re-ran
compositing with the new scroll offsets, but is still incorrect
behavior. To address this issue, the overlap testing bounds for the
top-most fixed-pos layer is expanded to include its composited stacking
children.

All of these cases also have tests added to validate them.

Bug: 1124753
Change-Id: I42b22892fb0040bde8cbabb7c85fe30f71f4ee57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2522407Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: default avatarChris Harrelson <chrishtr@chromium.org>
Commit-Queue: Sam Fortiner <samfort@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#827547}
parent fb131969
...@@ -463,13 +463,17 @@ void CompositingInputsUpdater::UpdateAncestorDependentCompositingInputs( ...@@ -463,13 +463,17 @@ void CompositingInputsUpdater::UpdateAncestorDependentCompositingInputs(
properties.unclipped_absolute_bounding_box = properties.unclipped_absolute_bounding_box =
EnclosingIntRect(geometry_map_->AbsoluteRect( EnclosingIntRect(geometry_map_->AbsoluteRect(
layer->BoundingBoxForCompositingOverlapTest())); layer->LocalBoundingBoxForCompositingOverlapTest()));
bool affected_by_scroll = root_layer_->GetScrollableArea() && bool affected_by_scroll = root_layer_->GetScrollableArea() &&
layer->IsAffectedByScrollOf(root_layer_); layer->IsAffectedByScrollOf(root_layer_);
// At ths point, |unclipped_absolute_bounding_box| is in viewport space. // At this point, |unclipped_absolute_bounding_box| is in viewport space.
// To convert to absolute space, add scroll offset for non-fixed layers. // To convert to absolute space, add scroll offset for non-fixed layers.
// Content that is not affected by scroll, e.g. fixed-pos content and
// children of that content, stays in viewport space so we can expand its
// bounds during overlap testing without having a dependency on the scroll
// offset at the time these properties are calculated.
if (affected_by_scroll) { if (affected_by_scroll) {
properties.unclipped_absolute_bounding_box.Move( properties.unclipped_absolute_bounding_box.Move(
RoundedIntSize(root_layer_->GetScrollableArea()->GetScrollOffset())); RoundedIntSize(root_layer_->GetScrollableArea()->GetScrollOffset()));
......
...@@ -352,23 +352,8 @@ void CompositingRequirementsUpdater::UpdateRecursive( ...@@ -352,23 +352,8 @@ void CompositingRequirementsUpdater::UpdateRecursive(
unclipped_descendants.push_back(layer); unclipped_descendants.push_back(layer);
} }
IntRect abs_bounds = use_clipped_bounding_rect IntRect abs_bounds = layer->ExpandedBoundingBoxForCompositingOverlapTest(
? layer->ClippedAbsoluteBoundingBox() use_clipped_bounding_rect);
: layer->UnclippedAbsoluteBoundingBox();
if (!RuntimeEnabledFeatures::CompositingOptimizationsEnabled()) {
PaintLayer* root_layer = layout_view_.Layer();
// |abs_bounds| does not include root scroller offset. For the purposes
// of overlap, this only matters for fixed-position objects, and their
// relative position to other elements. Therefore, it's still correct to,
// instead of adding scroll to all non-fixed elements, add a reverse scroll
// to ones that are fixed.
if (root_layer->GetScrollableArea() &&
!layer->IsAffectedByScrollOf(root_layer)) {
abs_bounds.Move(
RoundedIntSize(root_layer->GetScrollableArea()->GetScrollOffset()));
}
}
absolute_descendant_bounding_box = abs_bounds; absolute_descendant_bounding_box = abs_bounds;
if (layer_can_be_composited && current_recursion_data.testing_overlap_ && if (layer_can_be_composited && current_recursion_data.testing_overlap_ &&
......
...@@ -461,12 +461,17 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient { ...@@ -461,12 +461,17 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient {
const PhysicalRect& damage_rect, const PhysicalRect& damage_rect,
const PhysicalOffset& offset_from_root) const; const PhysicalOffset& offset_from_root) const;
// MaybeIncludeTransformForAncestorLayer means that a transform on
// |ancestorLayer| may be applied to the bounding box, in particular if
// paintsWithTransform() is true.
enum CalculateBoundsOptions { enum CalculateBoundsOptions {
kIncludeClipsAndMaybeIncludeTransformForAncestorLayer, // Include clips between this layer and its ancestor layer (inclusive).
kIncludeTransformsAndCompositedChildLayers, kIncludeClips = 0x1,
// Include transforms, irrespective of if they are applied via composition
// or painting.
kIncludeTransforms = 0x2,
// Include child layers (recursive) whether composited or not.
kIncludeCompositedChildLayers = 0x4,
// Include transform for the ancestor layer (|composited_layer| in the
// initial call) if |PaintsWithTransform|
kMaybeIncludeTransformForAncestorLayer = 0x8
}; };
// Bounding box relative to some ancestor layer. Pass offsetFromRoot if known. // Bounding box relative to some ancestor layer. Pass offsetFromRoot if known.
...@@ -475,10 +480,11 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient { ...@@ -475,10 +480,11 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient {
PhysicalRect PhysicalBoundingBox(const PaintLayer* ancestor_layer) const; PhysicalRect PhysicalBoundingBox(const PaintLayer* ancestor_layer) const;
PhysicalRect FragmentsBoundingBox(const PaintLayer* ancestor_layer) const; PhysicalRect FragmentsBoundingBox(const PaintLayer* ancestor_layer) const;
PhysicalRect BoundingBoxForCompositingOverlapTest() const; PhysicalRect LocalBoundingBoxForCompositingOverlapTest() const;
IntRect ExpandedBoundingBoxForCompositingOverlapTest(
bool use_clipped_bounding_rect) const;
PhysicalRect BoundingBoxForCompositing() const; PhysicalRect BoundingBoxForCompositing() const;
// Static position is set in parent's coordinate space. // Static position is set in parent's coordinate space.
LayoutUnit StaticInlinePosition() const { return static_inline_position_; } LayoutUnit StaticInlinePosition() const { return static_inline_position_; }
LayoutUnit StaticBlockPosition() const { return static_block_position_; } LayoutUnit StaticBlockPosition() const { return static_block_position_; }
...@@ -804,7 +810,14 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient { ...@@ -804,7 +810,14 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient {
const PaintLayer* nearest_contained_layout_layer = nullptr; const PaintLayer* nearest_contained_layout_layer = nullptr;
// These two boxes do not include any applicable scroll offset of the // These two boxes do not include any applicable scroll offset of the
// root PaintLayer. // root PaintLayer. Note that 'absolute' here is potentially misleading as
// the actual coordinate system depends on if this layer is affected by the
// viewport's scroll offset or not. For content that is not affected by the
// viewport scroll offsets, this ends up being a rect in viewport coords.
// For content that is affected by the viewport's scroll offset this
// coordinate system is in absolute coords.
// Note: This stores LocalBoundingBoxForCompositingOverlapTest and not the
// expanded bounds (ExpandedBoundingBoxForCompositingOverlapTest).
IntRect clipped_absolute_bounding_box; IntRect clipped_absolute_bounding_box;
IntRect unclipped_absolute_bounding_box; IntRect unclipped_absolute_bounding_box;
...@@ -1274,18 +1287,20 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient { ...@@ -1274,18 +1287,20 @@ class CORE_EXPORT PaintLayer : public DisplayItemClient {
needs_paint_phase_float_ |= layer.needs_paint_phase_float_; needs_paint_phase_float_ |= layer.needs_paint_phase_float_;
} }
bool IsTopMostNotAffectedByScrollOf(const PaintLayer* ancestor) const;
void ExpandRectForStackingChildren(const PaintLayer& composited_layer, void ExpandRectForStackingChildren(const PaintLayer& composited_layer,
PhysicalRect& result, PhysicalRect& result,
PaintLayer::CalculateBoundsOptions) const; unsigned options) const;
// The return value is in the space of |stackingParent|, if non-null, or // The return value is in the space of |stackingParent|, if non-null, or
// |this| otherwise. // |this| otherwise.
PhysicalRect BoundingBoxForCompositingInternal( PhysicalRect BoundingBoxForCompositingInternal(
const PaintLayer& composited_layer, const PaintLayer& composited_layer,
const PaintLayer* stacking_parent, const PaintLayer* stacking_parent,
CalculateBoundsOptions) const; unsigned options) const;
bool ShouldApplyTransformToBoundingBox(const PaintLayer& composited_layer, bool ShouldApplyTransformToBoundingBox(const PaintLayer& composited_layer,
CalculateBoundsOptions) const; unsigned options) const;
AncestorDependentCompositingInputs& EnsureAncestorDependentCompositingInputs() AncestorDependentCompositingInputs& EnsureAncestorDependentCompositingInputs()
const { const {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment