1142926 - SelectionCarets show while APZ is still in momentum scrolling

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Steps to reproduce:
1. On B2G, open UI tests app.
2. Switch to "API" tab below, and click "Copy Paste".
3. Long tap to select a word. Selection carets should show.
4. Swipe the screen to scroll the content. 

Actual results:
Selection carets shows after the finger leaves the screen while APZ is still in momentum scrolling.

Expected result:
Selection carets should show after APZ stops momentum scrolling.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

According to UX spec, Selection carets should hide while it begins scrolling, and they are shown after the content stops scrolling. To do this, SelectionCarets register nsIScrollObserver to get callbacks AsyncPanZoomStarted and AsyncPanZoomStopped. 

Here is what I've observed.
a) While touching the screen, AsyncPanZoomStarted is called. This is expected.
b) After my finger leaves the screen, AsyncPanZoomStarted is called, and then AsyncPanZoomStopped is called immediately. That's why SelectionCarets show at this moment.
c) After momentum scrolling stops, AsyncPanZoomStopped is called.

Kats, is the behavior in step b) expected ? If my memory serves me well, AsyncPanZoomStarted and AsyncPanZoomStopped used to be called in a pair. In other words, after I get an AsyncPanZoomStarted, I will get an AsyncPanZoomStopped before I get another AsyncPanZoomStarted.

Comment 2

[Botond Ballo [:botond]]

The AsyncPanZoomStarted / AsyncPanZoomStopped notifications are triggered by state changes in APZ. Sometimes, a single conceptual state change is implemented as multiple actual state changes, leading to more notifications being dispatched than would be expected.

In bug 1053766 we introduced a mechanism for avoiding sending unnecessary notifications. We may want to extend that to apply to the scenario in this bug.

Comment 3

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

Attachment: gdb backtraces

I reproduced the problem and the attached the problematic stacks. When we switch from panning -> flinging, we do already have a StateChangeNotificationBlocker in place, but the problem is that the APZC that is panning doesn't go into the flinging state; one further up the handoff chain does instead. So the APZC that was panning goes into NOTHING, which issues a TransformEnd change, and the handoff parent goes from NOTHING to flinging, which issues a TransformBegin change. Except that the order of notifications ends up with two TransformBegins followed by two TransformEnds. I think we can fix this by tracking a count of actively transforming APZCs in APZEventState.

Comment 4

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

Attachment: Patch

This fixes the problem. Although I have no idea why the "scrollview" event that is dispatched all over the place starting at [1] includes the scroll position. Ting-Yu, what is this scroll position used for? It doesn't make sense to pass this around because if there are multiple scrollframes in the document you won't know which one is scrolling, and whichever one is scrolling is the one you'll be getting the coordinates for. If you're using those coordinates for anything it's likely broken.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.cpp?rev=f9b76a07fcc6#327

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8577297 [details] [diff] [review]
Patch

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

Is it possible, in a situation where we have multiple active APZ transforms, that the AsyncPanZoomStarted and AsyncPanZoomStopped notifications will go to different documents (because e.g. the Started is for an inner APZC in a subdocument, while the Stopped is for an outer APZC in a parent document)? If so, could that be a problem?

Comment 6

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

(In reply to Botond Ballo [:botond] from comment #5)
> Is it possible, in a situation where we have multiple active APZ transforms,
> that the AsyncPanZoomStarted and AsyncPanZoomStopped notifications will go
> to different documents (because e.g. the Started is for an inner APZC in a
> subdocument, while the Stopped is for an outer APZC in a parent document)?
> If so, could that be a problem?

Good point, but I don't think that will be a problem. The document is passed in to NotifyAPZStateChanged from the GeckoContentController implementation, so all APZCs in a given process should map to the same document (the root document for that process). Note that each APZC instance issues a balanced pair of state change notifications, which means that each GeckoCC impl will received a balanced number of state change notifications, and so each APZEventState will also receive a balanced number of state change notifications. Therefore each document will get a balanced number of state change notifications.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)

> This fixes the problem. Although I have no idea why the "scrollview" event
> that is dispatched all over the place starting at [1] includes the scroll
> position. Ting-Yu, what is this scroll position used for? 

The scroll position was added in bug 1020801. It was included in the scrollview change event, and will be dispatched to Gaia so that Gaia could calculate the position of the copy & paste dialog after scrolling.  As far as I know, Gaia removed the usage of the scroll position in [1].  I believe it's safe to remove the scroll position from gecko. 

I don't get a change to verify this patch today. Will do tomorrow.

[1] https://github.com/cctuan/gaia/commit/5ade148e6d079e2428ad47c790f1b09be0694440

Comment 8

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

Thanks, I filed bug 1143665 to remove the scroll position from the event.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8577297 [details] [diff] [review]
Patch

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> (In reply to Botond Ballo [:botond] from comment #5)
> > Is it possible, in a situation where we have multiple active APZ transforms,
> > that the AsyncPanZoomStarted and AsyncPanZoomStopped notifications will go
> > to different documents (because e.g. the Started is for an inner APZC in a
> > subdocument, while the Stopped is for an outer APZC in a parent document)?
> > If so, could that be a problem?
> 
> Good point, but I don't think that will be a problem. The document is passed
> in to NotifyAPZStateChanged from the GeckoContentController implementation,
> so all APZCs in a given process should map to the same document (the root
> document for that process). Note that each APZC instance issues a balanced
> pair of state change notifications, which means that each GeckoCC impl will
> received a balanced number of state change notifications, and so each
> APZEventState will also receive a balanced number of state change
> notifications. Therefore each document will get a balanced number of state
> change notifications.

Indeed, you're right.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/71c85590ab17

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8577297 [details] [diff] [review]
Patch

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

It works as expected. Thanks!

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71c85590ab17