Closed Bug 1298218 Opened 8 years ago Closed 7 years ago

Rework clipping to allow for APZ scrolled clips

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(11 files)

Bug 1298218 - Add the ability to know whether a background image is fixed before creating the display item.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1298218 - Create ActiveScrolledRoot struct.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1298218 - Add DisplayItemClipChain.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
tnikkel
: review+
Details
Bug 1298218 - Back out bug 1284586.
58 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1298218 - Use DisplayItemClipChain for tracking clips on display items.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
tnikkel
: review+
Details
Bug 1298218 - Save and restore the combined clip for the "top layer".
58 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1298218 - Create nsDisplayFixedPosition if the element has a scrolled clip.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1298218 - Apply the clip of a sticky item to the layer as a scrolled clip.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1298218 - Add a workaround for root scroll frame container layer scrolling.
58 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1298218 - Tests.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Follow-up to skip reftests that require APZ if no APZ is around
2.01 KB, patch
mstange
: review+
Details | Diff | Splinter Review 
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.
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 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+
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.
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 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 on attachment 8785050 [details]
Bug 1298218 - Create ActiveScrolledRoot struct.

https://reviewboard.mozilla.org/r/74374/#review74260
Attachment #8785050 - Flags: review?(matt.woodrow) → 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?
Good point, I hadn't thought about that. Will need to figure something out.
Comment on attachment 8785051 [details]
Bug 1298218 - Add DisplayItemClipChain.

https://reviewboard.mozilla.org/r/74376/#review74296
Attachment #8785051 - Flags: review?(matt.woodrow) → review+
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 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 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 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 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.
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 on attachment 8786938 [details]
Bug 1298218 - Tests.

https://reviewboard.mozilla.org/r/75790/#review75338
Attachment #8786938 - Flags: review?(matt.woodrow) → 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+
Blocks: 1300864
Comment on attachment 8785051 [details]
Bug 1298218 - Add DisplayItemClipChain.

https://reviewboard.mozilla.org/r/74376/#review89202
Attachment #8785051 - Flags: review?(tnikkel) → 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)?
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 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+
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.
Priority: -- → P3
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 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)
Blocks: 1333965
(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)
(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.
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
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
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
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)
https://hg.mozilla.org/mozilla-central/rev/a97b00d08fbd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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)
Blocks: 1336516
Depends on: 1336519
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+
Blocks: 1336530
(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.
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
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
(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.
(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.
Depends on: 1358185
Depends on: 1359233
Depends on: 1359569
Depends on: 1339009
Depends on: 1375455
Depends on: 1375795
Depends on: 1379406
Depends on: 1399977
Depends on: 1427792
Depends on: 1487181
Depends on: 1493250
Depends on: 1504865
Depends on: 1529324
Regressions: 1581702
Regressions: 1672234
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: