Element jumps to wrong position after perspective change on ancestor

• Component: Compositing
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

https://codepen.io/smfr/pen/pozMXLo shows a bug where a layer jumps to the wrong place.

[Radar WebKit Bug Importer]

<rdar://problem/55930710>

[Simon Fraser (smfr)]

This only happens if the element with the perspective has no other property that triggers a RenderLayer, so its layer is disappearing.

[Simon Fraser (smfr)]

This seems to be about the location() of the RenderLayer being wrong.

[Simon Fraser (smfr)]

We never hit RenderLayer::updateLayerPositions() after this type of style change.

[Simon Fraser (smfr)]

We detect that the RenderStyle "hasAutoZIndex" changes, so end up with diff=StyleDifference::RepaintLayer but that isn't enough to trigger layer position updates.

There's code in RenderStyle::diff() that makes assumptions about what style changes will trigger RenderLayer creation/destruction, but that's duplicating code in the various requiresLayer() functions. I think we need to detect RenderLayer tree mutations during style change more explicitly.

[Simon Fraser (smfr)]

Antti, how would you feel about allowing renderer styleWill/DidChange to get at the current RenderTreeUpdater (via Document?) so they can set a "layer tree changed" bit for a given render tree update?

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #6)
> Antti, how would you feel about allowing renderer styleWill/DidChange to get
> at the current RenderTreeUpdater (via Document?) so they can set a "layer
> tree changed" bit for a given render tree update?

Sounds terrible. I'd like to see some sort of patch to understand the problem better.

[Simon Fraser (smfr)]

Created attachment 384460 [details]
Patch

[Antti Koivisto]

Comment on attachment 384460 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:896
> +void RenderView::layerChildrenChangedDuringStyleChange(RenderLayer& layer)
> +{
> +    if (!m_styleChangeLayerMutationRoot) {
> +        m_styleChangeLayerMutationRoot = makeWeakPtr(layer);
> +        return;
> +    }
> +
> +    RenderLayer* commonAncestor = m_styleChangeLayerMutationRoot->commonAncestorWithLayer(layer);
> +    m_styleChangeLayerMutationRoot = makeWeakPtr(commonAncestor);
> +}
> +
> +RenderLayer* RenderView::takeStyleChangeLayerTreeMutationRoot()
> +{
> +    auto* result = m_styleChangeLayerMutationRoot.get();
> +    m_styleChangeLayerMutationRoot.clear();
> +    return result;
> +}

Should this go to the layer tree? It is bit weird to have this exclusively layer related code in RenderView. Don't we have any suitable root object?

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #9)
> Should this go to the layer tree? It is bit weird to have this exclusively
> layer related code in RenderView. Don't we have any suitable root object?

We don't.

Maybe RenderLayerCompositor should have a base class that's the layer tree root object? Not in this patch though.

[Simon Fraser (smfr)]

Created attachment 384467 [details]
Patch

[Antti Koivisto]

Comment on attachment 384467 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:812
> +void FrameView::styleDidChange()
> +{
> +    ScrollView::styleDidChange();
> +    RenderView* renderView = this->renderView();
> +    if (!renderView)
> +        return;
> +
> +    RenderLayer* layerTreeMutationRoot = renderView->takeStyleChangeLayerTreeMutationRoot();
> +    if (layerTreeMutationRoot && !needsLayout())
> +        layerTreeMutationRoot->updateLayerPositionsAfterStyleChange();
> +}

Can this code go to RenderView::styleDidChange where this function is invoked? You'll need to expose fewer functions (no takeStyleChangeLayerTreeMutationRoot).

> Source/WebCore/platform/ScrollView.h:143
> +    virtual void styleDidChange();

...and you avoid making this virtual.

> Source/WebCore/rendering/RenderView.cpp:880
> +void RenderView::layerChildrenChangedDuringStyleChange(RenderLayer& layer)

It feels like there is a need for a new root type where this sort of stuff can live.

> Source/WebCore/rendering/RenderView.cpp:891
> +RenderLayer* RenderView::takeStyleChangeLayerTreeMutationRoot()

Non-ownership transferring take() is bit surprising.

[Antti Koivisto]

I suppose not all styleDidChange calls go via RenderView though they could.

[Antti Koivisto]

Comment on attachment 384467 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerModelObject.cpp:-174
> -                layer()->updateLayerPositionsAfterStyleChange();

Can you remove updateLayerPositionsAfterStyleChange() after this too?

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #14)
> Comment on attachment 384467 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384467&action=review
> 
> > Source/WebCore/rendering/RenderLayerModelObject.cpp:-174
> > -                layer()->updateLayerPositionsAfterStyleChange();
> 
> Can you remove updateLayerPositionsAfterStyleChange() after this too?

No, it's still the right entry point for post-style-change-with-no-pending-layout.

Ideally this function would move to some kind of tree owner too.

[Simon Fraser (smfr)]

Created attachment 384477 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384477 [details]
Patch

Clearing flags on attachment: 384477

Committed r252935: <https://trac.webkit.org/changeset/252935>