Bug 235420 - Position:fixed layers shouldn't allocate a backing buffer if all children are offscreen
Summary: Position:fixed layers shouldn't allocate a backing buffer if all children are...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-20 14:04 PST by Matt Woodrow
Modified: 2022-01-23 21:30 PST (History)
11 users (show)

See Also:


Attachments
Patch (6.39 KB, patch)
2022-01-20 14:14 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (7.55 KB, patch)
2022-01-20 16:50 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (7.62 KB, patch)
2022-01-20 18:09 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2022-01-21 11:58 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (8.01 KB, patch)
2022-01-23 17:41 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (8.17 KB, patch)
2022-01-23 19:52 PST, Matt Woodrow
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Woodrow 2022-01-20 14:04:09 PST
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.
Comment 1 Matt Woodrow 2022-01-20 14:14:40 PST
Created attachment 449611 [details]
Patch
Comment 2 Matt Woodrow 2022-01-20 16:50:37 PST
Created attachment 449625 [details]
Patch
Comment 3 Simon Fraser (smfr) 2022-01-20 17:12:08 PST
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.
Comment 4 Matt Woodrow 2022-01-20 18:09:04 PST
Created attachment 449629 [details]
Patch
Comment 5 Matt Woodrow 2022-01-21 11:58:43 PST
Created attachment 449679 [details]
Patch
Comment 6 Matt Woodrow 2022-01-23 17:41:51 PST
Created attachment 449774 [details]
Patch
Comment 7 Darin Adler 2022-01-23 17:59:45 PST
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);
Comment 8 Matt Woodrow 2022-01-23 19:52:29 PST
Created attachment 449777 [details]
Patch
Comment 9 EWS 2022-01-23 20:49:36 PST
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].