964750 - Zooming elements with touch-action values = pan-x|pan-y| triggers wrong sequence of touch events

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Nick Lebedev [:nl]]

Attachment: html sample to reproduce the issue

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.76 Safari/537.36

Steps to reproduce:

Tried to pinch the image in attached html file


Actual results:

Zoom didn't happen. The following touch events were fired: touchstart, touchmove (a few times), touchcancel.


Expected results:

Zoom shouldn't happen happen. The following touch events should be fired: touchstart, touchmove (a lot of times), touchend.
(Div with image has touch-action value = pan-x. It means that zoom isn't allowed and usual sequence of events should be dispatched).

Comment 1

[Nick Lebedev [:nl]]

Attachment: Make GestureEventListener rely on the EventStatus returned from the apzc. Added few more checks for touch-action value when zooming to apzc.

Comment 2

[Nick Lebedev [:nl]]

Try server results: https://tbpl.mozilla.org/?tree=Try&rev=61cbdfdad6c2

Comment 3

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

Do you think it would be possible to write a gtest to cover this scenario?

Comment 4

[Nick Lebedev [:nl]]

Yes, sure. I'll just need to add one more test with gestureEventListener enabled.
Would you like me to create a new test?

I also found out that we're testing pan/zoom without gestureEventListener enabled in TestAsyncPanZoomController code. E.g. at [1]. Here we bypass gesture detector and create pinch events manually. But in real life we always have it enabled [2] and gestureEventListener creates pinches for us from raw touch move events. Possibly we might need to add more tests with gestureEventListener enabled to apzc and possibly add tests to gestureEventListener itself. Could you let me know your considerations about it?

[1] https://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp#230
[2] https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#188

Comment 5

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

(In reply to Nick Lebedev (MS) from comment #4)
> Yes, sure. I'll just need to add one more test with gestureEventListener
> enabled.
> Would you like me to create a new test?

Yes please.

> I also found out that we're testing pan/zoom without gestureEventListener
> enabled in TestAsyncPanZoomController code. E.g. at [1]. Here we bypass
> gesture detector and create pinch events manually. But in real life we
> always have it enabled [2] and gestureEventListener creates pinches for us
> from raw touch move events.

This is intentional - we want to unit-test AsyncPanZoomController in isolation and also test with using GestureEventListener. See for example https://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp#591 where we explicitly use the gesture detector to test short-press gesture detection.

> Possibly we might need to add more tests with
> gestureEventListener enabled to apzc and possibly add tests to
> gestureEventListener itself. Could you let me know your considerations about
> it?

But yes, if you feel we are lacking tests with GEL enabled then please feel free to add them.

Comment 6

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

Comment on attachment 8368653 [details] [diff] [review]
Make GestureEventListener rely on the EventStatus returned from the apzc. Added few more checks for touch-action value when zooming to apzc.

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

Dropping flag for now (pending tests), but I don't see any obvious problems with the patch.

Comment 7

[Nick Lebedev [:nl]]

Attachment: Make GestureEventListener rely on the EventStatus returned from the apzc. Add a check to apzc if touch-action allows zooming. Update apzc tests accordingly.

Added a few tests to apzc that cover pinch logic in different situations.
A few notes about the patch:
1. I used a rounding of a value of pinch size multiplied by scale for passing into ScreenIntPoint constructor. For current tests it should be ok since there is no high-precision scale values. But it looks a bit hackly.

2. I've split the words in the tests names by '_' character for better readability.. not sure if it is ok for current code style.

Comment 8

[Nick Lebedev [:nl]]

Also, i will add try server results soon.

Comment 9

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8416931 [details] [diff] [review]
Make GestureEventListener rely on the EventStatus returned from the apzc. Add a check to apzc if touch-action allows zooming. Update apzc tests accordingly.

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

Thanks for this fix!

I'm fine with the naming style you proposed, though I'd like one thing renamed.

Please post another version for review.

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +222,5 @@
>                                     mLastTouchInput.modifiers);
>  
> +      rv = mAsyncPanZoomController->HandleGestureEvent(pinchEvent);
> +    } else {
> +      rv = nsEventStatus_eConsumeNoDefault;

Is there a reason that you didn't apply this for every call to mAPZC->HandleGestureEvent()?

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +292,4 @@
>  }
>  
>  static void
> +ApzcPinch(AsyncPanZoomController* aApzc, int aFocusX, int aFocusY, float aScale, bool usePinchInput = true) {

I'd prefer to separate these into two functions, or one that calls the other.

@@ +321,5 @@
> +    float pinchLengthScaled = pinchLength * aScale;
> +
> +    MultiTouchInput mtiStart = MultiTouchInput(MultiTouchInput::MULTITOUCH_START, 0, 0);
> +    mtiStart.mTouches.AppendElement(SingleTouchData(0, ScreenIntPoint(aFocusX, aFocusY), ScreenSize(0, 0), 0, 0));
> +    mtiStart.mTouches.AppendElement(SingleTouchData(0, ScreenIntPoint(aFocusX, aFocusY), ScreenSize(0, 0), 0, 0));

Each of these touches should have a different identifier.

@@ +476,2 @@
>  
> +TEST_F(AsyncPanZoomControllerTester, Pinch_GestureRecognizer_NoTouchAction) {

s/GestureRecognizer/UseGestureDetector/g (since in APZC we call it USE_GESTURE_DETECTOR)

@@ -415,4 @@
>  
> -  nsTArray<uint32_t> values;
> -  values.AppendElement(mozilla::layers::AllowedTouchBehavior::VERTICAL_PAN);
> -  values.AppendElement(mozilla::layers::AllowedTouchBehavior::PINCH_ZOOM);

It looks like we're losing a test case here where the behaviors along each axis are different.

Comment 10

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

Comment on attachment 8416931 [details] [diff] [review]
Make GestureEventListener rely on the EventStatus returned from the apzc. Add a check to apzc if touch-action allows zooming. Update apzc tests accordingly.

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

I'm fine with drs reviewing this patch, don't need two reviewers I think.

Comment 11

[Nick Lebedev [:nl]]

Attachment: Make GestureEventListener rely on the EventStatus returned from the apzc. Add a check to apzc if touch-action allows zooming. Update apzc tests accordingly. v3.

Addressed comments.

>> Is there a reason that you didn't apply this for every call to mAPZC->HandleGestureEvent()?

In this patch i've added status checking to one more 'mAsyncPanZoomController->HandleGestureEvent' that handles tapup event. But after that i've had to change two tests - ShortPress and MediumPress. I've replaced comparing status of ApzcUp from nsEventStatus_eIgnore to nsEventStatus_eConsumeNoDefault which I believe is correct since if apzc consumed up event and it made some effect on apzc it should return consume result.

btw, try results are running, will add them soon.

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417535 [details] [diff] [review]
Make GestureEventListener rely on the EventStatus returned from the apzc. Add a check to apzc if touch-action allows zooming. Update apzc tests accordingly. v3.

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

Okay, this seems sound to me. Please make sure that you test every gesture function and that they all still work (double tap, long tap, etc.) Thanks!

Comment 13

[Nick Lebedev [:nl]]

Seems like gesture weren't broken by my patch and also try results didn't reveal any regression - https://tbpl.mozilla.org/?tree=Try&rev=aaec327ae495.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4486d2c281db

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4486d2c281db

Comment 16

[Justin D'Arcangelo [:justindarc]]

This needs either backed out or fixed. See Bug 1013378