Closed Bug 1661108 Opened 4 years ago Closed 4 years ago

MLS map continues to zoom after touchpad scroll gesture stops with allow_zooming on Windows

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- unaffected
firefox81 --- disabled
firefox82 --- fixed

People

(Reporter: cpeterson, Assigned: tnikkel)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: regression)

Attachments

(2 files)

Steps to reproduce

  1. In Firefox Nightly on Windows, open MLS map page: https://location.services.mozilla.com/map
  2. Use the laptop touchpad scroll gesture to zoom in and out of the MLS map.
  3. Lift your fingers from the laptop touchpad to stop zooming.

Expected results

The map should stop zooming.

Actual results

The map will zoom in and out, but when you lift your fingers from the laptop touchpad, the map will continue zooming in or out more more zoom level. I can reproduce this on my Windows 10 laptop, but not on my MacBook Pro.

This is a regression from allow_zooming bug 1620055, which is restricted to the Nightly channel. I bisected this bug to the following pushlog:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a518405abe4c93e6d8df622f59d5f01c4766e1d5&tochange=a09e4ce9ebba8fa4a29a4833560c8ee807018c03

Keywords: regression
Attached file wev2.html

apz.allow_zooming also enables direct manipulation. This means we can get pixel level scrolls out of those two finger scroll gestures on the touchpad instead of just line level events. direct manipulation automatically also gives us inertia for these gestures, in gecko we call it momentum. Either way it is events that happen after the user lifts their fingers. Without dmanip, the line level events we got either didn't give us momentum or the momentum was smaller.

On mac we also get momentum events, and I feel like I can reproduce the bug on mac as well, but I do not claim to be a very sensitive observer of this effect in general.

Using the testcase wev2.html that I uploaded it's a little easier to see what is going on instead of having to look at discrete jumps of zoom in a map that is also likely animated by the page (so the zooms are going to lag behind input almost always). Open it and do pan gestures over the yellow box while you have the console open. It will show the events it gets, the delta for each event, the accumulated deltaY for all events since the last time the clear button was pressed, and the delta mode for each event (0 == pixel, 1 == line). Playing around with this you can get a feel for how much momentum there is.

Using the testcase my subjective observation was that on Windows without dmanip we had very little to no momemtum, with dmanip we had quite a bit of momentum, and on mac we have some momentum but less than with dmanip on Windows.

I'm not exactly sure what, if anything we want to do here. The only thing that is initially coming to mind is to not generate WidgetWheelEvent's for pan gestures that are momentum, but I'm not even sure if that is a good idea in general.

How does Chrome behave in this scenario? If they have the same issue then I would say this is non-blocking.

Good call.

Chrome seems the same as Firefox (with zooming = true) to me. I tested on both mac and Windows, although the amount of momentum differs by platform, on each platform Firefox (with zooming = true) and Chrome feel like they match to me.

Ok. I'll mark it as a post-release bug. I think it would be nice to fix if we can. One approach might be to see if the non-momentum wheel events are getting canceled by content, and if they are, then don't send the momentum-based wheel events at all.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)

How does Chrome behave in this scenario? If they have the same issue then I would say this is non-blocking.

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

Chrome seems the same as Firefox (with zooming = true) to me. I tested on both mac and Windows, although the amount of momentum differs by platform, on each platform Firefox (with zooming = true) and Chrome feel like they match to me.

FWIW, I can't reproduce this zooming behavior on Chrome on my Windows laptop. I only see the problem in Firefox (with allow_zooming = true).

Could you try comparing scrolling over the yellow div in wev2.html attachment here with the console visible in both Chrome and Firefox to see if you get about the same amount of momentum wheel events?

Flags: needinfo?(cpeterson)

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Could you try comparing scrolling over the yellow div in wev2.html attachment here with the console visible in both Chrome and Firefox to see if you get about the same amount of momentum wheel events?

It's really hard to compare because I can't really tell if I'm scrolling a similar distance on the touchpad in both browsers, but it looks like Firefox is generating about the same or maybe slightly more wheel events.

One interesting difference is that Chrome always rounds event.deltaX/deltaY/deltaZ to a multiple of 0.5:

wheel handler called 0 32.5 0 tot 2332.5
deltamode 0
wheel handler called 0 40 0 tot 2405
deltamode 0
wheel handler called 0 2.5 0 tot 1222.5
deltamode 0
wheel handler called 0 2.5 0 tot 1225
deltamode 0
wheel handler called 0 0 0 tot 1225

whereas Firefox 82 Nightly returns delta values with many decimal digits:

wheel handler called 0 2.01607666015625 0 tot 1497.3048095703123
deltamode 0
wheel handler called 0 0.516064453125 0 tot 1497.8208740234372
deltamode 0
wheel handler called 0 1.30439453125 0 tot 1499.1252685546872
deltamode 0
wheel handler called 0 -0.0096435546875 0 tot 1499.1156249999997

But if I set apz.allow_zooming = 0, then Firefox rounds the delta values to a multiple of 0.025:

wheel handler called 0 -0.475 0 tot 20.349999999999998
deltamode 1
wheel handler called 0 -0.625 0 tot 54.550000000000004
deltamode 1
wheel handler called 0 -28.05 0 tot 26.500000000000004
deltamode 1
wheel handler called 0 -1.6 0 tot 24.900000000000002
deltamode 1
wheel handler called 0 -0.775 0 tot 24.125000000000004
Flags: needinfo?(cpeterson) → needinfo?(tnikkel)

(In reply to Chris Peterson [:cpeterson] from comment #8)

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Could you try comparing scrolling over the yellow div in wev2.html attachment here with the console visible in both Chrome and Firefox to see if you get about the same amount of momentum wheel events?

It's really hard to compare because I can't really tell if I'm scrolling a similar distance on the touchpad in both browsers, but it looks like Firefox is generating about the same or maybe slightly more wheel events.

I mainly just look at how long after my touch do events keep coming in.

The total numbers seem about the same to me.

I don't claim to be the best observer of this. Maybe a third person needs to try and give their observation.

Flags: needinfo?(tnikkel)

I tried it on my Windows laptop. On the MLS map I see similar behaviour in both FF and Chrome - the map continues zooming after I lift my fingers, depending on "aggressive" my fling is. The behaviour seems comparable in both browser.

I also tried the test page and get similar cumulative numbers for similar gestures.

(In reply to Chris Peterson [:cpeterson] from comment #6)

FWIW, I can't reproduce this zooming behavior on Chrome on my Windows laptop. I only see the problem in Firefox (with allow_zooming = true).

This part is surprising to me, I can't think of differences that would account for the problem only happening in Firefox for you but not for me. Specially if the behaviour on the test page is comparable with the browsers and you're getting momentum/dmanip in Chrome.

Severity: -- → S3
Priority: -- → P3

(In reply to Chris Peterson [:cpeterson] from comment #6)

FWIW, I can't reproduce this zooming behavior on Chrome on my Windows laptop. I only see the problem in Firefox (with allow_zooming = true).

This part is surprising to me, I can't think of differences that would account for the problem only happening in Firefox for you but not for me. Specially if the behaviour on the test page is comparable with the browsers and you're getting momentum/dmanip in Chrome.

Correction: I retested Chrome and I can reproduce the "map continues zooming after I lift my fingers" problem in Chrome, but only occasionally. In Firefox, the extra zoom happens every time I lift my fingers off the touchpad, no matter how long or motionless I leave my fingers resting on the touchpad after scrolling before lifting them.

Ah, I can reproduce something like that. On the test page, if I do a two-finger scroll and then leave my fingers on the trackpad as still as I can, Chrome stops sending events. However in FF we still send a trickle of events with small values. Maybe we should do some debouncing and throw away events with very small deltas.

Thanks for the investigation!

Yeah, seems like Chrome does something like that. If I do a simple quick pan (a brief touch, no lingering) then in the momentum phase Chrome never sends a delta less than 1.5.

So seems like we should try something about very small values. Two ideas come to mind: 1) ignore small deltas like kats suggested or 2) accumulate deltas and only send events when the accumulation exceeds 1.5. The latter is what we do to accumulate a value that we put in mLineOrPageDeltaX on the wheel event, which we then later use to dispatch legacy scroll events like MozMousePixelScroll (which we had to implement for dmanip to fix other issues). I'll give these a try and see what seems best.

That Chrome is ignoring small deltas is probably related to Chrome's event.deltaX/deltaY/deltaZ always being rounded to a multiple of 0.5.

Have you seen delta values less than 1.5 (other than 0)? I can't get any.

I did notice an interesting thing, if I start the gesture as a pinch and then do two finger panning Chrome suddenly starts giving me delta's that are not rounded to 0.5 and can be quite small. So they must have a pan mode that massages the delta's.

Looks like when Chromium converts pen gestures into wheel events it rounds to the nearest integer

https://source.chromium.org/chromium/chromium/src/+/master:ui/events/event.cc;l=642?q=MouseWheelEvent::MouseWheelEvent

I could easily see the 0.5 coming into play by taking into account the screen scale factor which they do in a number of places.

I implemented the rounding but I'm not sure if you'll find it is a significant improvement or not.

Here is a try build. Could you test it please?

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/aHojdI9ESQmx1iR2TsALIg/runs/0/artifacts/public/build/target.zip

Flags: needinfo?(cpeterson)

(In reply to Timothy Nikkel (:tnikkel) from comment #15)

Have you seen delta values less than 1.5 (other than 0)? I can't get any.

Correction: earlier I said Chrome rounds delta values to multiple of 0.5, but on closer inspection, it is actually rounding to multiples of 2.5! I never see a delta value between 0 and 2.5. I see Chrome delta values like:

wheel handler called 0 -2.5 0
wheel handler called 0 -5 0
wheel handler called 0 -7.5 0
wheel handler called 0 -10 0
wheel handler called 0 -12.5 0
wheel handler called 0 15 0
wheel handler called 0 -22.5 0
wheel handler called 0 -25 0
wheel handler called 5 -35 0
wheel handler called 5 -57.5

(In reply to Timothy Nikkel (:tnikkel) from comment #18)

I implemented the rounding but I'm not sure if you'll find it is a significant improvement or not.

Here is a try build. Could you test it please?

With a regular build, I see an extra zoom jump every time I zoom the map in or out. With your try build, I still see the extra zoom every time I zoom out, but I see the extra zoom much less frequently when I zoom in. So this build is better, but not equivalent behavior to Chrome or Firefox without allow_zooming.

Flags: needinfo?(cpeterson) → needinfo?(tnikkel)

Thanks for testing.

I'm not quite sure what to do next. We don't want to round the delta's of the PanGestureInput too much because then we lose precision (for scrolls that aren't intercepted by a web content wheel listener), but the delta's we put in PanGestureInput are directly linked to the delta's on the wheel event that web content gets. Open to ideas.

I made a new build that increases the threshold slightly. Instead of checking that the rounded pixel delta is at least 1 this new build checks that the pixel delta (before rounding) is at least 1. Thus effectively moving the threshold from 0.5 to 1 (the actually delta that shows up in the dom event will be different because of screen scaling and maybe full (reflow) zoom).

Can you try this and report how it feels in comparison to the last build and a regular nightly build? Thanks
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Na_DhK8-Rq-l1tttkvvGvA/runs/0/artifacts/public/build/target.zip

Flags: needinfo?(tnikkel) → needinfo?(cpeterson)

(In reply to Timothy Nikkel (:tnikkel) from comment #21)

I made a new build that increases the threshold slightly. Instead of checking that the rounded pixel delta is at least 1 this new build checks that the pixel delta (before rounding) is at least 1. Thus effectively moving the threshold from 0.5 to 1 (the actually delta that shows up in the dom event will be different because of screen scaling and maybe full (reflow) zoom).

FWIW, in the wev2.html test, the events' delta values now appear to always be multiples of 0.2.

Can you try this and report how it feels in comparison to the last build and a regular nightly build? Thanks
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Na_DhK8-Rq-l1tttkvvGvA/runs/0/artifacts/public/build/target.zip

This try build feels better than the last try build. I still see an extra zoom less than half the time when I zoom out, whereas the previous build always had an extra zoom when zooming out. Like the last try build, I rarely see an extra zoom when zooming in.

Flags: needinfo?(cpeterson) → needinfo?(tnikkel)

Testing on mac it seems as though the OS only sends us touchpad sourced scrolls if the delta is 1 or greater, which we then multiple by the backing scale factor (2 on retina screens). Only sending events that have a delta of at least 1 before multiplying by the backing scale factor might also explain Chrome's behaviour, so let's give that a try.

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Sb-M6IBkTpGxOmmsVinWQw/runs/0/artifacts/public/build/target.zip

Flags: needinfo?(tnikkel) → needinfo?(cpeterson)

(In reply to Timothy Nikkel (:tnikkel) from comment #23)

Testing on mac it seems as though the OS only sends us touchpad sourced scrolls if the delta is 1 or greater, which we then multiple by the backing scale factor (2 on retina screens). Only sending events that have a delta of at least 1 before multiplying by the backing scale factor might also explain Chrome's behaviour, so let's give that a try.

https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Sb-M6IBkTpGxOmmsVinWQw/runs/0/artifacts/public/build/target.zip

LGTM on my Windows laptop. I see very few extra jumps when zooming in or out.

However, zooming on all these try builds feels laggier than in Chrome or Firefox Nightly. I don't know if that's because these aren't PGO opt builds or due to the scroll event changes or just my imagination. It's pretty subjective.

Flags: needinfo?(cpeterson)

Thanks for testing. We can look at the other issue after we land this fix.

Assignee: nobody → tnikkel
Attachment #9172604 - Attachment description: Bug 1661108. Only send pan events from DManip if the delta will be greater than zero after rounding. → Bug 1661108. Only send pan events from DManip if the delta is at least 1, and round pixel deltas when converting the PanGestureInput to widget wheel events. r?kats
Status: NEW → ASSIGNED
Attachment #9172604 - Attachment description: Bug 1661108. Only send pan events from DManip if the delta is at least 1, and round pixel deltas when converting the PanGestureInput to widget wheel events. r?kats → Bug 1661108. Only send pan events from DManip if the delta is at least 1. r?kats
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a95c23aa4e10
Only send pan events from DManip if the delta is at least 1. r=kats
Flags: needinfo?(tnikkel)
Attachment #9172604 - Attachment description: Bug 1661108. Only send pan events from DManip if the delta is at least 1. r?kats → Bug 1661108. Only send pan events from DManip if the delta is at least 1. r=kats
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d871d71f5196
Only send pan events from DManip if the delta is at least 1. r=kats
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: