Closed Bug 1664558 Opened 1 year ago Closed 1 year ago

Can't zoom embedded Google Maps using Ctrl + scroll trackpad gesture with Fission

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
84 Branch
Fission Milestone M6c
Tracking Status
firefox-esr68 --- disabled
firefox-esr78 --- disabled
firefox80 --- disabled
firefox81 --- disabled
firefox82 --- disabled
firefox83 --- disabled
firefox84 --- fixed

People

(Reporter: cpeterson, Assigned: kats)

References

(Blocks 2 open bugs, )

Details

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1658647 +++

Steps to reproduce

  1. In Fx82 Nightly, load a page with an embedded Google Map, such as https://developers.google.com/maps/documentation/javascript/interaction.
  2. Scroll the embedded map on screen.
  3. Try to zoom the map in and out using Ctrl + scroll trackpad gesture.

Expected result

The map should zoom in and out.

Actual result

The page scrolls instead of the map zooming. If I open the page in a non-Fission window, then I can zoom the map.

If I set the apz.allow_zooming pref = false and restart Firefox, then I can zoom the map (with or without Fission).

I'm using Fx82 Nightly on Windows 10.

Hanging this off of desktop-zoom-post for now so it shows up on our desktop zooming dashboard. We can revisit (whether it needs to block desktop-zoom-release) once a Fission milestone is assigned.

Blocks: apz-fission
Severity: -- → S3
Priority: -- → P3

tnikkel, you fixed this Google Maps bug for apz.allow_zooming in bug 1658647, but I can still reproduce the problem when Fission (fission.autostart) is enabled. Seems like APZ using a different code path with Fission?

Tracking this bug for Fission Nightly milestone M6c

Fission Milestone: ? → M6c
Flags: needinfo?(tnikkel)

My first thought is that it is related to the events not getting to where they need to be (hit testing/routing) and not particularly related to the dmanip issues I've previously fixed, but I haven't looked into this at all, so that could be completely wrong. There are other open bugs with hit testing and fission and I'm not completely clear on the state and what we expect to work and not work.

Botond, I think you have looked into some fission hit testing bugs recently. Is there a quick way to tell if this is a fission hit testing issue?

Flags: needinfo?(botond)

I'd start by looking at the layers id that APZ Is assigning to the pan gesture event (assuming that's the relevant event type) here, and cross-referencing it against the layers ids of the RefLayers in a layer dump, to see if the event is being targeted to the content process we expect.

Flags: needinfo?(botond)

I can take a look at this, since I'm looking into bug 1541589 in nearby code.

Assignee: nobody → kats
Flags: needinfo?(tnikkel)

The functions in InputData.cpp to convert from InputData types to WidgetEvent types aren't propagating the mLayersId in most cases. Fixing that makes the zooming work. However there's still a bug in that on macOS with non-fission, if I pan over the map, it gives me an overlay saying "use command + scroll to zoom the map". With my fix for this bug, zooming happens right away even if I'm not holding down the command modifier. It's not clear to me why, the modifiers seem correct as far as I can tell.

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

With my fix for this bug, zooming happens right away even if I'm not holding down the command modifier. It's not clear to me why, the modifiers seem correct as far as I can tell.

Never mind. I didn't realize the page in question actually has multiple embedded maps with different gesture handling properties. I was scrolling over different maps in my local build vs non-fission Nightly.

I was wondering why our existing fission tests didn't catch this problem. Looks like most or all of our tests go through the InputData <-> WidgetEvent conversion code in APZInputBridge.cpp, e.g. this one. However things like PanGestureInput or PinchGestureInput which are created directly in the widget and then converted to gecko events after running them through APZ, go through the conversion in InputData.cpp and so were missing the LayersId propagation.

Seems like an oversight that these fields were never propagated from APZ to
the generated main-thread WidgetEvents.

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66d877d0037e
Propagate the layers id from InputData to gecko WidgetEvent types. r=botond
https://hg.mozilla.org/integration/autoland/rev/cf70edfb90e9
Improve layers id logging in layer tree. r=botond
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.