946502 - background-attachment: fixed images jitter and jump during async touch panning/scrolling

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jim Mathies [:jimm]]

STR: Load and touch scroll the test case. Note this doesn't reproduce when scrolling with the mouse wheel.

Here's a good test case site as well:

http://www.polygon.com/features/2013/12/2/5143856/no-girls-allowed

Comment 1

[Jim Mathies [:jimm]]

Attachment: testbackgroundimage.png

Comment 2

[Jim Mathies [:jimm]]

Attachment: test case

Comment 3

[Jim Mathies [:jimm]]

Per discussions between marcom and milan, we're requesting tracking on the remaining "big issues" related to apzc for metrofx, which is riding the 28 train. We expect these will get fixed during the long aurora train ride.

This is a really bad rendering bug that effect metro and may affect b2g as well. Requesting tracking since a lot of pages use stationary background images for their content.

Comment 4

[Matt Brubeck (:mbrubeck)]

Do we know what regressed this?  Did it happen at the same time as bug 941050?

Note to regression hunters: This is reproducible in Firefox for Metro (Windows 8 Touch), and possibly in Firefox OS and/or Android nightly builds.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Note to regression hunters: This is reproducible in Firefox for Metro
> (Windows 8 Touch), and possibly in Firefox OS and/or Android nightly builds.

Yes, Android is affected too.

Comment 6

[Chris Lord [:cwiiis]]

