1457743 - Momentum scrolling after a two-finger pan is broken when two fingers are lifted at the same time

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Load a page that's not zoomable on a touchscreen device.
  2. Pan the page with two fingers.
  3. Release the two fingers at the same time.

Expected results:
  The page momentum-scrolls after the fingers are released.

Actual results:
  The page does not momentum-scroll.

This is a regression from bug 1443231.

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1457743 - Fix check for entering momentum-scrolling codepath after both fingers are released in a two-finger pan.

Review commit: https://reviewboard.mozilla.org/r/240580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240580/

Comment 2

[Botond Ballo [:botond]]

A test that exercises this functionality is being written in bug 1355656.

Comment 3

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

Comment on attachment 8971839 [details]
Bug 1457743 - Fix check for entering momentum-scrolling codepath after both fingers are released in a two-finger pan.

https://reviewboard.mozilla.org/r/240580/#review246282

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4ec7081819f
Fix check for entering momentum-scrolling codepath after both fingers are released in a two-finger pan. r=kats

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f4ec7081819f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Can this ride the trains at this point or should we attempt to get this into 60 still?

Comment 7

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Can this ride the trains at this point or should we attempt to get this into
> 60 still?

We can try. From a risk point of view I think this patch is very low-risk.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8971839 [details]
Bug 1457743 - Fix check for entering momentum-scrolling codepath after both fingers are released in a two-finger pan.

Approval Request Comment
[Feature/Bug causing the regression]:
  Bug 1443231.

[User impact if declined]:
  On touchscreen devices (including Android), when
  scrolling with two fingers on a non-zoomable page,
  if the two fingers are lifted at the same time,
  no momentum scrolling occurs.

[Is this code covered by automated tests?]:
  Not yet, but one is being written in bug 1355656.

[Has the fix been verified in Nightly?]:
  Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  None.

[Is the change risky?]:
  Very low risk.

[Why is the change risky/not risky?]:
  This is a one liner fix which is obvious in retrospect.

[String changes made/needed]:
  None.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8971839 [details]
Bug 1457743 - Fix check for entering momentum-scrolling codepath after both fingers are released in a two-finger pan.

I'm going to pass on this for 60.0 because we've already built release candidates and I'd rather not respin for this one change, but I'll leave the uplift request around as a possible ride along for a dot release.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8971839 [details]
Bug 1457743 - Fix check for entering momentum-scrolling codepath after both fingers are released in a two-finger pan.

low risk fix for touch devices, let's take this for 60.0.1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/cccf97e1369b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/53b88f7b9982

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added this to Firefox and ESR 60.0.1 release notes

Comment 14

[Oana Horvath [:ohorvath]]

Verified on RC 60.0.1 on Android devices:
Nexus 9	tablet (Android 6.0.1)
Mi 4i phone (Android 5.0.2)

Comment 15

[James B]

Is there a chance that this patch could have caused an issue I just discovered?  Last week, I could visit https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/ and use the touch interface on my Surface Book to do multitouch gestures (pinch to zoom, two-finger pan to tilt, etc).  Today, that doesn't work, and the only thing I see that's changed is that Firefox automatically updated.  Is there an easy way for me to roll back to before the patch (would that be 60.0.0?) to confirm?  If it is a regression, would I need to file a new ticket or could this one be re-opened?

Comment 16

[Oana Horvath [:ohorvath]]

(In reply to James B from comment #15)
> Is there a chance that this patch could have caused an issue I just
> discovered?  Last week, I could visit
> https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/ and use the touch
> interface on my Surface Book to do multitouch gestures (pinch to zoom,
> two-finger pan to tilt, etc).  Today, that doesn't work, and the only thing
> I see that's changed is that Firefox automatically updated.  Is there an
> easy way for me to roll back to before the patch (would that be 60.0.0?) to
> confirm?  If it is a regression, would I need to file a new ticket or could
> this one be re-opened?

Hi,
You can download a previous build from here: http://archive.mozilla.org/pub/firefox/releases/60.0/
If it's a new issue on 60.0.1, please log a new bug and include the "regression" keywords. Thanks!

Comment 17

[James B]

I tried 60.0.0 and the bug is there, so I tried 52.8.0 ESR (no bug) and 59.0.3 (no bug).  If I read the version listing correctly, this means 60.0.0 (and not the subject patch) introduced the issue.  I will file a new issue.  Thanks for the link!

Comment 18

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

There is also a tool called mozregression [1] that will allow you to run nightly builds quickly to bisect which change introduced this regression. If you can run that and include the results in the issue you file that would be great.

[1] https://mozilla.github.io/mozregression/

Comment 19

[James B]

I filed #1463354 this morning, and am running the tool now.  I will report the results there.