921020 - Abort APZC animations when content updates the scroll offset

Status: RESOLVED WORKSFORME

(Core :: Panning and Zooming, defect)

Description

[Jim Mathies [:jimm]]

Spin off from bug 907100.

There are a lot of ways to scroll content. The most common (touch on tablets) is currently handled directly by the apz through events sent via widget. Another common input method is mouse wheel scroll, which will also go directly to the apz (bug 907100).

However there are other cases where content can scroll (keyboard input, content calling scrollBy) that do not get directed to the apz directly. These changes are picked up in the metro fx front end [1], forwarded down to widget [2][3], and passed to the apz [4].

[1] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#550
[2] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/apzc.js#136
[3] http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroWidget.cpp#1535
[4] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1437

A problem here is that UpdateScrollOffset doesn't update the display port via a call to RequestContentRepaint(). 

In metrofx the display port is currently updated in the front end code thanks to our older fennec based code base. For B2G this is currently handled in TabChild, logic we'll inherit when we move back to remote content.

RequestContentRepaint calls RequestContentRepaint on the GeckoContentController [1], which is picked up down in widget [2]. This forwards an observer up to chrome [3] which forward over to content [4], which updates the display port.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#979
[2] http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/APZController.cpp#32
[3] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/apzc.js#89
[4] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#589

So about this bug, I've tested calling RequestContentRepaint() in UpdateScrollOffset and it does fix the redraw problem we have for various forms of scroll. However, SetCacheViewport also calls scrollBy on the browser to update it's position, and this breaks smooth scroll in cases like mouse wheel.

If we can get all the input that triggers smooth scrolling going straight to the apz via widget (making existing smooth scroll obsolete) We can then add RequestContentRepaint to UpdateScrollOffset. This bug is about adding that or potentially figuring out some way to avoid calling scrollBy in cases where we know smooth scroll is active.

Comment 1

[Jim Mathies [:jimm]]

An additional optimization to consider here would be to look for a way to move the call to update the display port down closer to the apz. This would be temporary in nature, since as I mentioned we'll get this for free with remote content. The overhead here is pretty large though, so it might be worth looking at.

Comment 2

[Jim Mathies [:jimm]]

s/scrollBy/scrollTo 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#577

Comment 3

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

Note that the displayport when set in layout is already relative to the scroll position. This means that in theory you shouldn't need to re-set the displayport when other scrolling mechanisms trigger. So for example, say your visible region is x=100 y=100 w=200 h=200 and you set a displayport of x=50 y=50 w=300 h=300. This displayport is actually stored as relative to the scroll position, so it would be x=-50 y=-50 w=300 h=300. Let's say then content does a scrollTo to x=100 y=500. The next paint that Gecko does will be of the area x=50 y=450 w=300 h=300.

For the purposes of this bug, the only potential problem I see is that if you are in the middle of a fling, the APZC heuristics might be projecting the displayport out (based on current velocity and expected paint time) so that it doesn't fully encompass the currently visible region (i.e. the relative displayport coordinates might have positive x or y). If at that point content does a scrollTo and the fling is aborted, the displayport might end up stuck at the projected position. So we should only need to set the displayport explicitly if UpdateScrollOffset is called while the APZC's mState != NOTHING, or something along those lines. As an aside it also looks like when UpdateScrollOffset is called it should clear any fling animations in progress and reset mState.

I'm going to narrow the scope of this bug to be to "modify the UpdateScrollOffset implementation to be more complete." In terms of what the Fennec code currently does, the code that needs to be added here is roughly JavaPanZoomController.abortAnimation().

Comment 4

[Jim Mathies [:jimm]]

I was guessing that the lack of a RequestContentRepaint() call was the cause of all the black rect problems we see from scroll activity that passes through the event state manager. Unfortunately after adding that and playing with it a bit in bug 907100, I'm still seeing redraw problems.

I think we could coalesce this bug with the other keyboard scroll bugs plus the dom scroll wheel problems in bug 907100 if we want. Seems all these issues are caused by the same problem we have with dom scroll and the way we message back the apz when it happens.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #4)
> I was guessing that the lack of a RequestContentRepaint() call was the cause
> of all the black rect problems we see from scroll activity that passes
> through the event state manager. Unfortunately after adding that and playing
> with it a bit in bug 907100, I'm still seeing redraw problems.
> 
> I think we could coalesce this bug with the other keyboard scroll bugs plus
> the dom scroll wheel problems in bug 907100 if we want. Seems all these
> issues are caused by the same problem we have with dom scroll and the way we
> message back the apz when it happens.

I've found a fix for these issues, will post to and dupe any open bugs to bug 907100. Leaving this to the issue kats describes in comment 3.

Comment 6

[Jim Mathies [:jimm]]

Should the remaining issue here block roll out kats?

Comment 7

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

No, the missing pieces I described in comment 3 are unlikely to be hit often and don't need to block 28.

Comment 8

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

Updating title to match what this bug's new scope is, as per comment 3 and comment 5.

Comment 9

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

Bug 1014997 mostly addresses this. I think the only thing left is to normalize the viewport so that it's not left in (under|over)zoom or overscroll.

Comment 10

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

Overscroll clearing is also done now. We don't allow overzoom or underzoom, so i'm going to close this bug.