I would expect this to be a regression from bug 919144, much like bug 941050 was (it's the only significant change in layerisation that coincides with the breakage, I think). Cc'ing some layout peeps for comment.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #6)
> I would expect this to be a regression from bug 919144, much like bug 941050
> was (it's the only significant change in layerisation that coincides with
> the breakage, I think). Cc'ing some layout peeps for comment.

background-attachment: fixed is different from position: fixed (the element scrolls, but the background does not, like the element is a hole in the page and the background is below it and attached to the non-scrolling viewport) and is handled differently, so I'm not sure those bugs are related. So if this is a regression then the regression range would be useful indeed.

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Timothy Nikkel (:tn) from comment #7)
> (In reply to Chris Lord [:cwiiis] from comment #6)
> > I would expect this to be a regression from bug 919144, much like bug 941050
> > was (it's the only significant change in layerisation that coincides with
> > the breakage, I think). Cc'ing some layout peeps for comment.
> 
> background-attachment: fixed is different from position: fixed (the element
> scrolls, but the background does not, like the element is a hole in the page
> and the background is below it and attached to the non-scrolling viewport)
> and is handled differently, so I'm not sure those bugs are related. So if
> this is a regression then the regression range would be useful indeed.

Certainly, but the mechanism by which it stays fixed is basically the same (the layer has the 'fixed' flag set via FrameLayerBuilder, and then the async pan/zoom reconciliation code applies the inverse transform).

The regression range is definitely still useful to be certain.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #8)
> Certainly, but the mechanism by which it stays fixed is basically the same
> (the layer has the 'fixed' flag set via FrameLayerBuilder, and then the
> async pan/zoom reconciliation code applies the inverse transform).

I searched FrameLayerBuilder for "fixed", all of that code only deals with position fixed.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Timothy Nikkel (:tn) from comment #9)
> (In reply to Chris Lord [:cwiiis] from comment #8)
> > Certainly, but the mechanism by which it stays fixed is basically the same
> > (the layer has the 'fixed' flag set via FrameLayerBuilder, and then the
> > async pan/zoom reconciliation code applies the inverse transform).
> 
> I searched FrameLayerBuilder for "fixed", all of that code only deals with
> position fixed.

Checking again, you're right, but I swear their used to be a general check for whether a frame scrolls relative to the root scroll frame (or something like that) and SetIsFixedPosition used to be called based on that (which was aside from the metadata that gets set in nsDisplayList)...

It sounds too complicated for me to have imagined this :)

Comment 11

[Matt Brubeck (:mbrubeck)]

Based on testing of Android mozilla-central nightlies:
The last good nightly build is 2013-11-18-09.
The first bad nightly build is 2013-11-19-03.

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2adb62d07eb&tochange=ba9ecdea3a90

This strongly suggests that this is a regression from bug 919144.

Comment 12

[Timothy Nikkel (:tnikkel)]

Looking at the patches for bug 919144 they did delete some code that handled background attachment fixed, so that might be what Chris was thinking about.

Comment 13

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

FrameLayerBuilder calls nsLayoutUtils::GetAnimatedGeometryRootFor(nsDisplayItem*) on each display item. For background-attachment:fixed display items, nsDisplayItem::ShouldFixToViewport returns true and we specially assign that item to the viewport, in its own ThebesLayer.

Where I think we go wrong is that we're not adding the fixed-pos layer annotations to the ThebesLayers for those items anymore. Oops.

Comment 14

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

Attachment: patch

This is basically what's needed. It's a little late though so I'd better look at it again.

Comment 15

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

Attachment: patch v2

Sorry, it's the weekend and I didn't bring my phones home, so I haven't tested this...

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 8347227 [details] [diff] [review]
patch v2

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

This looks good, but I'd like to know it works before r+'ing - I'll try this out on Monday morning, but if you confirm it works before then, assume this is an r+.

Comment 17

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

Comment on attachment 8347227 [details] [diff] [review]
patch v2

I have written a rather different and more elaborate patch. This patch was ill-conceived. We need to set proper fixed-pos margins on background-attachment:fixed layers, and we should also set a proper anchor point since images do have one.

Comment 18

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

Attachment: Part 1: Now that nsDisplayCanvasBackgroundImage is just an image, it can support optimizing to an ImageLayer

Comment 19

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

Comment on attachment 8348668 [details] [diff] [review]
Part 1: Now that nsDisplayCanvasBackgroundImage is just an image, it can support optimizing to an ImageLayer

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

Actually I'm not sure about this.

Comment 20

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

Attachment: Part 2: Remove unimplemented, unused function

Comment 21

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

Attachment: Part 3: Extend FrameLayerBuilder to set fixed-pos metadata on layers created for background-attachment:fixed content

How exactly background-attachment:fixed should work with non-default background-position and APZC is unclear. Our old code did nothing other than use the CSS viewport top-left is the anchor point at all times (so zooming in makes fixed images positioned relative to the viewport bottm/right move out of view). To simplify things, I'm keeping that behavior here.

Arguably background-attachment:fixed images should align with the true CSS viewport at all times during zooming, but still be translated by APZC during panning, but that requires tweaks to the fixed-pos layers API which I don't want to do here.

Comment 22

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Comment on attachment 8348668 [details] [diff] [review]
> Part 1: Now that nsDisplayCanvasBackgroundImage is just an image, it can
> support optimizing to an ImageLayer
> 
> Review of attachment 8348668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually I'm not sure about this.

Why not?

The comment there still is true, but we could optimize the case where mColor has an alpha of 0. Or if the image is opaque and covers all of the pixels that mColor would draw.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8348677 [details] [diff] [review]
Part 3: Extend FrameLayerBuilder to set fixed-pos metadata on layers created for background-attachment:fixed content

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1695,5 @@
> +  } else {
> +    // A display item directly attached to the viewport.
> +    // For background-attachment:fixed items, the anchor point is always the
> +    // top-left of the viewport currently.
> +    viewportFrame = aFixedPosFrame;

This doesn't set a size for the anchor rect, should it?

::: layout/base/nsDisplayList.cpp
@@ +3362,5 @@
>        GetScrollPositionClampingScrollPortSize();
>    }
>  
> +  nsLayoutUtils::SetFixedPositionLayerData(layer, scrollFrame,
> +    nsRect(scrollFrame->GetOffsetTo(mStickyPosFrame) + ToReferenceFrame(), scrollFrameSize),

Is this actually computing the offset between the display list reference frame, and scrollFrame? It's not very easy to interpret.

scrollFrame should be an ancestor of mStickyPosFrame, right? So this will return a 'negative' value. It's also kinda slow since GetOffsetTo will walk up the tree looking for the frame, not find it, and then walk upwards again from mStickPosFrame.

::: layout/base/nsLayoutUtils.cpp
@@ +1216,5 @@
> +                       aContainerParameters.mYScale,
> +                     NSAppUnitsToFloatPixels(aAnchorRect.width, factor) *
> +                       aContainerParameters.mXScale,
> +                     NSAppUnitsToFloatPixels(aAnchorRect.height, factor) *
> +                       aContainerParameters.mYScale);

Would be nice to have a helper for this. Surely we convert an nsRect into float-pixels in other places too.

@@ +1220,5 @@
> +                       aContainerParameters.mYScale);
> +  // Need to transform anchorRect from the container layer's coordinate system
> +  // into aLayer's coordinate system.
> +  gfxMatrix transform2d;
> +  if (aLayer->GetTransform().Is2D(&transform2d)) {

Why do we give up for 3d transforms?

::: layout/base/nsLayoutUtils.h
@@ +382,5 @@
> +   * fixed-pos layer (i.e. the point to remain stable during zooming), based
> +   * on which of the fixed-pos frame's CSS absolute positioning offset
> +   * properties (top, left, right, bottom) are auto. aAnchorRect is relative to
> +   * aLayer's container layer (i.e. the display list reference frame for
> +   * aFixedPosFrame).

It's in the same coordinate space as the container layer. I don't think relative to is correct (especially if the container layer doesn't establish a display list reference frame).

Comment 24

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

(In reply to Matt Woodrow (:mattwoodrow) (on holiday until 20 Jan) from comment #22)
> Why not?
> 
> The comment there still is true, but we could optimize the case where mColor
> has an alpha of 0. Or if the image is opaque and covers all of the pixels
> that mColor would draw.

Right, that's why not. That patch was incorrect.

Comment 25

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

(In reply to Matt Woodrow (:mattwoodrow) (on holiday until 20 Jan) from comment #23)
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1695,5 @@
> > +  } else {
> > +    // A display item directly attached to the viewport.
> > +    // For background-attachment:fixed items, the anchor point is always the
> > +    // top-left of the viewport currently.
> > +    viewportFrame = aFixedPosFrame;
> 
> This doesn't set a size for the anchor rect, should it?

For background-attachment:fixed content, nsLayoutUtils::SetFixedPositionLayerData doesn't look at the left/top/right/bottom style properties, since that would be wrong. So it only uses the top-left of the anchor rect.

> ::: layout/base/nsDisplayList.cpp
> @@ +3362,5 @@
> >        GetScrollPositionClampingScrollPortSize();
> >    }
> >  
> > +  nsLayoutUtils::SetFixedPositionLayerData(layer, scrollFrame,
> > +    nsRect(scrollFrame->GetOffsetTo(mStickyPosFrame) + ToReferenceFrame(), scrollFrameSize),
> 
> Is this actually computing the offset between the display list reference
> frame, and scrollFrame? It's not very easy to interpret.
> 
> scrollFrame should be an ancestor of mStickyPosFrame, right? So this will
> return a 'negative' value. It's also kinda slow since GetOffsetTo will walk
> up the tree looking for the frame, not find it, and then walk upwards again
> from mStickPosFrame.

This was a mechanical change to preserve the behavior of the existing code.

As it happens, mStickyPosFrame == mFrame always, so this is equivalent to scrollFrame->GetOffsetTo(mStickyPosFrame) + mStickyPosFrame->GetOffsetToCrossDoc(ReferenceFrame()), i.e. converting scrollFrame's top-left to be relative to the reference frame. And that can be simplified to scrollFrame->GetOffsetToCrossDoc(ReferenceFrame()). I'll do that.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +1216,5 @@
> > +                       aContainerParameters.mYScale,
> > +                     NSAppUnitsToFloatPixels(aAnchorRect.width, factor) *
> > +                       aContainerParameters.mXScale,
> > +                     NSAppUnitsToFloatPixels(aAnchorRect.height, factor) *
> > +                       aContainerParameters.mYScale);
> 
> Would be nice to have a helper for this. Surely we convert an nsRect into
> float-pixels in other places too.

Yes :-).

> @@ +1220,5 @@
> > +                       aContainerParameters.mYScale);
> > +  // Need to transform anchorRect from the container layer's coordinate system
> > +  // into aLayer's coordinate system.
> > +  gfxMatrix transform2d;
> > +  if (aLayer->GetTransform().Is2D(&transform2d)) {
> 
> Why do we give up for 3d transforms?

We shouldn't really hit this. A CSS transform shouldn't occur between the layer containing the viewport and the layer for position:fixed content whose containing block is the viewport (since if there's a CSS transform on that element's ancestor, that ancestor becomes the containing block for the position:fixed content instead of the viewport). And of course it wouldn't happen for background-attachment:fixed either. I'll add an assertion here.

> ::: layout/base/nsLayoutUtils.h
> @@ +382,5 @@
> > +   * fixed-pos layer (i.e. the point to remain stable during zooming), based
> > +   * on which of the fixed-pos frame's CSS absolute positioning offset
> > +   * properties (top, left, right, bottom) are auto. aAnchorRect is relative to
> > +   * aLayer's container layer (i.e. the display list reference frame for
> > +   * aFixedPosFrame).
> 
> It's in the same coordinate space as the container layer. I don't think
> relative to is correct (especially if the container layer doesn't establish
> a display list reference frame).

