Firefox fires mousemove events while dragging content scrollbars, other browsers do not.

RESOLVED FIXED in Firefox 55

Status

()

Core
Event Handling
P3
normal
RESOLVED FIXED
11 months ago
4 months ago

People

(Reporter: Thomas Wisniewski, Assigned: botond)

Tracking

Trunk
mozilla55
Points:
---
Bug Flags:
webcompat ?

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [webcompat] [gfx-noted], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

11 months ago
While investigating webcompat issue #6627, I noticed that Firefox nightly fires mousemove events while the scrollbar is being mouse-dragged by its button. Chrome, Safari, and Edge do not fire these events.

This seems to be the root cause for why the page's scrollbar isn't working properly on Firefox nightly (the scroll direction ends up inverted while dragging the scrollbar button).

I've attached the trivial testcase I wrote to confirm whether the events are fired in different browsers.

Note that this may in fact be related to bug 703698 (which was closed for not having a test-case).

Also note that disabling APZ scrollbar dragging (apz.drag.enabled, bug 1324117) seems to let the site's scrollbar work as expected, though Firefox is still firing the extra mousemove events. I can't quite tell why.
(Reporter)

Updated

11 months ago
Summary: Firefox fires mousemove events while dragging content scrollbars, Chrome does not. → Firefox fires mousemove events while dragging content scrollbars, other browsers do not.
(Reporter)

Comment 1

11 months ago
Created attachment 8868763 [details]
testcase.html

Updated

11 months ago
Flags: webcompat?
(In reply to Thomas Wisniewski from comment #0)
> Also note that disabling APZ scrollbar dragging (apz.drag.enabled, bug
> 1324117) seems to let the site's scrollbar work as expected, though Firefox
> is still firing the extra mousemove events. I can't quite tell why.

Any idea for this issue?
Flags: needinfo?(bugmail)
Priority: -- → P3
Whiteboard: [webcompat] → [webcompat] [gfx-noted]
(Assignee)

Comment 3

11 months ago
This is next on my list of bugs to look into.
Flags: needinfo?(bugmail) → needinfo?(botond)
(Assignee)

Comment 4

11 months ago
diagnosis
The page in question implements scrolling by mouse-panning in JS. That is, it registers mouse listeners that scroll the page in such a way that, during a mouse-drag, the point on the page that was under the cursor remains under the cursor (just as with touch-panning).

The mouse listener does not discriminate between the case where the drag is over the page content, and the case where the drag is over the scrollbar. (This is understandable, since in other browsers, dragging the scrollbar doesn't fire mouse-move events, so no such discrimination is necessary, and even in Firefox, up until the recent introduction of async scrollbar dragging, running the mouse-move listener during scrollbar dragging didn't result in any user-visible bad behaviour.)

Now, during mouse-panning, dragging to the right causes the page to scroll to the left. During scrollbar dragging, draggging to the right causes the page to scroll to the right. If the browser's scrollbar dragging logic and the page's mouse-panning logic run at the same time, we get scroll events with conflicting scroll directions that cause the scroll position to jump back and forth, as observed.

With main-thread scrollbar dragging, this doesn't happen, because the events are always sequenced such that the page's mouse-panning logic runs first, and the browser's scrollbar-dragging logic runs second, so the scrollbar-dragging logic prevails, and you get the scrolling you expect from the scrollbar drag.

With async scrollbar dragging, there is no such guarantee on the ordering, and we thus end up with some composited frames that reflect the scroll position set by the page's mouse-panning logic, and some frames that reflect the scroll position set by the scrollbar dragging logic.

It seems like we could take one of three approaches to resolving this:

  1) Make sure that the same ordering is respected with async scrolling as with
     main-thread scrolling. That is, make sure that the scrollbar-dragging
     logic continues to always "win" in such a case.

  2) Don't send mouse-move events to the page during scrollbar dragging. This
     would match the behaviour of other browsers.

  3) Treat this as a Tech Evangelism issue, and ask that the page's author
     modify their mouse listener to detect when the drag is over the
     scrollbar, and not try to perform mouse-panning in that case.
Flags: needinfo?(botond)
(Assignee)

Comment 5

11 months ago
(In reply to Botond Ballo [:botond] from comment #4)
> With main-thread scrollbar dragging, this doesn't happen, because the events
> are always sequenced such that the page's mouse-panning logic runs first,
> and the browser's scrollbar-dragging logic runs second

More specifically, that's because the page's mouse listener is invoked earlier in EventTargetChainItem::HandleEventTargetChain() [1], where we are handling (I guess) regular listeners, while the main-thread scrollbar dragging logic is invoked later in that function [2] where we invoke a callback object provided by the PresShell.

[1] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/dom/events/EventDispatcher.cpp#442
[2] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/dom/events/EventDispatcher.cpp#510
(Assignee)

Comment 6

11 months ago
(In reply to Botond Ballo [:botond] from comment #4)
> It seems like we could take one of three approaches to resolving this:
> 
>   1) Make sure that the same ordering is respected with async scrolling as with
>      main-thread scrolling. That is, make sure that the scrollbar-dragging
>      logic continues to always "win" in such a case.
> 
>   2) Don't send mouse-move events to the page during scrollbar dragging. This
>      would match the behaviour of other browsers.
> 
>   3) Treat this as a Tech Evangelism issue, and ask that the page's author
>      modify their mouse listener to detect when the drag is over the
>      scrollbar, and not try to perform mouse-panning in that case.

I gave this some more thought, and I couldn't think of a good way to do #1. We generally don't want APZ to "overwrite" scroll actions performed by the page, and we have sophisticated logic involving scroll origins and scroll generation numbers that makes sure we do the right thing. A fix that doesn't break all that would necessary have to be specific to scrollbars, in which case we might as well just do #2.

Moreover, #2 would solve some other instances where generating mouse-move events during a scroll thumb drag gives people grief (Markus was showing me a case in the Gecko profiler today).

So, I'm recommending that we go with approach #2, and moving the bug to DOM: Events accordingly.
Component: Panning and Zooming → DOM: Events

Comment 7

11 months ago
#2 should be relatively easy. We could for example mark those mousemove events chrome only when dragging a scrollbar.

(I don't expect to have time for this very soon, so feel free to take.)
Component: DOM: Events → Event Handling
(Assignee)

Comment 8

11 months ago
Sure, I can give it a try.
Assignee: nobody → botond
Comment hidden (mozreview-request)

Comment 10

11 months ago
mozreview-review
Comment on attachment 8874530 [details]
Bug 1365761 - Do not dispatch mouse-move events targeted as a slider frame to web content.

https://reviewboard.mozilla.org/r/145898/#review149820

::: layout/generic/nsIFrame.h:2595
(Diff revision 1)
>  
> +  /**
> +   * Returns true if events of the given type targeted at this frame
> +   * should be marked as chrome-only.
> +   */
> +  virtual bool ShouldMarkEventAsChromeOnly(mozilla::EventMessage aMessage) const

Perhaps call this OnlySystemGroupDispatch

::: layout/xul/nsSliderFrame.cpp:1642
(Diff revision 1)
>      mSuppressionActive = false;
>    }
>  }
>  
> +bool
> +nsSliderFrame::ShouldMarkEventAsChromeOnly(EventMessage aMessage) const {

nit, { goes to its own line

::: layout/xul/nsSliderFrame.cpp:1645
(Diff revision 1)
>  
> +bool
> +nsSliderFrame::ShouldMarkEventAsChromeOnly(EventMessage aMessage) const {
> +  // Do not dispatch mouse-move events targeted at the slider frame to
> +  // web content. This matches the behaviour of other browsers.
> +  return aMessage == eMouseMove;

This would break sliders in browser chrome. I'm pretty sure we want this only when
aMessage == eMouseMove && GetContent()->IsInNativeAnonymousSubtree()
Attachment #8874530 - Flags: review?(bugs) → review+

Comment 11

11 months ago
mozreview-review
Comment on attachment 8874530 [details]
Bug 1365761 - Do not dispatch mouse-move events targeted as a slider frame to web content.

https://reviewboard.mozilla.org/r/145898/#review149822

::: layout/base/PresShell.cpp:8126
(Diff revision 1)
>        }
>        if (aEvent->IsAllowedToDispatchDOMEvent()) {
>          MOZ_ASSERT(nsContentUtils::IsSafeToRunScript(),
>            "Somebody changed aEvent to cause a DOM event!");
>          nsPresShellEventCB eventCB(this);
> +        if (GetCurrentEventFrame()->ShouldMarkEventAsChromeOnly(aEvent->mMessage)) {

Hmm, GetCurrentEventFrame may return null here I think. Add a null check
Comment hidden (mozreview-request)
(Assignee)

Comment 14

11 months ago
(In reply to Botond Ballo [:botond] from comment #13)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=192c00e47c6720d0d65b364cb71369be8d522f03

^ This passes all tests, except for the helper_scroll_over_scrollbar subtest in gfx/layers/apz/test/mochitest/test_group_wheelevents.html.

That test uses moveMouseAndScrollWheelOver() from apz_test_native_event_utils.js [1] to move the mouse over the scrollbar and then scroll the scroll-wheel there. moveMouseAndScrollWheelOver() registers a mouse-move event listeners, generates a mouse move event at the destination, waits for the mouse-move event to arrive, and then generates a mouse-wheel event. Naturally, since this patch stops dispatching mouse-move events when we're over the scrollbar, that listener never runs, and the test hangs.

The comment above moveMouseAndScrollWheelOver() gives the following reasoning for generating a mouse event:

  // Moving the mouse is necessary to avoid wheel events from two consecutive
  // moveMouseAndScrollWheelOver() calls on different elements being incorrectly
  // considered as part of the same wheel transaction.

However, that's not necessary for this test, because this test only generates a single wheel event. So, just generating the wheel event directly, without a mouse-move first, should be sufficient.

[1] http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js#249
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
(In reply to Botond Ballo [:botond] from comment #14)
> However, that's not necessary for this test, because this test only
> generates a single wheel event. So, just generating the wheel event
> directly, without a mouse-move first, should be sufficient.

Try push for this approach:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=615e8dc21de08e22430e58c130e95eb5d25a3f6d

Comment 18

11 months ago
mozreview-review
Comment on attachment 8874644 [details]
Bug 1365761 - Modify the mochitest for bug 1328065 to avoid relying on receiving a mouse event while the mouse is over the scrollbar.

https://reviewboard.mozilla.org/r/145988/#review149924

r+ with trailing whitespace removed
Attachment #8874644 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

11 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8b5660430d2
Modify the mochitest for bug 1328065 to avoid relying on receiving a mouse event while the mouse is over the scrollbar. r=kats
https://hg.mozilla.org/integration/autoland/rev/4dd4e37e40f7
Do not dispatch mouse-move events targeted as a slider frame to web content. r=smaug

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8b5660430d2
https://hg.mozilla.org/mozilla-central/rev/4dd4e37e40f7
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See Also: → bug 1352332
You need to log in before you can comment on or make changes to this bug.