Closed Bug 1324581 Opened 3 years ago Closed 3 years ago

test_scroll_snapping_scrollbars.html fails with apz.drag.enabled=true

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: botond, Assigned: kevin.m.wern)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

STR:
  1. Set the pref apz.drag.enabled to true
  2. Run the test layout/base/tests/test_scroll_snapping_scrollbars.html

This results in several failures:

33 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_scroll_snapping_scrollbars.html | Step 0: Synthesized mouse events move scroll position. (Drag scrollbar left, expect scroll snapping.) - didn't expect "(500,500)", but got it
34 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_scroll_snapping_scrollbars.html | Step 1: Synthesized mouse events move scroll position. (Drag scrollbar right, expect scroll snapping.) - didn't expect "(500,500)", but got it
35 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_scroll_snapping_scrollbars.html | Step 2: Synthesized mouse events move scroll position. (Drag scrollbar up, expect scroll snapping.) - didn't expect "(500,500)", but got it

(See https://treeherder.mozilla.org/logviewer.html#?job_id=8124350&repo=autoland#L2149 for more details)

We need to investigate and fix these failures before enabling apz.drag.enabled.
The test case is checking that on a page with scroll snap points, after dragging the scrollbar a small amount away from a snap point, the scroll position snaps back to the snap point.

Since the drag is handled by APZ, snapping after the drag needs to be handled by APZ as well.

We already have infrastructure in APZ to do scroll snapping; for example, see how we call ScrollSnap() at the end of a pan gesture [1]. We'll need to call it at the end of a drag gesture, too.

[1] http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/gfx/layers/apz/src/AsyncPanZoomController.cpp#1984
Summary: test_scroll_snapping_scrollbars.html fails with apz.drag.enabled=true → Implement scroll snapping for APZ scrollbar dragging
Thanks for looking into this! Seems like you did most of the heavy lifting for me already. I can add the ScrollSnap() call to the end of the gesture.
Assignee: nobody → kevin.m.wern
I've attempted to use ScrollSnap() in HandleDragEvent, but it isn't fixing the problem. I'm not sure if I'm doing something simple wrong, but I'm looking into the different drag events and trying to get a closer look at what the test itself does.

I'm going to look into it more later tonight. Just updating so you don't think I've taken 3 days to add one function call :).
Flags: needinfo?(botond)
It appears I misdiagnosed the problem - sorry!

The reason the test is failing is not that APZ does not implement scroll snapping for scrollbar dragging.

Rather, the scrollbar dragging in the test isn't working at all. The test is using the synthesizeMouse() method from EventUtils.js to synthesize the mouse events for the scrollbar drag. This method does not route the events through APZ, so APZ does not handle the drag.

The code in nsSliderFrame, however, checks the pref apz.drag.enabled, but doesn't check that the events are actually going through APZ - if the pref is enabled, it assumes that they are, and skips the main-thread code path for dragging.

So, neither APZ nor the main thread is handling the drag in the test case.

The solution is to adjust the code in nsSliderFrame to check for whether the events are actually going through APZ. This can be checked by checking the flag |event->mFlags.mHandledByAPZ| in nsSliderFrame::StartDrag(). If it's false, events are not going through APZ, and mScrollingWithAPZ should be set to false. (One way to do this would be to pass in 'event' as an argument to StartAPZDrag(), and have it early-return false if the flag is not set.)
Flags: needinfo?(botond)
Summary: Implement scroll snapping for APZ scrollbar dragging → test_scroll_snapping_scrollbars.html fails with apz.drag.enabled=true
That said, not calling ScrollSnap() at the end of an APZ drag gesture is still *a* problem, even though it's not the problem causing the test to fail. Might as well fix that too (in a separate patch please).
(Also, it wouldn't hurt to improve this test at some point so that it *does* actually route the synthesized events through APZ, and therefore exercises that code. Feel free to file a follow-up bug for that.)
Comment on attachment 8821475 [details]
Bug 1324581 - check event handled by apz in nsSliderFrame::StartAPZDrag

https://reviewboard.mozilla.org/r/100782/#review101452

Thanks!

In the commit message, instead of "synthesizeMouse does not flag events as handled by APZ", I would say "synthesizeMouse does not  route events through APZ".
Attachment #8821475 - Flags: review?(botond) → review+
Comment on attachment 8821484 [details]
Bug 1324581 - call ScrollSnap() in AsyncPanZoomController when drag ends

https://reviewboard.mozilla.org/r/100790/#review101454

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:901
(Diff revision 1)
>  
>    if (!GetApzcTreeManager()) {
>      return nsEventStatus_eConsumeNoDefault;
>    }
>  
> +  if (aEvent.mType == MouseInput::MouseType::MOUSE_DRAG_END) {

A quick test on Linux shows that the event received in HandleDragEvent at the end of a drag is a MOUSE_UP, not a MOUSE_DRAG_END.

Are you seeing MOUSE_DRAG_END on a different platform?
Comment on attachment 8821484 [details]
Bug 1324581 - call ScrollSnap() in AsyncPanZoomController when drag ends

https://reviewboard.mozilla.org/r/100790/#review101454

> A quick test on Linux shows that the event received in HandleDragEvent at the end of a drag is a MOUSE_UP, not a MOUSE_DRAG_END.
> 
> Are you seeing MOUSE_DRAG_END on a different platform?

My mistake, that must be a residual from my various attempts to get the test to pass.

I just double-checked and I'm getting MOUSE_UP, so I'm changing that for the next revision.
Also, I filed bug 1325747 to cover routing the synthesizeMouse events through APZ.
Comment on attachment 8821484 [details]
Bug 1324581 - call ScrollSnap() in AsyncPanZoomController when drag ends

https://reviewboard.mozilla.org/r/100790/#review101498
Attachment #8821484 - Flags: review?(botond) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b51431dfd912
check event handled by apz in nsSliderFrame::StartAPZDrag r=botond
https://hg.mozilla.org/integration/autoland/rev/18dbb713ee3c
call ScrollSnap() in AsyncPanZoomController when drag ends r=botond
https://hg.mozilla.org/mozilla-central/rev/b51431dfd912
https://hg.mozilla.org/mozilla-central/rev/18dbb713ee3c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.