Alright, I'll say it's relative to aFixedPosFrame's reference frame, which must also be the coordinate system of aLayer's parent layer.

Comment 26

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

Attachment: Part 3 v2

Comment 27

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

Attachment: Part 4: remove mStickyPosFrame

Comment 28

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

Attachment: Part 3 v3

-- Extended the check guarding SetAllDrawingAbove so that we only do it if the viewport we're fixed to actually has a displayport. Otherwise this triggers for some parts of toplevel XUL documents (since they don't have a root scroll frame).
-- reordered the code that sets the position of the anchorRect so it uses viewportFrame only after it's been properly set.

Comment 29

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

https://tbpl.mozilla.org/?tree=Try&rev=1534c10aa595

Comment 31

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

tn, can you take these reviews?

Comment 32

[Timothy Nikkel (:tnikkel)]

Yep.

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's make sure this gets uplifted to Aurora, or let Metro team know if it can't/shouldn't be.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Btw I applied these patches to my Metro build (latest m-c) and they don't appear to solve the problem. I still see jitter on the test cases in comment 0 and the background on about:start. The patches applied cleanly too, so I don't think it's a bitrot/rebase issue.

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Jim, can you see if you can reproduce with this patch?

Comment 37

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #36)
> Jim, can you see if you can reproduce with this patch?

Yeah, confirming unfortunately. This doesn't fix the problem after applying parts 1-4 and doing a fresh mach build.

Comment 38

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8355735 [details] [diff] [review]
Part 3 v3

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

::: layout/base/nsLayoutUtils.cpp
@@ +1217,5 @@
> +                       aContainerParameters.mYScale,
> +                     NSAppUnitsToFloatPixels(aAnchorRect.width, factor) *
> +                       aContainerParameters.mXScale,
> +                     NSAppUnitsToFloatPixels(aAnchorRect.height, factor) *
> +                       aContainerParameters.mYScale);

This still makes me a bit sad :)

Comment 39

[Marco Mucci [:MarcoM]]

Hi Robert, is the bug good for landing in time for the Metro uplift to Beta 28?

Comment 40

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

These patches should be able to land when the tree is open, but apparently they don't fix the bug anyway, at least on Metro.

I will retry them on Android and see if they still fix the problems I saw there.

Comment 41

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/90a811ee7752
https://hg.mozilla.org/integration/mozilla-inbound/rev/129efd1aa8ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0ee2a21c0e

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90a811ee7752
https://hg.mozilla.org/mozilla-central/rev/129efd1aa8ff
https://hg.mozilla.org/mozilla-central/rev/8c0ee2a21c0e

Comment 43

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

Attachment: Part 5: A ViewportFrame with a displayport on the root element needs t an animated geometry root

Seems to fix things for me on Android.

Comment 44

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8368909 [details] [diff] [review]
Part 5:  A ViewportFrame with a displayport on the root element needs t an animated geometry root

This fixes the problem on Metro also.

Comment 45

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3071397da31

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3071397da31

Comment 47

[Carsten Book [:Tomcat]]

correcting target milestone, sorry for the bugmail spam

Comment 48

[Kartikaya Gupta (email:[e-mail])]

So this fixes the metro issue with the firefox logo on about:home and also the test case jim attached in comment 2, but the polygon.com URL still exhibits jumpiness. I don't know if it's an evangelism issue or not - they might be moving that thing in response to scroll events or something, I'm not sure.

Regardless, this is an improvement and we should get it uplifted to 28.

Comment 49

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

