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

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Thomas Wisniewski]

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.

Comment 1

[Thomas Wisniewski]

Attachment: testcase.html

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(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?

Comment 3

[Botond Ballo [:botond]]

This is next on my list of bugs to look into.

Comment 4

[Botond Ballo [:botond]]

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.

Comment 5

[Botond Ballo [:botond]]

(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

Comment 6

[Botond Ballo [:botond]]

(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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

#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.)

Comment 8

[Botond Ballo [:botond]]

Sure, I can give it a try.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1365761 - Do not dispatch mouse-move events targeted as a slider frame to web content.

Review commit: https://reviewboard.mozilla.org/r/145898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145898/

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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()

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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 12

[Botond Ballo [:botond]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145898/diff/1-2/

Comment 13

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=192c00e47c6720d0d65b364cb71369be8d522f03

Comment 14

[Botond Ballo [:botond]]

(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 15

[Botond Ballo [:botond]]

Attachment: Bug 1365761 - Modify the mochitest for bug 1328065 to avoid relying on receiving a mouse event while the mouse is over the scrollbar.

Review commit: https://reviewboard.mozilla.org/r/145988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145988/

Comment 16

[Botond Ballo [:botond]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145898/diff/2-3/

Comment 17

[Botond Ballo [:botond]]

(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

[Kartikaya Gupta (email:[e-mail])]

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

Comment 19

[Botond Ballo [:botond]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145988/diff/1-2/

Comment 20

[Botond Ballo [:botond]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145898/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8b5660430d2
https://hg.mozilla.org/mozilla-central/rev/4dd4e37e40f7