Closed Bug 1087956 Opened 5 years ago Closed 5 years ago

SetupScrollingMetadata doesn't work for position:fixed layers

Categories

(Core :: Layout, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Clip fixed position layers (obsolete) — Splinter Review
Assignee: nobody → matt.woodrow
Attachment #8510712 - Flags: review?(roc)
Blocks: 1078994
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?
Attachment #8510712 - Flags: review?(roc) → review-
> 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.
Alternate fix, suggested by tn.
Attachment #8510912 - Flags: review?(roc)
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?
Attachment #8510912 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)

> Use GetRootScrollFrameAsScrollable

We need both the nsIFrame* and the nsIScrollable*
Attachment #8510712 - Attachment is obsolete: true
Attachment #8510912 - Attachment is obsolete: true
Attachment #8515699 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/0cd1e60fd265
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.