Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 1 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 2 • 9 years ago

Attachment 8456709 [details] is a testcase with a rounded clip / mask layer.

	Maksim Lebedev
	Comment 3 • 9 years ago

If I correctly understand this bug, it happens at Windows too, not only FireFoxOS.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 4 • 9 years ago

Yeah it happens on all desktop platforms.

	Maksim Lebedev
	Comment 5 • 9 years ago

Some investigation:
Looks like issue is situated at:
nsCSSRendering::ComputeBackgroundPositioningArea(...)
{
  ...
  if (NS_STYLE_BG_ATTACHMENT_FIXED == aLayer.mAttachment) {
  ...
  }
  ...
}

	Maksim Lebedev
	Comment 6 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 7 • 9 years ago

It also causes the clip to lag behind. I'm currently looking into fixing this.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 9 • 9 years ago

I was seeing the sidebars on facebook jitter when scrolling, which I assume is this bug.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 10 • 9 years ago

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).

	Botond Ballo [:botond] Assignee
	Comment 14 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 18 • 9 years ago

Bug 1166301 - Layerize background images fixed to child elements. r=mstange

	Botond Ballo [:botond] Assignee
	Comment 19 • 9 years ago

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?

	Matt Woodrow (:mattwoodrow)
	Comment 20 • 9 years ago

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!

	Botond Ballo [:botond] Assignee
	Comment 21 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 22 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 23 • 9 years ago

Now with reftest. Carrying r+ from Matt on the fix patch.

	Markus Stange [:mstange]
	Comment 24 • 9 years ago

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!

	Botond Ballo [:botond] Assignee
	Comment 25 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20f55a693278

	Botond Ballo [:botond] Assignee
	Comment 26 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 27 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 28 • 9 years ago

Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats

	Botond Ballo [:botond] Assignee
	Comment 29 • 9 years ago

Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow

	Botond Ballo [:botond] Assignee
	Comment 30 • 9 years ago

Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow

	Botond Ballo [:botond] Assignee
	Comment 31 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 32 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 33 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 34 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 35 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 36 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d

	Pulsebot
	Comment 37 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/291cb35502bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2614ae6d32c5

	Botond Ballo [:botond] Assignee
	Comment 38 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 41 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 42 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 43 • 9 years ago

(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.

	Markus Stange [:mstange]
	Comment 44 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/291cb35502bd
https://hg.mozilla.org/mozilla-central/rev/2614ae6d32c5

	Botond Ballo [:botond] Assignee
	Comment 46 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 47 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 48 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 49 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 50 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 51 • 9 years ago

Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow

	Botond Ballo [:botond] Assignee
	Comment 52 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 53 • 9 years ago

Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow

	Botond Ballo [:botond] Assignee
	Comment 54 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 55 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 56 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 57 • 9 years ago

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

	Markus Stange [:mstange]
	Comment 58 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 59 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 60 • 9 years ago

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();
}

	Botond Ballo [:botond] Assignee
	Comment 61 • 9 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 62 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 63 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 64 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 65 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 66 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 67 • 9 years ago

Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats

	Botond Ballo [:botond] Assignee
	Comment 68 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 69 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 70 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 71 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 72 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 73 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 74 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 75 • 9 years ago

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

	Markus Stange [:mstange]
	Comment 76 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 77 • 9 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 78 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 79 • 9 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 80 • 9 years ago

Ah, gotcha. r+ with the split fixed.

	Matt Woodrow (:mattwoodrow)
	Comment 81 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 82 • 9 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 83 • 9 years ago

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?

	Matt Woodrow (:mattwoodrow)
	Comment 84 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 85 • 9 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 86 • 9 years ago

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

	Matt Woodrow (:mattwoodrow)
	Comment 87 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 88 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 89 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 90 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 91 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 92 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 93 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 94 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 95 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 96 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 97 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 98 • 9 years ago

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..

	Botond Ballo [:botond] Assignee
	Comment 99 • 9 years ago

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+.

	Matt Woodrow (:mattwoodrow)
	Comment 100 • 9 years ago

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?

	Botond Ballo [:botond] Assignee
	Comment 101 • 9 years ago

(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...

	Botond Ballo [:botond] Assignee
	Comment 102 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 103 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=081dfc726202

	Botond Ballo [:botond] Assignee
	Comment 105 • 9 years ago

Had some build failures, trying again:

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

	Botond Ballo [:botond] Assignee
	Comment 106 • 9 years ago

(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 :)

	Botond Ballo [:botond] Assignee
	Comment 107 • 9 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 108 • 9 years ago

Fixed locally, new Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cfd97854f10

	Botond Ballo [:botond] Assignee
	Comment 109 • 9 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 110 • 9 years ago

The assertions show up in the logcar, which is linked from the job details pane on treeherder.

	Botond Ballo [:botond] Assignee
	Comment 111 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 112 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 113 • 9 years ago

(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);".

	Botond Ballo [:botond] Assignee
	Comment 114 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 115 • 9 years ago

(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

	Pulsebot
	Comment 116 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/085754ce9f61
https://hg.mozilla.org/integration/mozilla-inbound/rev/1137bd3b5434
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d5dac07434
https://hg.mozilla.org/integration/mozilla-inbound/rev/feb8c0e441f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4932d5e081d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f713731a9f3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2fda65a34f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6373ec1f943c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ed0b24e299
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24b11e4beb2

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 117 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/085754ce9f61
https://hg.mozilla.org/mozilla-central/rev/1137bd3b5434
https://hg.mozilla.org/mozilla-central/rev/63d5dac07434
https://hg.mozilla.org/mozilla-central/rev/feb8c0e441f5
https://hg.mozilla.org/mozilla-central/rev/4932d5e081d1
https://hg.mozilla.org/mozilla-central/rev/f713731a9f3b
https://hg.mozilla.org/mozilla-central/rev/4e2fda65a34f
https://hg.mozilla.org/mozilla-central/rev/6373ec1f943c
https://hg.mozilla.org/mozilla-central/rev/e8ed0b24e299
https://hg.mozilla.org/mozilla-central/rev/d24b11e4beb2