Closed Bug 1166301 Opened 5 years ago Closed 4 years ago

Background-attachment:fixed images jitter while scrolling with APZ on desktop

Categories

(Core :: Panning and Zooming, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: kats, Assigned: botond)

References

()

Details

(Whiteboard: [gfx-noted])

Attachments

(12 files, 2 obsolete files)

3.06 KB, text/html
Details
40 bytes, text/x-review-board-request
mattwoodrow
: review+
kats
: review+
Details
40 bytes, text/x-review-board-request
kats
: review+
Details
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
40 bytes, text/x-review-board-request
mstange
: review+
Details
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
40 bytes, text/x-review-board-request
mstange
: review+
Details
40 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
40 bytes, text/x-review-board-request
botond
: review+
mattwoodrow
: review+
Details
1016 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Attached file Test page
Load the attached page and scroll with APZ enabled on desktop. The image jitters because it is getting shifted on repaints, but it shouldn't be doing that.
The right way to fix this is to remove the nsLayoutUtils::UsesAsyncScrolling() condition from nsDisplayBackgroundImage::ShouldFixToViewport and fix the remaining bugs related to clipping + visible regions, which should be possible once we have bug 1148582.
The tricky part here is that the layer itself needs to stay fixed during scrolling, but its clip + mask layer should move. And when you have nested scroll frames, the layer should stay fixed with respect to the outer one and the clip needs to move along when scrolling both the outer and the inner one.
I think we should make the visible region just always be the entire viewport when APZ is enabled.
Attachment 8456709 [details] is a testcase with a rounded clip / mask layer.
Whiteboard: [gfx-noted]
If I correctly understand this bug, it happens at Windows too, not only FireFoxOS.
Yeah it happens on all desktop platforms.
OS: Gonk (Firefox OS) → All
Some investigation:
Looks like issue is situated at:
nsCSSRendering::ComputeBackgroundPositioningArea(...)
{
  ...
  if (NS_STYLE_BG_ATTACHMENT_FIXED == aLayer.mAttachment) {
  ...
  }
  ...
}
Some investigation:
Very strange source code is situated at:
> nsDisplayBackgroundImage::ShouldFixToViewport(...)
> {
>   // APZ doesn't (yet) know how to scroll the visible region for these type of
>   // items, so don't layerize them if it's enabled.
>   if (nsLayoutUtils::UsesAsyncScrolling(mFrame) ... )
>     return false;
>   }
>   ...
> }
If this comparison will be commented looks like jittering of fixed background image will be stoped.
Maybe some issues are situated in related code, but this comparisons can resolve current bug.
Flags: needinfo?(bugmail.mozilla)
It also causes the clip to lag behind. I'm currently looking into fixing this.
Assignee: nobody → mstange
Flags: needinfo?(bugmail.mozilla)
Blocks: 1169957
I was seeing the sidebars on facebook jitter when scrolling, which I assume is this bug.
Based on feedback from nightly users so far I'd say this and bug 1122916 are the two more noticeable regressions with APZ, so I'm bumping them to P1 to indicate that we should try to fix it sooner rather than later (relative to its siblings that are blocking bug 1178298).
Priority: -- → P1
Duplicate of this bug: 1189529
Markus and I discussed this yesterday to come up with a plan for fixing this bug and related issues with position:fixed items. I summarized the plan on this etherpad:

https://etherpad.mozilla.org/apz-background-fixed

Any feedback is welcome.
Duplicate of this bug: 1192483
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

This simple patch to layerize background images fixed to child elements (as opposed to just viewport elements) already makes things a lot better. Since these layers are annotated as being fixed, AsyncCompositionManager doesn't apply any async scroll transforms to them, and as a result they no longer jitter.

I intend to follow this up with a reftest, as well as two further correctness fixes (and corresponding tests) to round out this bug:

  - Clipping such layers correctly

  - Making sure we paint all areas of such layers that could be
    brought into view by async scrolling

While making this change, I noticed that in bug 1045864 we disabled the layerization of such background images with Basic compositing for performance reasons. With APZ, we need the layerization to happen for correctness, and so this patch undoes that change in the APZ-enabled case. Matt, does this seem reasonable to you?
Attachment #8652053 - Flags: feedback?(matt.woodrow)
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

https://reviewboard.mozilla.org/r/17059/#review15247

