Rework clipping to allow for APZ scrolled clips

RESOLVED FIXED in Firefox 54

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Depends on: 7 bugs, Blocks: 2 bugs)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments)

58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
58 bytes, text/x-review-board-request
mattwoodrow
: review+
tnikkel
: review+
Details | Review
58 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
58 bytes, text/x-review-board-request
mattwoodrow
: review+
tnikkel
: review+
Details | Review
58 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
58 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details | Review
2.01 KB, patch
mstange
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
This is needed to fix bug 1214146.

The idea is:
 - DisplayItemScrollClip currently stores both a clip and the information about what something is scrolled by. I want to separate those two concepts.
 - DisplayItemScrollClip will be removed. Instead, we will have 1) ActiveScrolledRoot and 2) DisplayItemClipChain.
 - ActiveScrolledRoot points to a scroll frame and allows traversing up the scroll frame chain.
 - DisplayItemClipChain is a linked list of clips, each clip being associated with the ActiveScrolledRoot that moves this clip.
 - Each display item has an ActiveScrolledRoot and a clip chain.
 - nsDisplayItem::GetClip returns the item of the clip chain that scrolls with the item's ASR. The separation between "regular clip" and "scroll clips" mostly goes away.
 - Tracking clips in the display list builder's clip state happens very similarly to how regular clips are currently tracked - there's a clip chain for content descendants and a clip chain for containing block descendants. These clip chains are intersected to create the combined clip chain.
 - There are strict rules for the ASR of a container item: A container item's ASR should be the innermost ASR which the item has finite clipped bounds with respect to.
 - At some point in the future, ASRs and AGRs should be reunified, but I haven't done that yet, because I needed to limit the scope of the change.
(Assignee)

Comment 1

2 years ago
I'll put the patches up for review now even though I haven't written tests yet, so that you can start reviewing them. I have most of the tests as regular html pages on my machine but I still need to convert them into reftests.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8785049 [details]
Bug 1298218 - Add the ability to know whether a background image is fixed before creating the display item.

https://reviewboard.mozilla.org/r/74372/#review72686
Attachment #8785049 - Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

2 years ago
I added a few tests.

I'll walk you through a few examples, hopefully this will make things easier to understand.

Let's start with the simplest example: a scrollable web page with an nsDisplayBorder that's scrolled.
The ASR of the nsDisplayBorder item is the ASR for the root scroll frame.

There are two clips here:
 - The viewport clip, which clips all content descendants and does not move if the root scroll frame is scrolled
 - The display port clip of the scroll frame, which clips containing block descendants and moves if the root scroll frame is scrolled.

So by the time the nsDisplayBorder is being created, the builder's clip state has:
 - mClipChainContentDescendants: the viewport clip, with mASR nullptr.
 - mClipChainContainingBlockDescendants: the display port clip, with mASR being the ASR created by the root scroll frame.
Neither of those ClipChains has a parent. Both of them point to an object on the stack, DisplayListClipState::AutoSaveRestore::mClipChain.
These two clips get intersected to the combined clip chain, which is created in the builder's arena. The leaf clip chain item is the display port clip, and its mParent is the viewport clip chain item. The nsDisplayBorder's mClip will point to the mClip field of its leaf mClipChain item, because that's the clip chain item whose ASR matches the display item's ASR.

If you now add a position:fixed element to our example page, then its nsDisplayFixedPosition will have "nullptr" (the root) as its ASR, and just the viewport clip as its clip chain and mClip. The builder's clip state's mClipChainContainingBlockDescendants was reset to null when the fixed position out-of-flow frame was entered, but its mClipChainContentDescendants remained the same and still carried the viewport clip.

If you now wrap the scrolled nsDisplayBorder in an opacity, then the nsDisplayOpacity will have the root scroll frame's ASR as its ASR, and its mClipChain will be just the viewport clip. Its mClip will be null.
The nsDisplayBorder's ASR and clip are the same as before.
The bounds of the root display list can be computed with respect to the root ASR (nullptr), because both the nsDisplayOpacity and the nsDisplayFixedPosition know their bounds with respect to the root ASR.

Now, to make it more complicated, you can move the position:fixed element inside the opacity, next to the nsDisplayBorder. This requires a change to the nsDisplayOpacity's ASR, because the nsDisplayOpacity now no longer has finite bounds with respect to the scrolled ASR - as you scroll, the position:fixed element keeps moving relative to the scrolled ASR, and it's not contained in any clip that moves together with that ASR, so it's impossible to compute finite bounds with respect to the scrolled ASR. So the nsDisplayOpacity now gets the root ASR (nullptr) as its ASR, and a null mClip and mClipChain.

