919144 - Improve handling of nsDisplayFixedPosition and other layerizing properties that don't create stacking contexts

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

CSS properties that need to set properties on layers fall into two categories: those that force their element to be a stacking context, and those that don't. The former case is relatively easy to handle: nsFrame::BuildDisplayListForChild recognizes the property as creating a stacking context and calls nsFrame::BuildDisplayListForStackingContext, which then creates a display item which returns something other than LAYER_NONE from nsDisplayItem::GetLayerState and builds a layer with the right properties set on it in nsDisplayItem::BuildLayer. The element being a stacking context means all its descendant content can be rendered as the children of a single display item.

The latter case is difficult to handle because the element's descendants may be interleaved in z-order with other content that is not affected by the CSS property, so multiple layers must be modified for a given CSS property on a given element. For position:fixed, we tried to handle this by creating multiple nsDisplayFixedPosition display items and later opportunistically merging them. This turns out to be a real pain for lots of reasons. For example it complicates invalidation, and it's complex to make sure that the right display items are created and merged. It also interacts poorly with our invariant that display item (frame, key) pairs are unique.

We already have a better way to handle the latter case, and we use it for scrolling. FrameLayerBuilder calls nsLayoutUtils::GetActiveScrolledRoot on the frame for every display item to figure out what the nearest scrolling ancestor is, and display items are clustered into ThebesLayers so that all the items in a given ThebesLayer have the same active scrolled root. Each ThebesLayer is given a transform based on the scroll position of the active scrolled root for its items. (In bug 876321 and bug 913444 I extend that so that elements with animated top/left/right/bottom and margins are also considered active scrolled roots --- and it works beautifully and simply.)

I think we can keep extending this approach to handle position:fixed, touch-action and other properties that don't create stacking contexts. Here's how it looks:
-- GetActiveScrolledRoot is generalized to compute, for each display item, a key tuple of the values that will determine properties of its ThebesLayer. The members of this tuple will be:
* The nearest geometric ancestor frame that is being scrolled, or having its position animated, or is position:fixed.
* A boolean indicating whether the display item should trigger default panning behavior when touched.
Other fields can be added as needed.
-- Items with different key tuples are forced into different layers. Thus all items in a ThebesLayer have the same key tuple.
-- FrameLayerBuilder is responsible for assigning layer properties to ThebesLayers (and other layers) based on the key tuple. These properties include a layer transform, fixed positioning properties, and some new property for touch-action.

The first thing to do here is to rework nsDisplayFixedPosition to basically get rid of the display item and generalize GetActiveScrolledRoot instead.

Comment 1

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

This isn't going to help with touch-action unless we fix 918288 somehow.

Comment 2

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

This works nicely.

Comment 3

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

Attachment: Part 1: Rename identifiers from 'active scroll(ed) root' to 'animated geometry root'

Comment 4

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

Attachment: Part 2: Force creation of a layer for every viewport with a displayport

Comment 5

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

Attachment: part 3: The displayport out parameters to GetDisplayPort and GetCriticalDisplayPort are optional, so make them optional

Comment 6

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

Attachment: Part 4: Create nsLayoutUtils::IsFixedPosFrameWithDisplayPort

Comment 7

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

Attachment: Part 5: Add ThebesLayerData::mReferenceFrame

Comment 8

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

Attachment: Part 6: nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay should call IsFixedPosFrameWithDisplayPort and make fixed-pos frames intersect their overflow-rect with the viewport's displ ayport to get th

Comment 9

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

Attachment: Part 7: Make fixed-pos frames with displayports animated geometry roots, and make FrameLayerBuilder responsible for setting fixed-pos layer parameters instead of nsDisplayFixedPositio n

This is the main patch.

Comment 10

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

Attachment: Part 8: Delete lots of code that's no longer needed

Comment 11

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

Attachment: Part 9: Move FrameLayerBuilder::ContainerParameters to mozilla::ContainerLayerParameters

Comment 12

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

Attachment: Part 10: Fold nsDisplayFixedPosition into nsDisplayStickyPosition

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 810382 [details] [diff] [review]
Part 4: Create nsLayoutUtils::IsFixedPosFrameWithDisplayPort

r=mats with a few nits:

>+++ b/layout/base/nsLayoutUtils.cpp
>+    aFrame->PresContext()->GetPresShell()->GetRootScrollFrame();

s/GetPresShell/PresShell/

>+  // Treat a fixed-pos frame as an animated geometry root if it belongs to
>+  // a viewport which has a scrollframe and a displayport

End the sentence with a period.

>+++ b/layout/base/nsLayoutUtils.h
>+   * Return true if aFrame is a fixed-pos frame and its viewport (its parent) has
>+   * a displayport. 