On mobile polygon.com seems to send a special mobile site that doesn't use the same background. I suppose I should have tried some UA spoofing.

Comment 50

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

Comment on attachment 8368909 [details] [diff] [review]
Part 5:  A ViewportFrame with a displayport on the root element needs t an animated geometry root

This is a request for Firefox 28, for parts 2 through 5 of this bug.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 919144
User impact if declined: jittery background-attachment:fixed scrolling on mobile/Metro
Testing completed (on m-c, etc.): not much
Risk to taking this patch (and alternatives if risky): low-ish. All well-understood stuff. No real alternatives other than backing out 919144 on beta.
String or IDL/UUID changes made by this patch: none

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/682af2ea9aac
https://hg.mozilla.org/releases/mozilla-beta/rev/bd0b078aef3d
https://hg.mozilla.org/releases/mozilla-beta/rev/a3d2cf169644
https://hg.mozilla.org/releases/mozilla-beta/rev/2f0cfe4a7971

Comment 52

[Ryan VanderMeulen [:RyanVM]]

This hit bustage I wasn't sure how to fix, so I backed it out for now.
https://hg.mozilla.org/releases/mozilla-beta/rev/9eaff8e0448d

https://tbpl.mozilla.org/php/getParsedLog.php?id=34219206&tree=Mozilla-Beta

Comment 53

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

Good, because it caused a regression that I need to sort out before this uplifts.

Comment 54

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

A small part of part 3 was backed out, see https://hg.mozilla.org/integration/mozilla-inbound/rev/706a4e2b081c

Comment 55

[Kartikaya Gupta (email:[e-mail])]

The patches from this bug landed on 29; setting milestone to match. Still need a branch patch for 28 to uplift this.

Comment 56

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54)
> A small part of part 3 was backed out, see
> https://hg.mozilla.org/integration/mozilla-inbound/rev/706a4e2b081c

I've confirmed this doesn't regress the watermark jitter issue, however there are two new bugs on mc:

1) a small black rect is rendered briefly to the left of the watermark image when you first pan about:start. 
2) navigating via a top site tile causes the watermark to shift down about 30px prior to about:start unloading.

I'll confirm these regressed from the above backout. It's possible one or both are unrelated.

Comment 57

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #56)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54)
> > A small part of part 3 was backed out, see
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/706a4e2b081c
> 
> I've confirmed this doesn't regress the watermark jitter issue, however
> there are two new bugs on mc:
> 
> 1) a small black rect is rendered briefly to the left of the watermark image
> when you first pan about:start. 

This is a regression from bug 967587, filed bug 969354.

> 2) navigating via a top site tile causes the watermark to shift down about
> 30px prior to about:start unloading.

This is not related. Probably a regression in our front end, filed bug 969356.

Comment 58

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

