Closed Bug 1078316 Opened 5 years ago Closed 5 years ago

[Flame] scrolling when zoomed in causes the scroll bar to oscillate

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: njpark, Assigned: kats)

References

Details

Attachments

(3 files, 4 obsolete files)

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
Blocks: 995519
Blocks: 995519
Blocks: 1078373
No longer blocks: 1078373
[Blocking Requested - why for this release]:

If we want bug 995519 uplifted for 2.1, we need this bug fixed as well.
blocking-b2g: --- → 2.1?
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.
Depends on: 1076447
Attached patch Patch to fix oscillation (obsolete) — Splinter Review
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.
Attached patch Patch to fix oscillation (obsolete) — Splinter Review
Slightly improved version that doesn't regress subframes.
Attachment #8501827 - Attachment is obsolete: true
Attached patch Patch to fix oscillation (obsolete) — Splinter Review
Attachment #8501834 - Attachment is obsolete: true
Attachment #8501856 - Flags: review?(tnikkel)
Attachment #8501856 - Flags: review?(botond)
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.
Attachment #8501856 - Flags: review?(botond) → review+
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?
Attached patch Patch to fix oscillation (v2) (obsolete) — Splinter Review
Attachment #8501856 - Attachment is obsolete: true
Attachment #8501856 - Flags: review?(tnikkel)
Attachment #8502046 - Flags: review?(tnikkel)
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 :)
Assignee: nobody → bugmail.mozilla
Attachment #8502046 - Attachment is obsolete: true
Attachment #8502046 - Flags: review?(tnikkel)
Attachment #8502153 - Flags: review?(tnikkel)
Attachment #8502153 - Flags: review?(tnikkel) → review+
blocking-b2g: 2.1? → 2.1+
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
Attachment #8502153 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7fce65c28e7c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Attachment #8502153 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Blocks: 1078373
No longer depends on: 1076447
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.
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker) → needinfo?(dharris)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(dharris)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.