Now let's remove the opacity again, and instead wrap the position:fixed element in a scrolled clip, by wrapping it in a <div style="position: absolute; top/left/width/height:...; clip: rect(auto auto auto auto)">. This creates a clip for content descendants that scrolls with the root scroll frame's ASR. When we enter the position:fixed out-of-flow frame, this clip is still in the builder's clip state's mClipChainContentDescendants, and so it will be part of the combined clip chain that we set on the nsDisplayFixedPosition. The important part here is that we keep around two separate clip chain items for the viewport clip (root ASR) and the "clip" clip (root scroll frame's ASR). This allows the leaf clip to be applied by FrameLayerBuilder using SetScrolledClip, and it makes us prerender all of the position:fixed element's contents without applying that clip prematurely. The painted layer contents are only clipped to the viewport clip - this is fine because the viewport clip does not move relative to the layer contents.

If you now re-introduce the opacity, like this:
 - opacity
    - scrolled clip
       - position:fixed
    - nsDisplayBorder
Then the nsDisplayOpacity's ASR can be the root scroll frame's ASR again, because now the opacity's contents have finite bounds with respect to that ASR. This is beneficial for layerization because it means that we don't unnecessarily split layers that are outside the opacity's bounds (I added layout/reftests/layers/fixed-pos-scrolled-clip-opacity-layerize.html to test that), and it even allows the opacity item to pull up a scrolled background color.

background-attachment:fixed for the canvas frame works very similarly to position:fixed. We pick up the dirty rect and the clip for containing block descendants from display data that we stored when we entered the viewport frame. This has the advantage that background-attachment:fixed images on the canvas frame are no longer clipped to the root scroll frame's display port clip (because the display port clip is a clip for containing block descendants which gets overwritten).
We don't do the clip overwriting for background-attachment:fixed images of non-canvas frames.
(Assignee)

Comment 26

2 years ago
I should also say something about how we map the clip chain to the layers' ScrollMetadata + ScrolledClip.

The scroll metadata struct has an mScrollClip field. This clip is optional, and specific to a scrollmetadata+layer pair. In other words, different layers can have scroll metadata for the same scroll ID but with a different mScrollClip in that scroll metadata.
A layer can also have its own clip (SetClipRect), and a scrolled clip (SetScrolledClip). These two can have different meanings depending on the layer's fixed annotation.

A layer's own clip always moves with the layer itself. If the layer is fixed with respect to some scroll frame, then that scroll frame will move neither the layer contents (i.e. affect the layer transform) nor the layer's own clip.
A layer's scrolled clip is always moved by all scroll frames in the layer's scroll metadata list, regardless of fixed annotations.
A scroll metadata clip is moved by all ancestor scroll frames, but not by the scroll frame for this scroll metadata.

If you have two nested scroll frames, scroll frame A on the outside and scroll frame B on the inside, and then a display item with ASR A (i.e. moved by scroll frame A but not by scroll frame B) and a scroll clip chain like this: clipB [B] << clipA [A] << clipRoot [root], then the layer for that item will have:
 - clipB as the layer's scrolled clip
 - scroll metadata for scroll frame B with mScrollClip clipA
 - scroll metadata for scroll frame A with mScrollClip clipRoot
 - a fixed annotation that marks it as fixed with respect to the scroll ID of scroll frame B. (This means: "not moved by B, but moved by all ancestors of B".)

Comment 27

2 years ago
mozreview-review
Comment on attachment 8785050 [details]
Bug 1298218 - Create ActiveScrolledRoot struct.

https://reviewboard.mozilla.org/r/74374/#review73780

::: layout/base/nsDisplayList.h:968
(Diff revision 2)
> +
> +    void SetCurrentActiveScrolledRoot(const ActiveScrolledRoot* aActiveScrolledRoot)
> +    {
> +      MOZ_ASSERT(!mUsed);
> +      mBuilder->mCurrentActiveScrolledRoot = aActiveScrolledRoot;
> +      mBuilder->mCurrentContainerASR = ActiveScrolledRoot::PickAncestor(

A comment here about why we update mCurrentContainerASR and don't reset it during the dtor would be useful.

It's because the container ASR needs to contain all ASRs used within the container, right? So once we've seen one that isn't, we need to update the ContainerASR, and keep that new value for the remainder of the container.

Can we ensure somehow that we don't read from the ContainerASR before we might potentially change it?

Comment 28

2 years ago
mozreview-review
Comment on attachment 8785050 [details]
Bug 1298218 - Create ActiveScrolledRoot struct.

https://reviewboard.mozilla.org/r/74374/#review74260
Attachment #8785050 - Flags: review?(matt.woodrow) → review+

Comment 29

2 years ago
mozreview-review
Comment on attachment 8785050 [details]
Bug 1298218 - Create ActiveScrolledRoot struct.

https://reviewboard.mozilla.org/r/74374/#review74290

::: layout/base/nsDisplayList.cpp:755
(Diff revision 2)
> +  // All child ASRs of parentASR that were created while this
> +  // AutoCurrentActiveScrolledRootSetter object was on the stack belong to us
> +  // now. Reparent them to asr.
> +  nsTHashtable<nsPtrHashKey<const ActiveScrolledRoot>> descendantParents;
> +  descendantParents.PutEntry(parentASR);
> +  for (size_t i = mDescendantsStartIndex; i < descendantsEndIndex; i++) {

This loop assumes that descendants always come after their ancestors in the mActiveScrolledRoots list.

That's no longer true after we've done it once (the newly inserted ancestor is at the end), so what happens if we have nested AutoCurrentActiveScrolledRootSetters that both call InsertScrollFrame?
(Assignee)

Comment 30

2 years ago
Good point, I hadn't thought about that. Will need to figure something out.

Comment 31

2 years ago
mozreview-review
Comment on attachment 8785051 [details]
Bug 1298218 - Add DisplayItemClipChain.

https://reviewboard.mozilla.org/r/74376/#review74296
Attachment #8785051 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 32

2 years ago
Hmm, actually, shouldn't that work just fine? The inner one will call InsertScrollFrame first, and then when the outer one calls InsertScrollFrame, it will also pick up the ASR added by the inner one.
Won't we miss calling IncrementDepth on all descendants of the inner one?

Comment 34

2 years ago
mozreview-review
Comment on attachment 8785053 [details]
Bug 1298218 - Use DisplayItemClipChain for tracking clips on display items.

https://reviewboard.mozilla.org/r/74380/#review74312

Looks awesome overall, really like the high level idea.

Some comments for now, running out of energy to finish it today sorry.

::: layout/base/FrameLayerBuilder.cpp:1324
(Diff revision 2)
>     */
>    PaintedLayerData NewPaintedLayerData(nsDisplayItem* aItem,
>                                         AnimatedGeometryRoot* aAnimatedGeometryRoot,
> -                                       const DisplayItemScrollClip* aScrollClip,
> -                                       const nsPoint& aTopLeft,
> -                                       bool aShouldFixToViewport);
> +                                       const ActiveScrolledRoot* aASR,
> +                                       const DisplayItemClipChain* aClipChain,
> +                                       const ActiveScrolledRoot* aScrollMetadataASR,

Adding aClipChain and aScrollMetadataASR to the function comments would be nice.

::: layout/base/FrameLayerBuilder.cpp:3945
(Diff revision 2)
> -        layerClipRect = ViewAs<ParentLayerPixel>(
> +        clipRectUntyped = clipRect.ToUnknownRect();
> -          ScaleToNearestPixels(layerClip.GetClipRect()) + mParameters.mOffset);
> -        clipRectUntyped = layerClipRect.ToUnknownRect();
>          clipPtr = &clipRectUntyped;
>        }
> -      if (*animatedGeometryRoot == item->Frame() &&
> +      

Trailing whitespace!

::: layout/generic/nsGfxScrollFrame.cpp:3504
(Diff revision 2)
>          // recompute the current animated geometry root if needed.
>          // It's too late to change the dirty rect so pass a copy.
>          nsRect copyOfDirtyRect = dirtyRect;
>          Unused << DecideScrollableLayer(aBuilder, &copyOfDirtyRect,
>                      /* aAllowCreateDisplayPort = */ false);
> +        asrSetter.InsertScrollFrame(sf);

Don't we only want to insert an ASR if DecideScrollableLayer returned true (and created an AGR)?

::: layout/xul/nsSliderFrame.cpp:379
(Diff revision 2)
>        masterList.AppendToTop(tempLists.Outlines());
>  
>        // Wrap the list to make it its own layer.
>        aLists.Content()->AppendNewToTop(new (aBuilder)
> -        nsDisplayOwnLayer(aBuilder, this, &masterList, flags, scrollTargetId,
> +        nsDisplayOwnLayer(aBuilder, this, &masterList,
> +                          aBuilder->CurrentActiveScrolledRoot(),

Why do we use the CurrentActiveScrolledRoot here, whereas nsBoxFrame uses the ContainerASR and clears clips below it?

Comment 35

2 years ago
mozreview-review
Comment on attachment 8785055 [details]
Bug 1298218 - Create nsDisplayFixedPosition if the element has a scrolled clip.

https://reviewboard.mozilla.org/r/74384/#review74324
Attachment #8785055 - Flags: review?(matt.woodrow) → review+

Comment 36

2 years ago
mozreview-review
Comment on attachment 8785056 [details]
Bug 1298218 - Apply the clip of a sticky item to the layer as a scrolled clip.

https://reviewboard.mozilla.org/r/74386/#review74326
Attachment #8785056 - Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

2 years ago
mozreview-review-reply
Comment on attachment 8785050 [details]
Bug 1298218 - Create ActiveScrolledRoot struct.

https://reviewboard.mozilla.org/r/74374/#review74290

> This loop assumes that descendants always come after their ancestors in the mActiveScrolledRoots list.
> 
> That's no longer true after we've done it once (the newly inserted ancestor is at the end), so what happens if we have nested AutoCurrentActiveScrolledRootSetters that both call InsertScrollFrame?

You were of course completely right. I've removed the parent caching and just call ActiveScrolledRoot::IsAncestor(parentASR, descendantASR) every time now.
(Assignee)

Comment 48

2 years ago
mozreview-review-reply
Comment on attachment 8785053 [details]
Bug 1298218 - Use DisplayItemClipChain for tracking clips on display items.

https://reviewboard.mozilla.org/r/74380/#review74312

> Don't we only want to insert an ASR if DecideScrollableLayer returned true (and created an AGR)?

You're right.

> Why do we use the CurrentActiveScrolledRoot here, whereas nsBoxFrame uses the ContainerASR and clears clips below it?

I've changed it to use ContainerASR and clear the clip appropriately. I wasn't using ContainerASR here before because it's very unlikely that the scrollbar thumb's contents escape to an ancestor ASR, but it's probably better to be safe.
The clip was indeed double-applied in the previous version of the patch.

Comment 49

2 years ago
mozreview-review
Comment on attachment 8786938 [details]
Bug 1298218 - Tests.

https://reviewboard.mozilla.org/r/75790/#review75338
Attachment #8786938 - Flags: review?(matt.woodrow) → review+

Comment 50

2 years ago
mozreview-review
Comment on attachment 8785053 [details]
Bug 1298218 - Use DisplayItemClipChain for tracking clips on display items.

https://reviewboard.mozilla.org/r/74380/#review75340

Looks good! Massive patch though, glad Tim is looking at it as well.

::: layout/base/FrameLayerBuilder.cpp:3822
(Diff revision 3)
> -    AnimatedGeometryRoot* animatedGeometryRootForClip = nullptr;
> +    const ActiveScrolledRoot* itemASR = nullptr;
> +    const DisplayItemClipChain* layerClipChain = nullptr;
>      if (mFlattenToSingleLayer && layerState != LAYER_ACTIVE_FORCE) {
>        forceInactive = true;
>        animatedGeometryRoot = lastAnimatedGeometryRoot;
> +      itemASR = mContainerASR;

lastAnimatedGeometryRoot isn't always the same as mContainerAnimatedGeometryRoot. Shouldn't we pick the ASR relating to that one instead?
Attachment #8785053 - Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Blocks: 1300864
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 68

2 years ago
mozreview-review
Comment on attachment 8785051 [details]
Bug 1298218 - Add DisplayItemClipChain.

https://reviewboard.mozilla.org/r/74376/#review89202
Attachment #8785051 - Flags: review?(tnikkel) → review+

Comment 70

2 years ago
mozreview-review
Comment on attachment 8785055 [details]
Bug 1298218 - Create nsDisplayFixedPosition if the element has a scrolled clip.

https://reviewboard.mozilla.org/r/74384/#review90014

::: layout/generic/nsFrame.cpp:2226
(Diff revision 5)
>    bool useStickyPosition = disp->mPosition == NS_STYLE_POSITION_STICKY &&
>      IsScrollFrameActive(aBuilder,
>                          nsLayoutUtils::GetNearestScrollableFrame(GetParent(),
>                          nsLayoutUtils::SCROLLABLE_SAME_DOC |
>                          nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN));
> -  bool useFixedPosition = nsLayoutUtils::IsFixedPosFrameInDisplayPort(this);
> +  bool useFixedPosition = disp->mPosition == NS_STYLE_POSITION_FIXED &&

Is it just the ViewportHasDisplayPort condition in IsFixedPosFrameInDisplayPort that you are trying to get around?

Do we want to do this for frames that have fixed style but that are not treated as fixed wrt the viewport because they are in a transformed subtree (and hence are absolute wrt the transformed frame)?
(Assignee)

Comment 71

2 years ago
mozreview-review-reply
Comment on attachment 8785055 [details]
Bug 1298218 - Create nsDisplayFixedPosition if the element has a scrolled clip.

https://reviewboard.mozilla.org/r/74384/#review90014

> Is it just the ViewportHasDisplayPort condition in IsFixedPosFrameInDisplayPort that you are trying to get around?
> 
> Do we want to do this for frames that have fixed style but that are not treated as fixed wrt the viewport because they are in a transformed subtree (and hence are absolute wrt the transformed frame)?

I'm trying to get around both conditions in IsFixedPosFrameInDisplayPort. In the patch with the tests, there's a test called fixed-pos-scrolled-clip-3.html, which has a scrolled clip on a fixed-inside-transform element.

In an earlier patch I was creating nsDisplayFixedPosition items for all "disp->mPosition == NS_STYLE_POSITION_FIXED" frames, but that led to lots of unnecessary extra layers and fuzzy reftest failures, so I reverted to just doing it in the cases where it was necessary for correct APZ behavior.

Comment 72

2 years ago
mozreview-review
Comment on attachment 8785053 [details]
Bug 1298218 - Use DisplayItemClipChain for tracking clips on display items.

https://reviewboard.mozilla.org/r/74380/#review90026

::: layout/base/FrameLayerBuilder.cpp:1319
(Diff revision 5)
>     * @param  aVisibleRect          The visible rect of the item.
>     * @param  aAnimatedGeometryRoot The item's animated geometry root.
> -   * @param  aScrollClip           The scroll clip for this PaintedLayer.
> +   * @param  aASR                  The active scrolled root for this PaintedLayer.
> +   * @param  aClipChain            The clip chain that the compositor needs to
> +   *                               apply to this layer.
> +   * @param  aScrollMetadataASR    The leaf ASR for which scroll metadata needs to be

How is it distinguished if the clip or the layer moves with that ASR?

::: layout/base/FrameLayerBuilder.cpp:1372
(Diff revision 5)
>    nsIFrame*                        mContainerFrame;
>    nsIFrame*                        mContainerReferenceFrame;
>    AnimatedGeometryRoot*            mContainerAnimatedGeometryRoot;
>    ContainerLayer*                  mContainerLayer;
>    nsRect                           mContainerBounds;
> -  const DisplayItemScrollClip*     mContainerScrollClip;
> +  const ActiveScrolledRoot*        mContainerASR;

The meaning and difference between these three ASRs should probably be described in a comment.

::: layout/base/FrameLayerBuilder.cpp:4752
(Diff revision 5)
> +  //    scroll frame should be scrolled by events over the layer.
> +  // A fixed layer needs to be annotated with the scroll ID of the scroll frame
> +  // that it is *fixed with respect to*, i.e. the outermost scroll frame which
> +  // does not move the layer. nsDisplayFixedPosition only ever annotates layers
> +  // with the scroll ID of the presshell's root scroll frame, which is
> +  // sometimes the wrong thing to do, so we correct it here.

When is it the wrong thing to do?

::: layout/base/FrameLayerBuilder.cpp:4814
(Diff revision 5)
> +      // which has a scrolled clip. As far as scroll metadata is concerned,
> +      // the scroll frame's scroll metadata will be a child of the scroll ID
> +      // that scrolls the clip on the fixed layer. But as far as ASRs are
> +      // concerned, those two ASRs are siblings, parented to the ASR of the
> +      // fixed layer.
> +      do {

How does that happen (them being made siblings)?

::: layout/base/FrameLayerBuilder.cpp:4871
(Diff revision 5)
>  
>      Maybe<ScrollMetadata> metadata =
> -      scrollFrame->ComputeScrollMetadata(aEntry->mLayer, mContainerReferenceFrame, mParameters, clip);
> +      scrollFrame->ComputeScrollMetadata(aEntry->mLayer, mContainerReferenceFrame,
> +                                         mParameters, clip);
>      if (!metadata) {
> -      continue;
> +      return;

Do we really want to return here without setting the metrics array on the layer?

::: layout/base/nsDisplayList.h:761
(Diff revision 5)
>     * Clone the supplied clip chain's chain items into this builder's arena.
>     */
>    const DisplayItemClipChain* CopyWholeChain(const DisplayItemClipChain* aClipChain);
>  
>    /**
> -   * Allocate a new DisplayItemClip in the arena. Will be cleaned up
> +   * Allocate a new DisplayItemClipChain in the arena. Will be cleaned up

This comment seems to be useless, there is no function after it.

::: layout/base/nsDisplayList.h:991
(Diff revision 5)
>  
>      void SetCurrentActiveScrolledRoot(const ActiveScrolledRoot* aActiveScrolledRoot)
>      {
>        MOZ_ASSERT(!mUsed);
>        mBuilder->mCurrentActiveScrolledRoot = aActiveScrolledRoot;
> +      const ActiveScrolledRoot* finiteBoundsASR = ActiveScrolledRoot::PickDescendant(

Can you explain this in a comment?

::: layout/base/nsDisplayList.cpp:2974
(Diff revision 5)
> +          // Override the dirty rect on the builder to be the dirty rect of
> +          // the viewport.
> +          // displayData->mDirtyRect is relative to the presshell's viewport
> +          // frame (the root frame), and we need it to be relative to aFrame.
> +          nsIFrame* rootFrame = aBuilder->CurrentPresShellState()->mPresShell->GetRootFrame();
> +          nsRect dirtyRect = displayData->mDirtyRect + aFrame->GetOffsetTo(rootFrame);

Can't there be transforms between the viewport frame and the frame with a bg attachment fixed background?

::: layout/generic/nsFrame.cpp:2400
(Diff revision 5)
>  #endif
>    resultList.AppendToTop(set.Outlines());
>    // 8, 9: non-negative z-index children
>    resultList.AppendToTop(set.PositionedDescendants());
>  
>    // Get the scroll clip to use for the container items that we create here.

Update the code comment?
Attachment #8785053 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 73

2 years ago
mozreview-review-reply
Comment on attachment 8785053 [details]
Bug 1298218 - Use DisplayItemClipChain for tracking clips on display items.

https://reviewboard.mozilla.org/r/74380/#review90026

> How is it distinguished if the clip or the layer moves with that ASR?

aASR always specifies that the layer moves with aASR. If it has a clip that moves with a different ASR, then that information is deterined from aClipChain.
I'll change the comment for aASR to "The active scrolled root that moves this PaintedLayer."

> The meaning and difference between these three ASRs should probably be described in a comment.

Agreed.

> When is it the wrong thing to do?

If the fixed element is inside a transform. I'll add that to the comment.

> Do we really want to return here without setting the metrics array on the layer?

Oops, good catch. This should stay a "continue". I had the inner part of the loop moved out to a helper function in an earlier version of the patch, that's why this became a return.

> This comment seems to be useless, there is no function after it.

Indeed.

> Can you explain this in a comment?

Will do.

> Can't there be transforms between the viewport frame and the frame with a bg attachment fixed background?

Not a CSS transform at least - we changed that in bug 735857. Not quite sure how this line interacts with APZ zooming.

> Update the code comment?

Will do.

Updated

2 years ago
Priority: -- → P3

Comment 74

2 years ago
mozreview-review
Comment on attachment 8785054 [details]
Bug 1298218 - Save and restore the combined clip for the "top layer".

https://reviewboard.mozilla.org/r/74382/#review95634

::: layout/base/nsDisplayList.h:1178
(Diff revision 5)
> +      , mCombinedClipChain(aCombinedClipChain)
>        , mContainingBlockActiveScrolledRoot(aContainingBlockActiveScrolledRoot)
>        , mDirtyRect(aDirtyRect)
>      {}
>      const DisplayItemClipChain* mContainingBlockClipChain;
> +    const DisplayItemClipChain* mCombinedClipChain;

Can you put a comment here that the combined clip is only needed for the special case of top layer?
Attachment #8785054 - Flags: review?(tnikkel) → review+

Comment 75

2 years ago
mozreview-review
Comment on attachment 8785057 [details]
Bug 1298218 - Add a workaround for root scroll frame container layer scrolling.

https://reviewboard.mozilla.org/r/74388/#review95638

::: layout/base/FrameLayerBuilder.cpp:3892
(Diff revision 5)
>        if (itemASR != mContainerASR) {
>          const DisplayItemClip* clip = DisplayItemClipChain::ClipForASR(item->GetClipChain(), mContainerASR);
> +        if (!gfxPrefs::LayoutUseContainersForRootFrames()) {
> -        MOZ_ASSERT(clip, "the item should have finite bounds with respect to mContainerASR.");
> +          MOZ_ASSERT(clip, "the item should have finite bounds with respect to mContainerASR.");
> -        if (clip) {
>            bounds = clip->GetClipRect();

Do we not want to execute |bounds = clip->GetClipRect()| if |gfxPrefs::LayoutUseContainersForRootFrames() && clip| is true?

::: layout/base/nsDisplayList.cpp:792
(Diff revision 5)
>        mRootAGR(aReferenceFrame, nullptr),
>        mUsedAGRBudget(0),
>        mDirtyRect(-1,-1,-1,-1),
>        mGlassDisplayItem(nullptr),
>        mScrollInfoItemsForHoisting(nullptr),
> +      mActiveScrolledRootForRootScrollframe(nullptr),

Where is the implementation of SetActiveScrolledRootForRootScrollframe?

I ask because the naive implementation would be incorrect as the places where it is called are also called for content iframes. Making it ignore all calls after the first one that with a non-null argument would fix that I think.
Attachment #8785057 - Flags: review?(tnikkel) → review+
What's the up to date status of this one?  Asking because we're tracking it as possibly bug 1311505 solution.
Flags: needinfo?(mstange)
(Assignee)

Updated

2 years ago
Blocks: 1333965
(Assignee)

Comment 77

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #76)
> What's the up to date status of this one?

I updated the patches on Friday and addressed the review comments. Need to push to try again, and there's a problem with the new mask layers for clip-path / mask-image, which I'll try to address.
Flags: needinfo?(mstange)
(Assignee)

Comment 78

2 years ago
(In reply to Timothy Nikkel (:tnikkel) from comment #75)
> ::: layout/base/nsDisplayList.cpp:792
> (Diff revision 5)
> >        mRootAGR(aReferenceFrame, nullptr),
> >        mUsedAGRBudget(0),
> >        mDirtyRect(-1,-1,-1,-1),
> >        mGlassDisplayItem(nullptr),
> >        mScrollInfoItemsForHoisting(nullptr),
> > +      mActiveScrolledRootForRootScrollframe(nullptr),
> 
> Where is the implementation of SetActiveScrolledRootForRootScrollframe?

In nsDisplayList.h.

> I ask because the naive implementation would be incorrect as the places
> where it is called are also called for content iframes. Making it ignore all
> calls after the first one that with a non-null argument would fix that I
> think.

Well, it is the naive implementation, but ScrollFrameHelper only calls SetActiveScrolledRootForRootScrollframe if mIsRoot is true. Shouldn't that address the content iframe problem?

(In reply to Timothy Nikkel (:tnikkel) from comment #72)
> Comment on attachment 8785053 [details]
> ::: layout/base/FrameLayerBuilder.cpp:4814
> (Diff revision 5)
> > +      // which has a scrolled clip. As far as scroll metadata is concerned,
> > +      // the scroll frame's scroll metadata will be a child of the scroll ID
> > +      // that scrolls the clip on the fixed layer. But as far as ASRs are
> > +      // concerned, those two ASRs are siblings, parented to the ASR of the
> > +      // fixed layer.
> > +      do {
> 
> How does that happen (them being made siblings)?

The clip's ASR is a child ASR of the root ASR. When the fixed frame is entered, we reset the current ASR to the root ASR. Then we create an ASR for the scroll frame inside the fixed frame, which uses the current ASR (the root ASR) as its parent ASR. So then the root ASR has two children.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 89

2 years ago
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/8eeb5e0b825b
Add the ability to know whether a background image is fixed before creating the display item. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/071bde29b52f
Create ActiveScrolledRoot struct. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d7b10f6c7982
Add DisplayItemClipChain. r=mattwoodrow,tnikkel
https://hg.mozilla.org/integration/autoland/rev/a97b00d08fbd
Back out bug 1284586. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2023dadca59c
Use DisplayItemClipChain for tracking clips on display items. r=mattwoodrow,tnikkel
https://hg.mozilla.org/integration/autoland/rev/faefb3b25988
Save and restore the combined clip for the "top layer". r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/59d03e3235e5
Create nsDisplayFixedPosition if the element has a scrolled clip. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/4e09e6f2c6ea
Tests. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/37283192367c
Apply the clip of a sticky item to the layer as a scrolled clip. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/5746c9b9db68
Add a workaround for root scroll frame container layer scrolling. r=tnikkel

Comment 90

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5b8ee56f0343
Disable test_frame_reconstruction.html on android for being permafail a=me

Comment 92

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/28228fbbdb1e
Disable more android tests to green things up on android a=me
(Assignee)

Comment 93

2 years ago
Thanks for disabling those tests, Wes. I'm going to look into them tomorrow.

It turns out that these tests also failed on my try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe8e438327e0a9eb80542ea012a75dc6a34de6c but I didn't pay attention to it because there was only one orange job and not two of the same - it looks like failing Android jobs don't get auto-retriggered?

(In reply to Timothy Nikkel (:tnikkel) from comment #75)
> Comment on attachment 8785057 [details]
> Bug 1298218 - Add a workaround for root scroll frame container layer
> scrolling.
> 
> https://reviewboard.mozilla.org/r/74388/#review95638
> 
> ::: layout/base/FrameLayerBuilder.cpp:3892
> (Diff revision 5)
> >        if (itemASR != mContainerASR) {
> >          const DisplayItemClip* clip = DisplayItemClipChain::ClipForASR(item->GetClipChain(), mContainerASR);
> > +        if (!gfxPrefs::LayoutUseContainersForRootFrames()) {
> > -        MOZ_ASSERT(clip, "the item should have finite bounds with respect to mContainerASR.");
> > +          MOZ_ASSERT(clip, "the item should have finite bounds with respect to mContainerASR.");
> > -        if (clip) {
> >            bounds = clip->GetClipRect();
> 
> Do we not want to execute |bounds = clip->GetClipRect()| if
> |gfxPrefs::LayoutUseContainersForRootFrames() && clip| is true?

I made this suggested change and it's probably what's causing the assertion failures. I've started a try build with this change reverted to see whether that's the case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf307343b1631538536ee25adc6034da428bbc66
Flags: needinfo?(mstange)

Comment 94

2 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/a97b00d08fbd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1312362
No longer blocks: 1311505
Duplicate of this bug: 1316840
Duplicate of this bug: 1319371
status-firefox51: affected → wontfix
status-firefox52: --- → wontfix
status-firefox53: --- → wontfix
Created attachment 8833296 [details] [diff] [review]
Follow-up to skip reftests that require APZ if no APZ is around

Right now these two tests *should* be failing in non-e10s reftest runs. But they're not (I'll file something for that). The logs from m-c show IPDL errors but the tests pass anyway, e.g:

[task 2017-02-03T13:20:28.899091Z] 13:20:28     INFO - REFTEST TEST-START | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/layers/fixed-pos-scrolled-clip-layerize.html != about:blank
[task 2017-02-03T13:20:28.899869Z] 13:20:28     INFO - REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/layers/fixed-pos-scrolled-clip-layerize.html | 1685 / 1889 (89%)
[task 2017-02-03T13:20:28.952977Z] 13:20:28     INFO - IPDL protocol error: Handler returned error code!
[task 2017-02-03T13:20:28.954126Z] 13:20:28     INFO - ###!!! [Parent][DispatchSyncMessage] Error: PLayerTransaction::Msg_SetAsyncScrollOffset Processing error: message was deserialized, but the handler returned false (indicating failure)
[task 2017-02-03T13:20:28.967190Z] 13:20:28     INFO - REFTEST TEST-PASS | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/layers/fixed-pos-scrolled-clip-layerize.html != about:blank | image comparison
[task 2017-02-03T13:20:28.968170Z] 13:20:28     INFO - REFTEST TEST-END | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/layers/fixed-pos-scrolled-clip-layerize.html != about:blank
Attachment #8833296 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Blocks: 1336516
(Assignee)

Updated

2 years ago
Depends on: 1336519
(Assignee)

Comment 100

2 years ago
Comment on attachment 8833296 [details] [diff] [review]
Follow-up to skip reftests that require APZ if no APZ is around

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

Oops, thanks.
Attachment #8833296 - Flags: review?(mstange) → review+
(Assignee)

Updated

2 years ago
Blocks: 1336530
(Assignee)

Comment 101

2 years ago
(In reply to Markus Stange [:mstange] from comment #93)
> I made this suggested change and it's probably what's causing the assertion
> failures. I've started a try build with this change reverted to see whether
> that's the case:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bf307343b1631538536ee25adc6034da428bbc66

Well, that push was even worse, so tn's suggested change was the right one. I'm just going to disable the assertion on Android so that we can re-enable the tests. I'm going to do that in bug 1336530.

Comment 102

2 years ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/feb887b2faec
Follow-up to skip APZ-requiring reftests if APZ is not present. r=mstange
(Assignee)

Updated

2 years ago
Blocks: 1336544
and the good news, talos for some improvements:

== Change summary for alert #4988 (as of February 01 2017 21:20 UTC) ==

Improvements:

  9%  tscrollx summary linux64 pgo         3.98 -> 3.62
  8%  tscrollx summary windows8-64 opt     2.41 -> 2.22
  7%  tscrollx summary linux64 opt         4.02 -> 3.72
  6%  tscrollx summary windows8-64 pgo     2.37 -> 2.24
  4%  tscrollx summary osx-10-10 opt       3.18 -> 3.07
  2%  tart summary windows8-64 pgo         6.15 -> 5.99
  2%  tart summary windows8-64 opt         7.23 -> 7.06

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4988
(Assignee)

Comment 105

2 years ago
(In reply to Joel Maher ( :jmaher) from comment #104)
> and the good news, talos for some improvements:

These are unexpected, unfortunately. I've clicked on the "graph" links and it was revealed to me that all these improvements are in the e10s version of the tests, which means APZ is on, and there's currently a known bug with APZ introduced by these patches (bug 1336544), so my guess is that these improvements will go away again once that bug is fixed.
(Assignee)

Comment 106

2 years ago
(In reply to Markus Stange [:mstange] from comment #105)
> and there's currently a known bug with APZ
> introduced by these patches (bug 1336544)

That should be bug 1336519.

Updated

2 years ago
Duplicate of this bug: 1337368
Depends on: 1358185

Updated

a year ago
Depends on: 1359233

Updated

a year ago
Depends on: 1359569

Updated

a year ago
Duplicate of this bug: 1369717
(Assignee)

Updated

a year ago
Depends on: 1339009

Updated

a year ago
Depends on: 1375455

Updated

a year ago
Depends on: 1375795
(Assignee)

Updated

a year ago
Depends on: 1379406

Updated

10 months ago
Depends on: 1399977

Updated

3 months ago
Depends on: 1452476

Updated

2 months ago
Depends on: 1427792
You need to log in before you can comment on or make changes to this bug.