1087956 - SetupScrollingMetadata doesn't work for position:fixed layers

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

SetupScrollingMetadata walks up the animated geometry roots for the frame, but for position:fixed frames this will go immediately to the viewport frame.

GetScrollableFrameFor returns null for this, so we don't call ComputeFrameMetrics, and don't end up setting a clip on the layer.

I'm seeing this when enabling displayports on desktop, and a position:fixed layer causes the scrollbars to be removed by occlusion culling.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Clip fixed position layers

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8510712 [details] [diff] [review]
Clip fixed position layers

Review of attachment 8510712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks to me like this clip rect will be wiped out by a clip rect set in ContainerState::ProcessDisplayItems after calling SetFixedPositionLayerData :-(.

Needs a test!

We need a sane way to coalesce multiple contributions to the clip rect, in a way that doesn't cause unnecessary IPC churn. Any ideas?

Comment 3

[Matt Woodrow (:mattwoodrow)]

> Looks to me like this clip rect will be wiped out by a clip rect set in
> ContainerState::ProcessDisplayItems after calling SetFixedPositionLayerData
> :-(.

Oh, indeed it will. It doesn't for optimized ImageLayers!

> 
> Needs a test!

We have a test! But it relies on non-overlay scrollbars. It shows up on windows with my tiling patches, which is how I found this.


> We need a sane way to coalesce multiple contributions to the clip rect, in a
> way that doesn't cause unnecessary IPC churn. Any ideas?

For this, we could just have SetFixedPositionLayerData return the clip rect as an outparam and the callers can combine that with their clip rect before setting it on the layer.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Clip fixed position layers alternate

Alternate fix, suggested by tn.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8510912 [details] [diff] [review]
Clip fixed position layers alternate

Review of attachment 8510912 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea, just a few nits...

::: layout/base/nsDisplayList.cpp
@@ +615,5 @@
>      }
> +
> +    // Always clip fixed items to the root scroll frame's scrollport, because
> +    // we skip setting the clip in ScrollFrameHelper if we have a displayport.
> +    nsIFrame* rootScroll = aFrame->PresContext()->PresShell()->GetRootScrollFrame();

Use GetRootScrollFrameAsScrollable

@@ +618,5 @@
> +    // we skip setting the clip in ScrollFrameHelper if we have a displayport.
> +    nsIFrame* rootScroll = aFrame->PresContext()->PresShell()->GetRootScrollFrame();
> +    nsIScrollableFrame* scrollable = do_QueryFrame(rootScroll);
> +
> +    nsRect clipRect = scrollable->GetScrollPortRect() + ToReferenceFrame(rootScroll);

Better test that scrollable is non-null. XUL windows don't have one, for example.

@@ +625,5 @@
> +    if (haveRadii) {
> +      clip.SetTo(clipRect, radii);
> +    } else {
> +      clip.SetTo(clipRect);
> +    }

Surely this code exists somewhere else in the tree where you can share it?

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)

> Use GetRootScrollFrameAsScrollable

We need both the nsIFrame* and the nsIScrollable*

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Clip fixed position layers alternate

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

landing w/ permission: https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd1e60fd265

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0cd1e60fd265