Closed Bug 1621781 Opened 4 years ago Closed 4 years ago

Support mobile swipe/drag gestures in RDM

Categories

(DevTools :: Responsive Design Mode, enhancement, P3)

enhancement

Tracking

(firefox79 fixed)

RESOLVED FIXED
Firefox 79
Tracking Status
firefox79 --- fixed

People

(Reporter: mtigley, Assigned: mtigley)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 4 obsolete files)

This bug looks at supporting dragging gestures in RDM, which will allow the user to simulate proper swiping/scrolling gestures with the mouse.

Assignee: nobody → mtigley
Status: NEW → ASSIGNED
Blocks: 1282089
See Also: → 1621206
Attached file WIP: Supporting mobile swipe gestures (obsolete) —

Updates to this:

Since Bug 1623941 landed, RDM is now able to perform drag gestures on the page via dragging the pointer on a trackpad or mouse. The next missing piece is having the "fling" animation that occurs on a touchend event. This should be handled at: https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#1499

Because RDM's touch simulator is synthesizing native touch points to trigger gestures on the platform-level, this should just work. But it seems that PanGestureInput events are not being triggered so that the state PAN_MOMENTUM is set on AsyncPanZoomController: https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2698.

So I think what should be happening is that the APZCTreeManager input queue should be processing a PanGestureInput at some point (perhaps starting at: https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/InputQueue.cpp#415) before a touchend is processed. But right now the processing of this PanGestureInput just isn't happening.

Botond, do you know where the start of dispatching of pan gestures for drag gestures might occur?

Flags: needinfo?(botond)

The PanGestureInput code isn't relevant to this particular scenario; it's used with hardware input devices that can trigger panning directly, such as trackpads.

With touch events, the touchend event handling should trigger a fling animation, by going into HandleEndOfPan() which then starts a fling animation via this call. Most likely something is bailing out before getting to that point.

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

With touch events, the touchend event handling should trigger a fling animation, by going into HandleEndOfPan() which then starts a fling animation via this call. Most likely something is bailing out before getting to that point.

Thanks for the input! I built my original assumption around mState == PAN_MOMENTUM for touchend event handling to trigger the HandleEndOfPan() call. But I see there are other states such as PANNING that can cause it too. I'll continue my investigation by looking at the code responsible for setting those states.

Flags: needinfo?(botond)

I had a brief look at this. The issue seems to be that APZ is receiving both mouse and touch events, and the mouse-up causes it to transition to the NOTHING state, which causes the touch-end to not enter the case for handling the PANNING state.

The only reason APZ cares about mouse events is scrollbar dragging. Since scrollbar dragging also works via touch events, perhaps we should make APZ just ignore mouse events when RDM is active.

It occurred to me that perhaps RDM prevent-defaulting the mouse events should already have this effect, but it doesn't because mouse events aren't APZ-aware, which means they don't opt into the mechanism that causes APZ to wait and see if an event was prevent-defaulted and only process it if it wasn't.

I cooked up a quick patch to make the mousedown event APZ-aware, such that prevent-defaulting it causes APZ to ignore the drag gesture it starts.

With this patch, we get further: we get into the PANNING case in OnTouchEnd(), but we still don't trigger a fling animation because the computed velocity is zero. That will require further investigation.

I'm also not sure whether making the mousedown event APZ-aware in general is the best solution, as it can have negative side effects for performance (APZ needs to wait for content more often). We may want a solution more targeted to RDM.

Attachment #9152277 - Attachment is patch: true

(In reply to Botond Ballo [:botond] from comment #7)

I'm also not sure whether making the mousedown event APZ-aware in general is the best solution, as it can have negative side effects for performance (APZ needs to wait for content more often). We may want a solution more targeted to RDM.

Thanks for looking into this a bit, Botond! I agree we should try to make this RDM-specific, especially because of performance losses. I'll work on a patch that does this using your work as guidance.

(In reply to Botond Ballo [:botond] from comment #7)

With this patch, we get further: we get into the PANNING case in OnTouchEnd(), but we still don't trigger a fling animation because the computed velocity is zero. That will require further investigation.

This turned out to be a Linux-specific issue related to synthesized touch events having incorrect timestamps. The attached patch fixes it. With these two patches, mouse-drags successfully trigger fling animations for me in RDM (tested on Linux).

Attachment #9132708 - Attachment is obsolete: true

Having a valid timestamp is important because APZ uses it for
velocity calculations.

Attachment #9152277 - Attachment is obsolete: true
Attachment #9152462 - Attachment is obsolete: true
Attachment #9152462 - Attachment is patch: true
Attachment #9152600 - Attachment description: Bug 1621781 - Add an "IsRDMActive" field to ScrollMetadata. r=botond → Bug 1621781 - Add an "IsRDMTouchSimulationActive" field to ScrollMetadata. r=botond
Attachment #9152600 - Attachment description: Bug 1621781 - Add an "IsRDMTouchSimulationActive" field to ScrollMetadata. r=botond → Bug 1621781 - Add an "IsRDMActive" field to ScrollMetadata. r=botond
Attachment #9152600 - Attachment description: Bug 1621781 - Add an "IsRDMActive" field to ScrollMetadata. r=botond → Bug 1621781 - Add an "IsRDMTouchSimulationActive" field to ScrollMetadata. r=botond
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58a9b7f50a4b
Add an "IsRDMTouchSimulationActive" field to ScrollMetadata. r=botond
https://hg.mozilla.org/integration/autoland/rev/a5fe71693070
Have APZ ignore mouse events when RDM is active. r=botond

Looks like the call at https://hg.mozilla.org/integration/autoland/rev/a5fe71693070#l1.20 needs to be inside a scoped block with a RecursiveMutexAutoLock lock(mRecursiveMutex); to avoid a data race with reading/writing the scroll metadata.

Right, sorry, should have caught this during review.

To elaborate a bit, mScrollMetadata can be accessed from multiple threads, and therefore accesses to it need to be guarded by acquiring mRecursiveMutex, as mentioned here. (The comment is a bit out of date as it uses the word "monitor" when it means "mutex". The type of the mutex used to be RecursiveMonitor as has since been changed to RecursiveMutex.)

The "scoped block" that Kats is referring to is a technique that involves using a loose pair of braces like e.g. here to cause the RecursiveMutexAutoLock to go out of scope (and therefore the mutex to be unlocked) earlier than at the end of the function. This is important to respect lock ordering, as code later in HandleDragEvent() (in particular, this call) acquires the tree lock (which the lock ordering says cannot be acquired while mRecursiveMutex is held).

Thank you, Kats and Botond for explaining the reason behind the backout. I've updated the patch accordingly and have a green try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7bacb8aa705ce0dc8880c55b198848b60516e0f.

Flags: needinfo?(mtigley)
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cbcf8cdb97d
Add an "IsRDMTouchSimulationActive" field to ScrollMetadata. r=botond
https://hg.mozilla.org/integration/autoland/rev/a6eed456d959
Have APZ ignore mouse events when RDM is active. r=botond
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 79
See Also: → 1642788

This is awesome Micah, thanks for working on this. It is making RDM so much better mobile-like.

Attachment #9152808 - Attachment is obsolete: true

Added mention of drag events under "Enable/Disable touch simulation" in the Controlling Responsive Design Mode section. Also, general copy-editing and clean-up of this article.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: