Bug 235420 – Position:fixed layers shouldn't allocate a backing buffer if all children are offscreen

• Component: Compositing
• Status: RESOLVED FIXED

[Matt Woodrow]

rdar://86612099

Layers for position:fixed content can't scroll, so any descendants that don't intersect with the compositing bounds of the layer shouldn't be included when we're determine if we need a backing store.

[Matt Woodrow]

Created attachment 449611 [details]
Patch

[Matt Woodrow]

Created attachment 449625 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 449611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449611&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:2793
> +static bool intersectsWithAncestor(const RenderLayer& ancestor, const LayoutRect& ancestorCompositedBounds, const RenderLayer& child)

I would put child as the first argument.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2798
> +            return true;

It would be more truthful to have the function return std::optional<bool> and return nullopt here.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2802
> +    LayoutPoint delta = child.convertToLayerCoords(&ancestor, LayoutPoint(), RenderLayer::AdjustForColumns);

auto delta. Maybe also "{ }" instead of LayoutPoint().

It's unfortunate that this function is called inside a descendant layer tree walk, then your loop above, and child.convertToLayerCoords() both do ancestor tree walks, which can be a perf issue (we've seen this in the past). Maybe traverseVisibleNonCompositedDescendantLayers() can accumulate the "offset from ancestor" as it's doing the descendant traversal.

Probably fine in this case (we don't expect a lot of ancestor walking) but generally to be avoided.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2803
> +    LayoutRect overlap = child.overlapBounds();

auto

> Source/WebCore/rendering/RenderLayerBacking.cpp:2804
> +    overlap.move(delta.x(), delta.y());

overlap.moveBy(delta);

> LayoutTests/compositing/backing/no-backing-for-offscreen-children-of-position-fixed.html:19
> +  <div style="position: fixed;z-index: -1;left: 0;top: 0;right: 0;bottom: 0;opacity: 0.5;">

I don't think the z-index: -1 is relevant. Also this layer doesn't need to be full height or width (and making it so makes it harder to identify in the layer output). Also the opacity is not required.

[Matt Woodrow]

Created attachment 449629 [details]
Patch

[Matt Woodrow]

Created attachment 449679 [details]
Patch

[Matt Woodrow]

Created attachment 449774 [details]
Patch

[Darin Adler]

Comment on attachment 449774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449774&action=review

Says Simon already reviewed this. I do have a couple comments.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2795
> +    const RenderLayer* layer = &child;

This can and should be written as a for loop:

    for (auto* layer = &child; layer != &ancestor; layer = layer->parent()) {
        If (!layer->canUseOffsetFromAncestor())
            return std::nullopt;
    }

Writing it this way also makes it even clear erhow strong the dependency is on the fact that ancestor is actually an ancestor of child. Otherwise we’ll crash with a null pointer dereference.

It also is a little confusing. The function name is “canUseOffsetFromAncestor”, but for some reason it has to be called along the entire ancestor chain. It’s as if the function only really answers “can use offset from parent”.

Finally, I think this could use a comment explaining why this check is so important. If any ancestors use transforms, then the clipping computation below could be inaccurate, basically. That’s the kind of “why” thing that could go in a comment. The function name is good, but not enough alone to explain that.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2802
> +    auto delta = child.convertToLayerCoords(&ancestor, { }, RenderLayer::AdjustForColumns);

To me this seems like “offset” more than “delta”.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2805
> +    return overlap.intersects(ancestorCompositedBounds);

I would have thought we could write:

    return (child.overlapBounds()+ offset).intersects(ancestorCompositedBounds);

But perhaps we don’t have that operator+ defined.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2815
> +            std::optional<bool> mayIntersect = intersectsWithAncestor(layer, m_owningLayer, compositedBounds());

I would use value_or(true) here rather than having a local variable of type std::optional<bool>. The optional<bool> answers the question “does intersect”. Using value_or(true) turns into a boolean that answers the question “may intersect”. So the local variable name is better if you use value_or(true).

    // If the function returns nullopt because it can’t determine if this intersects with the ancestor, then it may intersect, which is why we do value_or(true).
    bool mayIntersect = intersectsWithAncestor(layer, m_owningLayer, compositedBounds()).value_or(true);

[Matt Woodrow]

Created attachment 449777 [details]
Patch

[EWS]

Committed r288429 (246320@main): <https://commits.webkit.org/246320@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449777 [details].