Closed Bug 1304729 Opened 8 years ago Closed 8 years ago

Pinch gestures on desktop (non-zoomable) result in unexpected jumps and flings

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Version:
52 Branch
Platform:
x86
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

Bug 1304729 - Ensure we schedule a composite if we change the scroll position during a pinch gesture with no zoom change.
58 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1304729 - When transitioning from a pinch to a pan, make sure we start the pan with the correct coordinates.
58 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1304729 - Followup to fix careless mistake.
58 bytes, text/x-review-board-request
botond
: review+
Details 
On Windows touch devices, if you do two-finger gestures such as pinches or two-finger scrolls the behaviour is quite unexpected. Generally nothing moves until you lift a finger or both fingers at which point the content jumps or goes into a crazy fling.

The "nothing moves" part is because even though we are updating the focus point and scrolling, we're not scheduling composites, so nothing gets updated on-screen.

The crazy fling part I'm still investigating
Comment on attachment 8793873 [details]
Bug 1304729 - Ensure we schedule a composite if we change the scroll position during a pinch gesture with no zoom change.

https://reviewboard.mozilla.org/r/80476/#review79158

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1358
(Diff revision 1)
>      // If displacing by the change in focus point will take us off page bounds,
>      // then reduce the displacement such that it doesn't.
>      focusChange.x -= mX.DisplacementWillOverscrollAmount(focusChange.x);
>      focusChange.y -= mY.DisplacementWillOverscrollAmount(focusChange.y);
>      ScrollBy(focusChange / userZoom);
> +    ScheduleComposite();

Scheduling a composite isn't super-cheap - it schedules a task, when that task runs it schedules another task...

So instead of calling it twice in the case where we're actually zooming, can we use a flag and do it once at the end of OnScale() if necessary?
Comment on attachment 8793874 [details]
Bug 1304729 - When transitioning from a pinch to a pan, make sure we start the pan with the correct coordinates.

https://reviewboard.mozilla.org/r/80478/#review79160

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1445
(Diff revision 1)
>  
>    // Non-negative focus point would indicate that one finger is still down
>    if (aEvent.mFocusPoint.x != -1 && aEvent.mFocusPoint.y != -1) {
>      mPanDirRestricted = false;
> -    mX.StartTouch(aEvent.mFocusPoint.x, aEvent.mTime);
> -    mY.StartTouch(aEvent.mFocusPoint.y, aEvent.mTime);
> +    mX.StartTouch(aEvent.mLocalFocusPoint.x, aEvent.mTime);
> +    mY.StartTouch(aEvent.mLocalFocusPoint.y, aEvent.mTime);

Good catch!

If bug 1060421 were fixed, these lines would have been compile errors :/

::: gfx/layers/apz/src/GestureEventListener.cpp:395
(Diff revision 1)
>        if (mTouches.Length() == 1) {
>          // As user still keeps one finger down the event's focus point should
>          // contain meaningful data.
> -        point = mTouches[0].mScreenPoint;
> +        point = mTouches[0].mLocalScreenPoint;
>        }
>        PinchGestureInput pinchEvent(PinchGestureInput::PINCHGESTURE_END,

Can we get rid of the PinchGestureInput constructor that takes a ScreenPoint? With this change, the only remaining call site is in gtest code, which could easily be adjusted.
Attachment #8793874 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #3)
> >      ScrollBy(focusChange / userZoom);
> > +    ScheduleComposite();
> 
> Scheduling a composite isn't super-cheap - it schedules a task, when that
> task runs it schedules another task...
> 
> So instead of calling it twice in the case where we're actually zooming, can
> we use a flag and do it once at the end of OnScale() if necessary?

Sure, but we don't even need the flag. The instance I added is unconditional, so now assuming we get past the initial early-returns in the function, we will always schedule at least one composite. I can move that down to the end of the function and remove the other one.

(In reply to Botond Ballo [:botond] from comment #4)
> If bug 1060421 were fixed, these lines would have been compile errors :/

Yeah I was kinda surprised we still had instances of this :/

> ::: gfx/layers/apz/src/GestureEventListener.cpp:395
> Can we get rid of the PinchGestureInput constructor that takes a
> ScreenPoint? With this change, the only remaining call site is in gtest
> code, which could easily be adjusted.

Sure, will do.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920bc2c4fd3a
Ensure we schedule a composite if we change the scroll position during a pinch gesture with no zoom change. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/df5f4903e927
When transitioning from a pinch to a pan, make sure we start the pan with the correct coordinates. r=botond
https://hg.mozilla.org/mozilla-central/rev/920bc2c4fd3a
https://hg.mozilla.org/mozilla-central/rev/df5f4903e927
Status: NEW → RESOLVED
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
status-firefox52: affected → fixed
Resolution: --- → FIXED
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Target Milestone: --- → mozilla52
Err whoops I just noticed you didn't actually give an r+ on the first patch - can you take a look at the landed patch and let me know if you want me to make any changes? Sorry about that!
.. and while writing a gtest for bug 1298886 I discovered that I forgot to update s/mFocusPoint/mLocalFocusPoint/ in the line at https://hg.mozilla.org/mozilla-central/rev/df5f4903e927#l1.10, I'll land a followup for that along with any other changes you would like for the first patch.
The landed patch looks good (modulo the issue you spotted in comment 10).
(I'd r+ it but the version on MozReview is still the old version.)
Updated the existing patches on MozReview and added the new one as well ^
Comment on attachment 8793873 [details]
Bug 1304729 - Ensure we schedule a composite if we change the scroll position during a pinch gesture with no zoom change.

https://reviewboard.mozilla.org/r/80476/#review79474
Attachment #8793873 - Flags: review?(botond) → review+
Comment on attachment 8794276 [details]
Bug 1304729 - Followup to fix careless mistake.

https://reviewboard.mozilla.org/r/80802/#review79476
Comment on attachment 8794276 [details]
Bug 1304729 - Followup to fix careless mistake.

https://reviewboard.mozilla.org/r/80802/#review79478
Attachment #8794276 - Flags: review?(botond) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6029a9a8b8db
Followup to fix careless mistake. r=botond
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: