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

VERIFIED FIXED in Firefox 28

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: jimm, Assigned: roc)

Tracking

({regression})

28 Branch
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 unaffected, firefox28+ verified, firefox29+ verified, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [metro] [beta28] [layout] r=ff28, )

Attachments

(9 attachments, 4 obsolete attachments)

3.25 KB, image/png
Details
685 bytes, text/html
Details
1.27 KB, patch
Details | Diff | Splinter Review
1.46 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.38 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
18.44 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.11 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
34.33 KB, patch
Details | Diff | Splinter Review
40.24 KB, patch
Details | Diff | Splinter Review
Reporter

Description

6 years ago
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
Reporter

Comment 1

6 years ago
Reporter

Comment 2

6 years ago
Posted file test case
Reporter

Updated

6 years ago
Version: 26 Branch → Trunk
Reporter

Updated

6 years ago
Whiteboard: [metro]
Reporter

Updated

6 years ago
Whiteboard: [metro] → [metro][beta28]
Blocks: 946664
Reporter

Comment 3

6 years ago
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.
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.
(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.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: Trunk → 28 Branch
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.
(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.
(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.
(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.
(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 :)
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.
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.
Component: Panning and Zooming → Layout
Summary: background-attachment: fixed images jitter and jump when touch scrolling → background-attachment: fixed images jitter and jump during async touch panning/scrolling
Whiteboard: [metro][beta28] → [metro][beta28][layout]
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.
Assignee: nobody → roc
Posted patch patch (obsolete) — Splinter Review
This is basically what's needed. It's a little late though so I'd better look at it again.
Posted patch patch v2 (obsolete) — Splinter Review
Sorry, it's the weekend and I didn't bring my phones home, so I haven't tested this...
Attachment #8347208 - Attachment is obsolete: true
Attachment #8347227 - Flags: review?(chrislord.net)
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 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.
Attachment #8347227 - Attachment is obsolete: true
Attachment #8347227 - Flags: review?(chrislord.net)
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.
Attachment #8348668 - Flags: review?(matt.woodrow)
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.
Attachment #8348677 - Flags: review?(matt.woodrow)
Attachment #8348670 - Flags: review?(matt.woodrow) → review+
(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 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).
Whiteboard: [metro][beta28][layout] → [metro] [beta28] [layout]
(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.
(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.
Posted patch Part 3 v2 (obsolete) — Splinter Review
Attachment #8355404 - Flags: review?(matt.woodrow)
Attachment #8348677 - Attachment is obsolete: true
Attachment #8348677 - Flags: review?(matt.woodrow)
Attachment #8355405 - Flags: review?(matt.woodrow)
Posted patch Part 3 v3Splinter Review
-- 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.
Attachment #8355404 - Attachment is obsolete: true
Attachment #8355404 - Flags: review?(matt.woodrow)
Attachment #8355735 - Flags: review?(matt.woodrow)
tn, can you take these reviews?
Flags: needinfo?(tnikkel)
Yep.
Flags: needinfo?(tnikkel)
Attachment #8355405 - Flags: review?(tnikkel)
Attachment #8355735 - Flags: review?(tnikkel)
Let's make sure this gets uplifted to Aurora, or let Metro team know if it can't/shouldn't be.
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.
Reporter

Updated

6 years ago
Duplicate of this bug: 964184
Jim, can you see if you can reproduce with this patch?
Reporter

Comment 37

6 years ago
(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.
Attachment #8355405 - Flags: review?(tnikkel)
Attachment #8355405 - Flags: review?(matt.woodrow)
Attachment #8355405 - Flags: review+
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 :)
Attachment #8355735 - Flags: review?(tnikkel)
Attachment #8355735 - Flags: review?(matt.woodrow)
Attachment #8355735 - Flags: review+
Hi Robert, is the bug good for landing in time for the Metro uplift to Beta 28?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Whiteboard: [metro] [beta28] [layout][leave open] → [metro] [beta28] [layout]
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.
Attachment #8368909 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/a3071397da31
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
correcting target milestone, sorry for the bugmail spam
Target Milestone: mozilla30 → mozilla29
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.
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 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
Attachment #8368909 - Flags: approval-mozilla-beta?
Attachment #8368909 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(roc)
Good, because it caused a regression that I need to sort out before this uplifts.
Flags: needinfo?(roc)
Target Milestone: mozilla29 → mozilla28
The patches from this bug landed on 29; setting milestone to match. Still need a branch patch for 28 to uplift this.
Target Milestone: mozilla28 → mozilla29
Reporter

Comment 56

5 years ago
(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.
Reporter

Comment 57

5 years ago
(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.
Whiteboard: [metro] [beta28] [layout] → [metro] [beta28] [layout] r=ff28
(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.

Updated

5 years ago
Keywords: verifyme
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?
QA Contact: manuela.muntean
Reporter

Comment 60

5 years ago
(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.
Reporter

Updated

5 years ago
Blocks: 970942
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.
Flags: needinfo?(roc)
I think we have a handle on all the regressions now that bug 967587 and bug 969354 are fixed.
Flags: needinfo?(roc)
(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
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.
[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
Attachment #8380417 - Flags: approval-mozilla-beta?
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 on attachment 8380417 [details] [diff] [review]
rollup patch for b2g28

Better to take this risk earlier so we have several betas still to evaluate.
Attachment #8380417 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This was rebased on top of b2g28 rather than mozilla-beta and has conflicts. Roc, I assume you'll handle landing this?
oops yeah, I'll handle that
Flags: needinfo?(roc)
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.
I ported all my tests from trunk to beta and verified that, with bug 976412, they pass.
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.)
Attachment #8382064 - Flags: approval-mozilla-beta?
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.
Attachment #8382064 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
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?
Flags: needinfo?(jmathies)
Reporter

Comment 77

5 years ago
(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.
Flags: needinfo?(jmathies)
Marking this verified as fixed based on comment 76 and comment 77. Thanks Jim!
Status: RESOLVED → VERIFIED
Verified as fixed on latest Aurora (build ID: 20140304004003) using a Microsoft Surface Pro 2 device running Windows 8.1 64bit.
Depends on: 1176355
You need to log in before you can comment on or make changes to this bug.