Closed Bug 1457743 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
relnote-firefox --- 60+
firefox-esr52 --- unaffected
firefox-esr60 60+ fixed
firefox59 --- unaffected
firefox60 + verified
firefox61 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
A test that exercises this functionality is being written in bug 1355656.
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
Attachment #8971839 - Flags: review?(bugmail) → review+
Pushed by bballo@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f4ec7081819f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Can this ride the trains at this point or should we attempt to get this into 60 still?
(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.
Flags: needinfo?(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.
Attachment #8971839 - Flags: approval-mozilla-beta?
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.
Attachment #8971839 - Flags: approval-mozilla-beta? → approval-mozilla-release?
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
Attachment #8971839 - Flags: approval-mozilla-release?
Attachment #8971839 - Flags: approval-mozilla-release+
Attachment #8971839 - Flags: approval-mozilla-esr60+
Added this to Firefox and ESR 60.0.1 release notes
Verified on RC 60.0.1 on Android devices:
Nexus 9	tablet (Android 6.0.1)
Mi 4i phone (Android 5.0.2)
Status: RESOLVED → VERIFIED
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?
(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!
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!
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/
I filed #1463354 this morning, and am running the tool now.  I will report the results there.
You need to log in before you can comment on or make changes to this bug.