(In reply to Jim Mathies [:jimm] from comment #57)
> This is a regression from bug 967587, filed bug 969354.

That makes sense. You can probably work around it in about:start by making the image slightly transparent or something like that.

Comment 59

[Manuela Muntean [Away]]

This issue is still reproducible with Firefox 28 beta 2 (build ID: 20140210161136) on Dell XPS 12 ultrabook with Windows 8 64-bit: I can still see the jittering and jumping by loading both the testcase and http://www.polygon.com/features/2013/12/2/5143856/no-girls-allowed

Any thoughts?

Comment 60

[Jim Mathies [:jimm]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #59)
> This issue is still reproducible with Firefox 28 beta 2 (build ID:
> 20140210161136) on Dell XPS 12 ultrabook with Windows 8 64-bit: I can still
> see the jittering and jumping by loading both the testcase and
> http://www.polygon.com/features/2013/12/2/5143856/no-girls-allowed
> 
> Any thoughts?

This hasn't uplifted to beta yet due to some regressions caused by the original landing on mc.

Also note the polygon.com case apparently wasn't fixed by this either, just the test case posted here. So we should probably file a new bug about polygon.com.

Comment 61

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get an updated patch for uplift in the next week?  This is the kind of bug we'll want to make sure we have a few weeks of testing on with Beta users before shipping.

Comment 62

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

I think we have a handle on all the regressions now that bug 967587 and bug 969354 are fixed.

Comment 63

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #62)
> I think we have a handle on all the regressions now that bug 967587 and bug
> 969354 are fixed.

bug 967587 is marked unaffected on 28 and I've asked in but 969354 for uplift to Aurora (which looks to be affected based on the status of 967587).  What about comment 60 though?  It suggests we're still missing something here to call this fixed on 28

Comment 64

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

The page http://www.polygon.com/features/2013/12/2/5143856/no-girls-allowed no longer uses fixed-pos images AFAICT. Not sure when that changed. I can't find other pages on that site that use them, though there is one page that uses an image in a position:fixed element (which should be unaffected by this bug). So I'm not sure now what exactly regressed that isn't fixed by these patches.

Comment 65

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

Attachment: rollup patch for b2g28

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 919144
User impact if declined: Jerky rendering of fixed background during async scrolling
Testing completed (on m-c, etc.): most patches have been on central for a while. Patch in bug 969354 landed recently.
Risk to taking this patch (and alternatives if risky): Uncomfortably risky but the only alternative is to backout 919144 on beta which is also risky.
String or IDL/UUID changes made by this patch: none

Comment 66

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

Sorry, forgot to mention, this patch is a rollup of the patches in this bug plus the patches in bugs 967587 and 969354 (which fix regressions from this bug).

Comment 67

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8380417 [details] [diff] [review]
rollup patch for b2g28

Better to take this risk earlier so we have several betas still to evaluate.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

This was rebased on top of b2g28 rather than mozilla-beta and has conflicts. Roc, I assume you'll handle landing this?

Comment 69

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

oops yeah, I'll handle that

Comment 70

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

Sigh. In bug 975931 I've been working on automated tests for async scrolling, which are badly needed to prevent these cascades of regressions, and while writing some tests I found that the fix in bug 969354 is broken. My patch in bug 976412 fixes it again, properly. Well at least it passes tests this time. But my confidence is a bit low at the moment.

Comment 71

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

I ported all my tests from trunk to beta and verified that, with bug 976412, they pass.

Comment 72

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

Attachment: mozilla-beta rollup of patches for bugs 946502, 967587, 969354 and 976412

I'm still not feeling particularly good about this, but it does pass a reasonable set of automated tests now. (Tests not included in this patch though.)

Comment 73

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8382064 [details] [diff] [review]
mozilla-beta rollup of patches for bugs 946502, 967587, 969354 and 976412

Hoping that Ryan is able to land this one (or Roc is around) for this to get in today's beta.  This is the last window we can work with on something this concerning/risky.  I'll be watching for this to land before giving the gtb on today's beta.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

http://www.quickmeme.com/img/4a/4a5c519396c22eef4722df1c81c2d589854e5bfe5982e1824d7d6e6d8a2650fd.jpg

https://hg.mozilla.org/releases/mozilla-beta/rev/078261348763

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/078261348763

And because KDiff3 apparently thinks it's fun to move a hunk of code to an entirely different function than it was in previously:
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/904e130b4555

Comment 76

[Manuela Muntean [Away]]

With Firefox 28 beta 7 on a Dell XPS 12 ultrabook with Windows 8 64-bit, in Metro mode: I can still see the jittering and jumping by loading http://www.polygon.com/features/2013/12/2/5143856/no-girls-allowed, but there isn't any jumping or jittering when loading the testcase.

In desktop mode, with 28 beta 7 on the same machine, both the URL and testcase work fine.

Any thoughts?

Comment 77

[Jim Mathies [:jimm]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #76)
> With Firefox 28 beta 7 on a Dell XPS 12 ultrabook with Windows 8 64-bit, in
> Metro mode: I can still see the jittering and jumping by loading
> http://www.polygon.com/features/2013/12/2/5143856/no-girls-allowed, but
> there isn't any jumping or jittering when loading the testcase.
> 
> In desktop mode, with 28 beta 7 on the same machine, both the URL and
> testcase work fine.
> 
> Any thoughts?

The polygon issue was different was refiled under dep bug 970942. If the test case is working right in metro and b2g this bug is verified fixed.

Comment 78

[Manuela Muntean [Away]]

Marking this verified as fixed based on comment 76 and comment 77. Thanks Jim!

Comment 79

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Aurora (build ID: 20140304004003) using a Microsoft Surface Pro 2 device running Windows 8.1 64bit.