That suggests that all fixed-pos frames are children of the viewport.
Could you rephrase it so that it doesn't do that, but also make it clear that only
(fixed-pos) child frames of the viewport are considered (which I assume is what
you mean).  Perhaps:

     * Return true if aFrame is a fixed-pos child frame of the viewport and
     * the viewport has a displayport. 

Also, the method name IsFixedPosFrameWithDisplayPort is a bit confusing.
It suggests that the displayport belongs to the fixed-pos frame.
Perhaps IsFixedPosChildFrameInDisplayPort matches the description better?

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 810378 [details] [diff] [review]
Part 1: Rename identifiers from 'active scroll(ed) root' to 'animated geometry root'

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2069,5 @@
>      }
>  
>      bool isFixed;
>      bool forceInactive;
> +    const nsIFrame* AnimatedGeometryRoot;

animatedGeometryRoot

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 810380 [details] [diff] [review]
Part 2: Force creation of a layer for every viewport with a displayport

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

::: layout/generic/nsSubDocumentFrame.cpp
@@ +373,5 @@
>    nsIScrollableFrame *sf = presShell->GetRootScrollFrameAsScrollable();
>    bool constructZoomItem = subdocRootFrame && parentAPD != subdocAPD;
>    bool needsOwnLayer = constructZoomItem ||
> +    presContext->IsRootContentDocument() || (sf && sf->IsScrollingActive()) ||
> +    (sf && nsLayoutUtils::GetDisplayPort(presShell->GetRootScrollFrame()->GetContent()));

This is slightly different to the GetDisplayPort call a few lines further up, can we just share them instead?

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 810385 [details] [diff] [review]
Part 7: Make fixed-pos frames with displayports animated geometry roots, and make FrameLayerBuilder responsible for setting fixed-pos layer parameters instead of nsDisplayFixedPositio n

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2157,2 @@
>      bool forceInactive;
> +    const nsIFrame* animatedGeometryRoot;

Looks like this fixes my comment from earlier :)

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 810386 [details] [diff] [review]
Part 8: Delete lots of code that's no longer needed

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

::: layout/base/nsDisplayList.cpp
@@ -1678,5 @@
> -    }
> -
> -    // Check if this background layer is attachment-fixed
> -    if (mBackgroundStyle->mLayers[mLayer].mAttachment == NS_STYLE_BG_ATTACHMENT_FIXED) {
> -      aBuilder->SetHasFixedItems();

Heh, this confused me for a while. I guess this should have been in the nsDisplayCanvasBackgroundImage constructor really, we never return true for ShouldFixToViewport for non-canvas backgrounds.

Comment 18

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

(In reply to Mats Palmgren (:mats) from comment #13)
> That suggests that all fixed-pos frames are children of the viewport.
> Could you rephrase it so that it doesn't do that, but also make it clear
> that only
> (fixed-pos) child frames of the viewport are considered (which I assume is
> what
> you mean).  Perhaps:
> 
>      * Return true if aFrame is a fixed-pos child frame of the viewport and
>      * the viewport has a displayport. 
> 
> Also, the method name IsFixedPosFrameWithDisplayPort is a bit confusing.
> It suggests that the displayport belongs to the fixed-pos frame.
> Perhaps IsFixedPosChildFrameInDisplayPort matches the description better?

Yes, all good ideas.

Comment 19

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

Attachment: Part 2 v2

Comment 20

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

https://tbpl.mozilla.org/?tree=Try&rev=4de27991a6fe

https://hg.mozilla.org/integration/mozilla-inbound/rev/9820c500c65e
https://hg.mozilla.org/integration/mozilla-inbound/rev/910b46ec2248
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab45d37e8d99
https://hg.mozilla.org/integration/mozilla-inbound/rev/146367e368d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c809ffb8d08
https://hg.mozilla.org/integration/mozilla-inbound/rev/b337478518d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d20f37b046
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f7ce805487
https://hg.mozilla.org/integration/mozilla-inbound/rev/e815bda6bd08
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff63da70a65f

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/9820c500c65e
https://hg.mozilla.org/mozilla-central/rev/910b46ec2248
https://hg.mozilla.org/mozilla-central/rev/ab45d37e8d99
https://hg.mozilla.org/mozilla-central/rev/146367e368d0
https://hg.mozilla.org/mozilla-central/rev/1c809ffb8d08
https://hg.mozilla.org/mozilla-central/rev/b337478518d7
https://hg.mozilla.org/mozilla-central/rev/c7d20f37b046
https://hg.mozilla.org/mozilla-central/rev/b5f7ce805487
https://hg.mozilla.org/mozilla-central/rev/e815bda6bd08
https://hg.mozilla.org/mozilla-central/rev/ff63da70a65f

Comment 22

[Aaron Train [:aaronmt]]

This has caused all fixed-position headers on mobile-websites to jank when content is scrolled; see all the duplicates.