1874199 - Fix invalid clip of a sticky positioned item not attached to the root

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Dan Robertson (:dlrobertson)]

Summary

Bug 1854010 introduced a workaround for a pre-existing issue with clipping a sticky position item that is not attached to the root.

For more details see this comment. In particular the following quote describing what seems to be causing the sticky position item to be clipped:

• Having an animation id causes us to create a ReferenceFrame WebRender display item here
• Having a reference frame causes mAffectsClipPositioning to be set here
• mAffectsClipPositioning causes us to enter this branch in ClipManager::BeginList() and change some state related to clips and ASRs, ultimately leading to the sticky item being (incorrectly) clipped out

Steps to reproduce

1. Remove the workaround here.
   a. This should essentially return mFrame->PresContext()->IsRootContentDocumentCrossProcess()
   b. (Optional) If testing on desktop remove the ifdef here
2. Navigate to the reduced testcase from bug 1854010 (https://bug1854010.bmoattachments.org/attachment.cgi?id=9356341)
3. Ensure that the blue box is rendered.

Comment 1

[Dan Robertson (:dlrobertson)]

On second thought this should probably be a defect. While there is a workaround in place, there is a bug in the clipping logic.

Comment 2

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:tnikkel, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1874199 - Split the handling of StickyFrame and ScrollFrame in SpatialNode::update_transform(). r=gw

The two cases share some code, but the next patch will add extra logic
specific to the StickyFrame case, which makes it cleaner to split them.

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1874199 - Add support for animation properties on StickyFrame spatial nodes. r=gw

The support is currently limited to translation transforms.

Depends on D208236

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1874199 - Animate sticky elements by using an animation property on the StickyFrame rather than creating a ReferenceFrame. r=gw

This uses the support added in the previous patch to set an animation property
on a StickyFrame spatial node directly.

Depends on D208237

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1874199 - Remove the workaround added in 1854010 to limit APZ animation of sticky elements to the root. r=dlrobertson

We still limit it to Android with dynamic toolbar enabled, which is
all we currently use it for.

Depends on D208238

Comment 7

[Botond Ballo [:botond]]

Try push shows a bunch of dynamic-toolbar-sticky-* tests failing on Android, we'll need to investigate that before the patch series can land.

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)

Try push shows a bunch of dynamic-toolbar-sticky-* tests failing on Android, we'll need to investigate that before the patch series can land.

Should be fixed in updated patches, new Try push at https://treeherder.mozilla.org/jobs?repo=try&revision=944de5813a28ea49289b5fb064d7f7b62c06822e

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9756d029484d
Split the handling of StickyFrame and ScrollFrame in SpatialNode::update_transform(). r=gw
https://hg.mozilla.org/integration/autoland/rev/4c6a338c5c30
Add support for animation properties on StickyFrame spatial nodes. r=gw
https://hg.mozilla.org/integration/autoland/rev/00d4835ec154
Animate sticky elements by using an animation property on the StickyFrame rather than creating a ReferenceFrame. r=gw,dlrobertson
https://hg.mozilla.org/integration/autoland/rev/0367fe61d2c9
Remove the workaround added in 1854010 to limit APZ animation of sticky elements to the root. r=dlrobertson

Comment 10

[pstanciu]

Backed out for causing (part of commit message)

• Backout link
• Push with failures
• Failure Log

Comment 11

[Botond Ballo [:botond]]

The failing job is webrender-cargotest-macos-build, and it's failing with:

error[E0063]: missing field `transform` in initializer of `StickyFrameInfo`
    --> webrender/src/spatial_tree.rs:2062:9
     |
2062 |         StickyFrameInfo {
     |         ^^^^^^^^^^^^^^^ missing `transform`

I guess that job wasn't included in any of our Try pushes so far.

Comment 12

[Timothy Nikkel (:tnikkel)]

That's a bit of a weird job because even with mach try chooser --full you cannot select that job. try fuzzy can find it though.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/902352eef221
Split the handling of StickyFrame and ScrollFrame in SpatialNode::update_transform(). r=gw
https://hg.mozilla.org/integration/autoland/rev/04fe90830b82
Add support for animation properties on StickyFrame spatial nodes. r=gw
https://hg.mozilla.org/integration/autoland/rev/1d04a25e3326
Animate sticky elements by using an animation property on the StickyFrame rather than creating a ReferenceFrame. r=gw,dlrobertson
https://hg.mozilla.org/integration/autoland/rev/78f216153c06
Remove the workaround added in 1854010 to limit APZ animation of sticky elements to the root. r=dlrobertson

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/902352eef221
https://hg.mozilla.org/mozilla-central/rev/04fe90830b82
https://hg.mozilla.org/mozilla-central/rev/1d04a25e3326
https://hg.mozilla.org/mozilla-central/rev/78f216153c06