STR: Open browser, and go to http://people.mozilla.org/~kgupta/grid.html Zoom in slightly, and scroll horizontally. Expected: the scroll bar moves smoothly. Actual: the scroll bar oscillates while moving. As can be seen in below video, it happens while vertically scrolling as well. Video: https://www.dropbox.com/s/7k9taucduxpsqfo/flickering_scrollbar.mp4?dl=0 https://www.dropbox.com/s/idda9ltgwgeb3z/scrollbar_oscillating_vertical.mp4?dl=0 Version Info: Gaia-Rev 470826d13ae130a5c3d572d1029e595105485fb0 Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/e0d714f43edc Build-ID 20141006040204 Version 35.0a1 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141006.074035 FW-Date Mon Oct 6 07:40:45 EDT 2014 Bootloader L1TC00011840

Correct link for 2nd video: https://www.dropbox.com/s/idda9ltgwgeb3zb/scrollbar_oscillating_vertical.mp4?dl=0

[Blocking Requested - why for this release]: If we want bug 995519 uplifted for 2.1, we need this bug fixed as well.

I took a look at this issue, and I suspect it is a layout issue. If you zoom out all the way on the grid.html page the vertical scrollbar *should* be the entire height, because you can't scroll the page vertically. However the vertical scrollbar only shows up at the top of the page. This indicates the positioning of the scrollbar by the layout code is wrong when there is a zoom applied, even in the absence of APZ transient transforms. When zoomed out a little bit (but not all the way), scrolling around will repaint the scrollbars on repaint requests based on where layout positions them. Between repaint requests, the AsyncCompositionManager code applies a transform. If the layout positioning is not correct then the async transform code will not work properly because it assumes the layout positioning is correct. I believe this is resulting in the "oscillation" of the scrollbar, because the scrollbar keeps jumping to the layout-position on repaints which doesn't match what the async code is expecting. I suspect that the investigation into bug 1076447 will give us some insight into what's going on here, so marking it as a dependency.

Moving over wip patch from bug 1043859.

This patch fixes the oscillation problem based on what's in current master. It includes a hunk from tn's wip patch (attachment 8501334 [details] [diff] [review]), a couple of corrections in AsyncCompositionManager, and a fix to the "maxpos" attribute used in scrollbar layout.

Slightly improved version that doesn't regress subframes.

Comment on attachment 8501856 [details] [diff] [review] Patch to fix oscillation Review of attachment 8501856 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the ACM bits. ::: gfx/layers/composite/AsyncCompositionManager.cpp @@ +696,5 @@ > AsyncPanZoomController* apzc = aContent.GetApzc(); > > Matrix4x4 asyncTransform = apzc->GetCurrentAsyncTransform(); > Matrix4x4 nontransientTransform = apzc->GetNontransientAsyncTransform(); > + Matrix4x4 transientTransform = nontransientTransform.Inverse() * asyncTransform; I should have caught this during review, sorry! @@ +718,5 @@ > + if (aScrollbarIsDescendant) { > + // In cases where the scrollbar is a descendant of the content, the > + // scrollbar gets painted at the same resolution as the content. Since the > + // coordinate space we apply this transform in includes the resolution, we > + // need to adjust for it as well here. Please add to this comment that below unapply the entire async transform, including the non-transient portion, which would normally account for the resolution.

Comment on attachment 8501856 [details] [diff] [review] Patch to fix oscillation >@@ -10633,16 +10633,19 @@ nsIPresShell::SetScrollPositionClampingScrollPortSize(nscoord aWidth, nscoord aH > { > if (!mScrollPositionClampingScrollPortSizeSet || > mScrollPositionClampingScrollPortSize.width != aWidth || > mScrollPositionClampingScrollPortSize.height != aHeight) { > mScrollPositionClampingScrollPortSizeSet = true; > mScrollPositionClampingScrollPortSize.width = aWidth; > mScrollPositionClampingScrollPortSize.height = aHeight; > >+ if (nsIFrame* rootScrollFrame = GetRootScrollFrame()) { >+ FrameNeedsReflow(rootScrollFrame, eResize, NS_FRAME_IS_DIRTY); >+ } > MarkFixedFramesForReflow(eResize); > } > } This might do extra work reflowing the scroll frame and children. Instead let's just mark the scrollbars themselves dirty, since they are reflow roots this reflow will be very predictably fast. Maybe add a function to scroll frames that marks their scrollbars dirty might be the best way to do that?

Comment on attachment 8502046 [details] [diff] [review] Patch to fix oscillation (v2) Looks good thanks. I think MarkScrollbarsDirtyForReflow is a better name. I think the calculated page increment should also use the scroll position clamping scroll port size, because that is what actually determines the length of the scroll thumb.

Right you are :)

I tested with applying all related 2.1 patches from http://people.mozilla.org/~kgupta/tmp/scrollbar/ on flame, and I could not reproduced this issue, and scroll functionality looks good as well.

Comment on attachment 8502153 [details] [diff] [review] Patch to fix oscillation (v3) Approval Request Comment [Feature/regressing bug #]: bug 995519 [User impact if declined]: root scrollbars (e.g. the ones for pages in the browser) jitter when panning around after changing the zoom level [Describe test coverage new/current, TBPL]: no-jun tested the patch rebased on 2.1, see above [Risks and why]: should be pretty low risk. some of the code is shared with desktop but should not get activated as it depends on properties that are only set on APZ-enabled environments (B2G and metro). [String/UUID change made/needed]: none

please verifyme on 2.1 after it gets uplifted and landed. Await comments here (and flag: status-b2g-2.1 ==> fixed) to track when the patch lands.

This bug is verified fixed on both Flame 2.1 and Flame 2.2 When navigating to http://people.mozilla.org/~kgupta/grid.html the scroll bar will smoothly scroll around in all directions without any flickering. Flame 2.1 KK (319mb) (Full Flash) Device: Flame 2.1 BuildID: 20141011000201 Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1 Gecko: d813d79d3eae Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf Version: 34.0a2 (2.1) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 Flame 2.2 Master KK (319mb) (Full Flash) Device: Flame 2.2 Master BuildID: 20141011040204 Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322 Gecko: 3f6a51950eb5 Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf Version: 35.0a1 (2.2 Master) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0