Ship It!
Attachment #8652053 - Flags: review+
Attachment #8652053 - Attachment description: MozReview Request: Bug 1166301 - Layerize background images fixed to child elements. r=mstange → MozReview Request: Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow
Attachment #8652053 - Flags: feedback?(matt.woodrow)
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow
Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange
Attachment #8652990 - Flags: review?(mstange)
Now with reftest. Carrying r+ from Matt on the fix patch.
Comment on attachment 8652990 [details]
MozReview Request: Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange

https://reviewboard.mozilla.org/r/17331/#review15409

Ship It!
Attachment #8652990 - Flags: review?(mstange) → review+
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow
Comment on attachment 8652990 [details]
MozReview Request: Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange

Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange
Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.
Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow

Markus, I'm going to ask Matt to review these patches to have an extra set of eyes on this code, but first I wanted to run them by you to see if I've explained things properly or missed anything.
Attachment #8654298 - Flags: feedback?(mstange)
Attachment #8654299 - Flags: feedback?
Attachment #8654299 - Flags: feedback? → feedback?(mstange)
Attachment #8654300 - Flags: feedback?(mstange)
Attachment #8654302 - Flags: feedback?(mstange)
(In reply to Pulsebot from comment #37)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/291cb35502bd
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2614ae6d32c5

I landed the first two patches as they can stand alone, and already constitute a significant improvement.

Setting leave-open while the remaining patches are reviewed.
Keywords: leave-open
Assignee: mstange → botond
Duplicate of this bug: 1169957
Duplicate of this bug: 1189746
No longer blocks: 1193055
No longer blocks: 1169957
No longer blocks: 1189746
I went through the test cases of the various duplicate bugs. They all seem to be working well with these patches, except for the site reported in bug 1197654 (https://www.amazon.com/clouddrive/home/) which looks better but still a bit jumpy. I'll investigate what's going on there.
(In reply to Botond Ballo [:botond] from comment #36)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d

The -clip and -mask reftests are failing on B2G. Investigating.
(In reply to Botond Ballo [:botond] from comment #42)
> (In reply to Botond Ballo [:botond] from comment #36)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d
> 
> The -clip and -mask reftests are failing on B2G. Investigating.

The problem is that B2G uses containerful scrolling for the RCD-RSF, but with the current patches, the clip adjustment is only done correctly with containerless scrolling.
I just tested the patches and it looks like the border radius / mask layer in attachment 8456709 [details] isn't being applied. This happens both with and without APZ in my local build on HiDPI Mac.
(In reply to Botond Ballo [:botond] from comment #43)
> (In reply to Botond Ballo [:botond] from comment #42)
> > (In reply to Botond Ballo [:botond] from comment #36)
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d
> > 
> > The -clip and -mask reftests are failing on B2G. Investigating.
> 
> The problem is that B2G uses containerful scrolling for the RCD-RSF, but
> with the current patches, the clip adjustment is only done correctly with
> containerless scrolling.

Specifically, the problem is that with containerful scrolling, there is no difference in layer tree structure between a fixed background layer attached to a child element, and a fixed position layer, so there's no way AsyncCompositionManager can know to only un-adjust the clip rect in the latter case.

To rectify this, we'll have to store a flag on the layer that tells us whether it's fixed background or fixed position.
(In reply to Markus Stange [:mstange] from comment #44)
> I just tested the patches and it looks like the border radius / mask layer
> in attachment 8456709 [details] isn't being applied. This happens both with
> and without APZ in my local build on HiDPI Mac.

The problem here is that the removal of the clip from the display item messes with the code that builds a mask layer for border-radius, causing the mask layer to never be created.
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow

Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Attachment #8654298 - Flags: feedback?(mstange)
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow

Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Attachment #8654299 - Flags: feedback?(mstange)
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.

This patch also ensures that we continue creating mask layers for fixed background layers correctly, where appropriate.
Attachment #8654300 - Flags: feedback?(mstange)
Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats

Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

This also tests that regions of a fixed background layer that may be revealed by async scrolling are painted.
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow

Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Attachment #8654302 - Flags: feedback?(mstange)
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange

Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
The updated patch series should fix the issues in comment 42 and comment 44.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca066edd80a
https://reviewboard.mozilla.org/r/17641/#review16045

::: layout/base/nsDisplayList.cpp:2807
(Diff revision 2)
> +    clipRect = nsRect(aBuilder->ToReferenceFrame(rootFrame), rootFrame->GetSize());

This is probably incorrect in the case where there's a transform between rootFrame and mFrame. For example, if you have background-attachment:fixed on an element with transform:scale(0.5), this will clip the rendering to half the viewport size.
https://reviewboard.mozilla.org/r/17641/#review16045

> This is probably incorrect in the case where there's a transform between rootFrame and mFrame. For example, if you have background-attachment:fixed on an element with transform:scale(0.5), this will clip the rendering to half the viewport size.

nsLayoutUtils::TransformRect(rootFrame, rootFrame->GetRectRelativeToSelf(), mFrame) + aBuilder->ToReferenceFrame(mFrame)
should do it.
https://reviewboard.mozilla.org/r/17641/#review16045

> nsLayoutUtils::TransformRect(rootFrame, rootFrame->GetRectRelativeToSelf(), mFrame) + aBuilder->ToReferenceFrame(mFrame)
> should do it.

Let's try that again:
nsRect rootRect = rootFrame->GetRectRelativeToSelf();
if (nsLayoutUtils::TransformRect(rootFrame, mFrame, rootRect) == nsLayoutUtils::TRANSFORM_SUCCEEDED) {
  clipRect = rootRect + aBuilder->ToReferenceFrame();
}
(In reply to Botond Ballo [:botond] from comment #57)
> The updated patch series should fix the issues in comment 42 and comment 44.
> 
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca066edd80a

The b2g reftests from comment 42 were still failing because of a silly mistake (I forgot to propagate the new layer attribute in ShadowLayerForwarder::EndTransaction()). I have this fixed locally. 

I also addressed comment 58 as suggested in comment 60.

New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31a5efbbb90
(In reply to Botond Ballo [:botond] from comment #41)
> I went through the test cases of the various duplicate bugs. They all seem
> to be working well with these patches, except for the site reported in bug
> 1197654 (https://www.amazon.com/clouddrive/home/) which looks better but
> still a bit jumpy. I'll investigate what's going on there.

It turns out that site doesn't use background-attachment:fixed; rather, it's using a parallax-like effect to scroll the background image at a slower rate than the main page content. Therefore, we shouldn't expect this bug to improve that page.
Blocks: 1193969
(In reply to Botond Ballo [:botond] from comment #61)
> New Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31a5efbbb90

Looking good! Posting patch series for review.
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow

Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Attachment #8654298 - Flags: review?(matt.woodrow)
Attachment #8654299 - Flags: review?(matt.woodrow)
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow

Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.

This patch also ensures that we continue creating mask layers for fixed background layers correctly, where appropriate.
Attachment #8654300 - Flags: review?(matt.woodrow)
Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Attachment #8656258 - Flags: review?(bugmail.mozilla)
Comment on attachment 8655185 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Attachment #8655185 - Flags: review?(matt.woodrow)
Attachment #8654297 - Flags: review?(bugmail.mozilla)
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats

Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow

Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Attachment #8655186 - Flags: review?(matt.woodrow)
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

This also tests that regions of a fixed background layer that may be revealed by async scrolling are painted.
Attachment #8654301 - Flags: review?(mstange)
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow

Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Attachment #8654302 - Flags: review?(matt.woodrow)
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange

Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Attachment #8654303 - Flags: review?(mstange)
Note to reviewers: if the order of the patches here in Bugzilla seems strange, it's because MozReview mixed them up. You can see the intended order in ReviewBoard.
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

https://reviewboard.mozilla.org/r/17645/#review16237
Attachment #8654301 - Flags: review?(mstange) → review+
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange

https://reviewboard.mozilla.org/r/17649/#review16239
Attachment #8654303 - Flags: review?(mstange) → review+
Attachment #8656258 - Flags: review?(bugmail.mozilla)
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

https://reviewboard.mozilla.org/r/18125/#review16241

::: gfx/layers/Layers.cpp:1705
(Diff revision 1)
> -    aStream << nsPrintfCString(" [isFixedPosition scrollId=%d anchor=%s]",
> +    aStream << nsPrintfCString(" [isFixedPosition scrollId=%lld anchor=%s%s]",

%lld is fine, but %s%s?
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats

https://reviewboard.mozilla.org/r/17637/#review16243
Attachment #8654297 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #77)
> Comment on attachment 8656258 [details]
> MozReview Request: Bug 1166301 - Use the correct format flags for printing
> fixed position data in the layers dump. r=kats
> 
> https://reviewboard.mozilla.org/r/18125/#review16241
> 
> ::: gfx/layers/Layers.cpp:1705
> (Diff revision 1)
> > -    aStream << nsPrintfCString(" [isFixedPosition scrollId=%d anchor=%s]",
> > +    aStream << nsPrintfCString(" [isFixedPosition scrollId=%lld anchor=%s%s]",
> 
> %lld is fine, but %s%s?

Oh, heh. The added "%s" was supposed to go into the "Store a flag on Layer..." patch, with the following hunk:

     aStream << nsPrintfCString(" [isFixedPosition scrollId=%lld anchor=%s%s]",
                      GetFixedPositionScrollContainerId(),
-                     ToString(anchor).c_str()).get();
+                     ToString(anchor).c_str(),
+                     IsFixedBackground() ? " fixedBackground" : "").get();
   }

I just didn't split it out properly.
Ah, gotcha. r+ with the split fixed.
Attachment #8654298 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow

https://reviewboard.mozilla.org/r/17639/#review16315

::: layout/base/FrameLayerBuilder.cpp:3757
(Diff revision 2)
> +        animatedGeometryRootForScrollMetadata = nsLayoutUtils::GetAnimatedGeometryRootForFrame(

A comment about this version of the function *not* taking ShouldFixToViewport() into account wouldn't hurt.
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow

https://reviewboard.mozilla.org/r/17641/#review16319
Attachment #8654299 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

https://reviewboard.mozilla.org/r/17643/#review16321

::: layout/base/FrameLayerBuilder.cpp:3185
(Diff revision 3)
> +      // data->mCommonClipCount will be zero because we removed the clip from

Assert this?
Attachment #8654300 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8655185 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

https://reviewboard.mozilla.org/r/17859/#review16329

::: gfx/layers/Layers.h:1167
(Diff revision 2)
> +                            bool aFixedBackground)

I think I'd prefer to name this 'aIsClipFixed' or 'aIsClipScrolled' (no preference as to which). 

Having the Layers API naming reflect the behaviour we want the layer to have seems better than the specific layout construct that required it.

I can probably be convinced otherwise though if you feel strongly.
Attachment #8655185 - Flags: review?(matt.woodrow)
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow

https://reviewboard.mozilla.org/r/17861/#review16333
Attachment #8655186 - Flags: review?(matt.woodrow)
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow

https://reviewboard.mozilla.org/r/17861/#review16335
Attachment #8655186 - Flags: review+
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow

https://reviewboard.mozilla.org/r/17647/#review16337
Attachment #8654302 - Flags: review?(matt.woodrow) → review+
Blocks: 1201894
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow

Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow

Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow

This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.

This patch also ensures that we continue creating mask layers for fixed background layers correctly, where appropriate.
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Attachment #8652053 - Attachment description: MozReview Request: Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow → MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Attachment #8652053 - Flags: review?(bugmail.mozilla)
Attachment #8656258 - Attachment description: MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats → MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Attachment #8656258 - Flags: review?(matt.woodrow)
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats

Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow

Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange

This also tests that regions of a fixed background layer that may be revealed by async scrolling are painted.
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow

Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange

Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Attachment #8652990 - Attachment is obsolete: true
Attachment #8655185 - Attachment is obsolete: true
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

MozReview didn't do such a great job of lining up the flags..
Attachment #8652053 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

Carrying r+.
Attachment #8656258 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

https://reviewboard.mozilla.org/r/18125/#review16437

::: gfx/layers/Layers.h:1165
(Diff revision 2)
> +   *   - |aIsClipFixed| is true if the clip rect of this layer should also

'clip' in this case also includes the mask layer as well as the clip rect, right?
Attachment #8656258 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #98)
> MozReview didn't do such a great job of lining up the flags..

In this case, that doesn't surprise me. To fix the bad split, I had to histedit the patch series once to split a patch into two, and then histedit it again to roll the split-out commit into another. Who knows what that does to the commit metadata that MozReview uses to figure out how to map patches from the updated series to patches to the original series...
(In reply to Matt Woodrow (:mattwoodrow) from comment #100)
> ::: gfx/layers/Layers.h:1165
> (Diff revision 2)
> > +   *   - |aIsClipFixed| is true if the clip rect of this layer should also
> 
> 'clip' in this case also includes the mask layer as well as the clip rect,
> right?

Yup, thanks. I updated the comment locally.
Keywords: leave-open
Duplicate of this bug: 990904
(In reply to Matt Woodrow (:mattwoodrow) from comment #83)
> Comment on attachment 8654300 [details]
> MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background
> images at the layer level rather than the display item level. r=mattwoodrow
> 
> https://reviewboard.mozilla.org/r/17643/#review16321
> 
> ::: layout/base/FrameLayerBuilder.cpp:3185
> (Diff revision 3)
> > +      // data->mCommonClipCount will be zero because we removed the clip from
> 
> Assert this?

And the Try run shows this assertion failing! Good call asking me to add it :)
(In reply to Botond Ballo [:botond] from comment #106)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #83)
> > Comment on attachment 8654300 [details]
> > MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background
> > images at the layer level rather than the display item level. r=mattwoodrow
> > 
> > https://reviewboard.mozilla.org/r/17643/#review16321
> > 
> > ::: layout/base/FrameLayerBuilder.cpp:3185
> > (Diff revision 3)
> > > +      // data->mCommonClipCount will be zero because we removed the clip from
> > 
> > Assert this?
> 
> And the Try run shows this assertion failing! Good call asking me to add it
> :)

The reason the assertions fails is that if we're in an inactive layer manager, we don't call UpdateCommonClipCount() at all [1], leaving mCommonClipCount at -1.

[1] https://dxr.mozilla.org/mozilla-central/rev/c0abc2a6e11f52761366e029eb1bae4c9864a8a3/layout/base/FrameLayerBuilder.cpp#4037
(In reply to Botond Ballo [:botond] from comment #108)
> Fixed locally, new Try push:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cfd97854f10

This looks much better, but the following Android reftest failures seem suspicious:

 REFTEST TEST-UNEXPECTED-FAIL | http://10.26.131.22:30413/tests/layout/reftests/scrolling/fixed-table-1.html | assertion count 6 is more than expected 0 assertions

 REFTEST TEST-UNEXPECTED-FAIL | http://10.26.131.22:30413/tests/layout/reftests/scrolling/fixed-opacity-2.html | assertion count 3 is more than expected 0 assertions

The log doesn't say what the assertion failures were, so I'll have to run the tests locally and see.
The assertions show up in the logcar, which is linked from the job details pane on treeherder.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #110)
> The assertions show up in the logcar, which is linked from the job details
> pane on treeherder.

Oh, thanks, I didn't know that!

Looks like the assertions are all this:

09-12 01:13:33.278 I/Gecko   ( 2103): [2103] ###!!! ASSERTION: Bounds computation mismatch: 'mContainerBounds.Contains(mAccumulatedChildBounds)', file /builds/slave/try-and-api-11-d-0000000000000/build/src/layout/base/FrameLayerBuilder.cpp, line 4784

I'm not quite sure what to make of that. Will investigate in more detail on Monday.
I bisected the patch series to determine that the assertion is caused by the "clip fixed background images at the layer level rather than the display item level" patch.

I guess the rather unusual thing we're doing in this patch (removing a clip that has been set on a display item and then setting it on a layer) is upsetting some invariants that FrameLayerBuilder expects to hold.
(In reply to Botond Ballo [:botond] from comment #112)
> I guess the rather unusual thing we're doing in this patch (removing a clip
> that has been set on a display item and then setting it on a layer) is
> upsetting some invariants that FrameLayerBuilder expects to hold.

I agree. I'd try inserting a "bounds = fixedToViewportClip.ApplyNonRoundedIntersection(bounds);" before the line "((nsRect&)mAccumulatedChildBounds).UnionRect(mAccumulatedChildBounds, bounds);".
(In reply to Markus Stange [:mstange] from comment #113)
> (In reply to Botond Ballo [:botond] from comment #112)
> > I guess the rather unusual thing we're doing in this patch (removing a clip
> > that has been set on a display item and then setting it on a layer) is
> > upsetting some invariants that FrameLayerBuilder expects to hold.
> 
> I agree. I'd try inserting a "bounds =
> fixedToViewportClip.ApplyNonRoundedIntersection(bounds);" before the line
> "((nsRect&)mAccumulatedChildBounds).UnionRect(mAccumulatedChildBounds,
> bounds);".

Thanks, Markus! That does indeed fix the problem, as shown by this Try push:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=344e7e1c21f3

Matt, does this seem reasonable to you? I'm going to fold it into the "clip fixed background images at the layer level rather than the display item level" patch, I'm just posting it separately for ease of reviewing.
Attachment #8660923 - Flags: review?(matt.woodrow)
Attachment #8660923 - Flags: review?(matt.woodrow) → review+
(In reply to Botond Ballo [:botond] from comment #114)
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=344e7e1c21f3

This Try push is clean, other than some "Android 4.0 API11+ opt" mochitest failures which are also happening without my patches [2].

I think this is ready to land!

[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=996b7e0d8b6a
Depends on: 1205630
Depends on: 1208438
Depends on: 1208457
No longer blocks: 1193934
Depends on: 1211520
Depends on: 1216580
Depends on: 1232939
Depends on: 1233590
Depends on: 1231243
You need to log in before you can comment on or make changes to this bug.