1528289 - Middle click-hold-release fires paste event after autoscrolling

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P3)

Description

[gwarser]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

• open facebook.com
• open chat window with someone you chatted earlier (to have scrollback history)
• select some text on page to fill middle click clipboard
• middle click and hold on old message in chat window and move mouse a bit to autoscroll
• release mouse button

Actual results:

middle click clipboard content is pasted into text area on the bottom of the chat window

Expected results:

• paste should not happen

Note:

• paste does not happen if I simply middle-click once, without moving mouse
• paste does not happen if I middle-click to enter auto-scroll mode, perform autoscrolling and then middle-click again to exit auto-scroll

Related:

https://bugzilla.mozilla.org/show_bug.cgi?id=1461708
https://bugzilla.mozilla.org/show_bug.cgi?id=1521396

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Sorry for the delay.

I got it. If I move selection at middle-mousedown, I see same behavior as Chrome.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Hmm, even if I change our "paste" event behavior exactly same as Chromium on Linux, Facebook handles the "paste" event. So, I guess that pasting issue on Facebook is not caused by this incompatibility.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

I think that we should stop dispatching eClick, eAuxClick and ePaste event when mouseup event is fired during auto-scroll. Perhaps, from AutoScrollController.jsm, we need to make ESM not dispatch them. However, I think that there is no good API to do that because we fire eClick and eAuxClick events even when eMouseUp is consumed, and "prevent multiple actions" is not useful because we've already used it for preventing double action of middle mouse paste.

Can I add new chrome API for MouseEvent?

Comment 4

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

Yeah, I agree we should stop dispatching those events when user is using the rarely used auto scrolling.

What kind of API would you need? Can it be something generic which might be useful with some other events too (and then add it as [ChromeOnly] to Event, and not just MouseEvent)?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Thanks, I'm thinking about [ChormeOnly] method of MouseEvent, e.g.,MouseEvent.preventFollowingClickEvent() or something. I think that it's really complicated if we add new generic API to Event since there are already Event.peventDefault() and Event.preventMultipleActions(). So, adding specific name API can avoid to make them more complicated.

Comment 6

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

I see. Could there be cases when key event initiated click should be prevented? If so, perhaps the API could live in UIEvent? But also, the API can be easily moved there later, if there is actual use case.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Ah, sorry, I misunderstood. All mouse events are propagated from auto-scroll popup to main window intentionally with using mousethrough="always". I'm investigating how can I prevent only click event in this case (mouseup event is propagated even on Chrome, but does not cause click/auxclick event).

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Ah, okay. Anyway, the new API is necessary. I'll try to write an automated test, but I'm not sure whether I can write it since the autoscroll popup is the main process's, but content is the content process's...

Comment 9

[gwarser]

Because of this bug, imgur constantly nags me about "URL failed to upload".

• Linux, autoscroll enabled
• copy https://imgur.com/a/TwY6q to clipboard
• open https://imgur.com/a/TwY6q
• autoscroll by press middle mouse button on the page margin -> hold and move -> release
• small "bubble" appears on top with "URL Failed to Upload" message

I did not associated this behavior with this issue at first, but now I wonder how many text snippets from my clipboard has been sent to imgur.

I will be better with middlemouse.paste turned off.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Resetting assignee which I don't work on in this several months.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1528289 - part 1: Move selection at middle button down rather than middle button up r=edgar!

Chrome and Safari move selection at middle button down and does not modify the
range at middle button up. However, they handle middle button down with
Shift key is "continue selection". So, we should handle selection in
nsIFrame when mousedown event for middle mouse button is fired, but ignore
multiple selection, drag and drop and maintaining selection for aligning the
behavior to the other browsers.

This patch splits nsIFrame::HandlePress() and calls new method which
moves selection from nsIFrame::HandleEvent() when middle button is pressed.
(Note that this patch does not check whether middle click paste is enabled
because Chrome moves selection even on Windows which Chrome always disable
middle click paste on.)

With this change, "paste" event target is changed. Previously, we used target
of the preceding mouseup event, but we start to use the target of the
preceding mousedown event.

Note that even with this patch, we still behave differently from Chrome even
in the following cases:

• middle mouse button down in selected range, we collapse it, but Chrome keeps
  the selection.
• middle mouse button click in selected range, we dispatch "paste" event, but
  Chrome collapse selection and not dispatch "paste" event.
• middle mouse button down in selected range and up in different element,
  Chrome does not modify the range nor dispatch "paste" event.
• Shift + middle mouse button in editable <table> works as non-editable
  in Chrome, but our editor handles it with special path. Therefore, we
  don't modify the range but dispatch "paste" event in the selected range.

Changing them requires bigger change and probably requires some other features'
behavior changes. Therefore, we shouldn't touch these issues until they are
actually reported as web-compat issues.

Depends on D103996

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=Gijs!,edgar!

Chrome behaves like this:

1. When user starts autoscroll with a middle click, mousedown and mouseup
   are fired, but auxclick nor paste event is not fired.
2. When user ends autoscroll with a left click, only mouseup event is fired.
   I.e, mousedown nor click event is not fired.
3. When user ends autoscroll with a middle click, only mouseup event is fired.
   I.e., mousedown, auxclick nor paste events is not fired.
4. When user ends autoscroll with a right click, mouseup and contextmenu
   events are fired, but mousedown and auxclick events are not fired.

This patch emulates these Chrome's behavior as far as possible. However,
unfortunately, we cannot do exactly same behavior without some big patches
because each widget (nsWindow or nsChildView) discards a mouse event
which rolled up a widget before dispatching it into the DOM. Therefore,
for now, this patch does not fix the following issues:

1. mousedown event is not fired in content when clicking outside the
   autoscroller to close it except when pressing the secondary button or on any
   buttons on Linux.
2. mouseup event is not fired in content when clicking outside the
   autoscroller to close it except when pressing the primary button macOS.
3. click event and auxclick events are fired when clicking outside the
   autoscroller with the secondary button.

So, the middle button click/auxclick events and paste event which is
reported to the bug won't be fired with this patch. I'll file follow up bugs.

Depends on D103997

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be5cf87707f9
part 1: Move selection at middle button down rather than middle button up r=edgar
https://hg.mozilla.org/integration/autoland/rev/33c7b633ada2
part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=Gijs,edgar

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27850 for changes under testing/web-platform/tests

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/be5cf87707f9
https://hg.mozilla.org/mozilla-central/rev/33c7b633ada2

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This has been backed out for for breaking navigation with clicks after the user scrolled by clicking with middle mouse button:

https://hg.mozilla.org/mozilla-central/rev/c7e489f5759a6dc1e0dabdab8772339410e5e12b

Steps to reproduce:

1. Click with the mouse wheel / middle mouse button.
2. Move the mouse up and down to scroll.
3. Click on the page to stop the scrolling.
4. Try to click a link with the left mouse button (behavior: expected navigation doesn't happen).

Comment 17

[Bernard Igiri]

Attachment: Backed out 2 changesets (Bug 1528289) for breaking navigation with clicks after user scrolled by clicking with middle mouse button. a=backout

Backed out changeset 33c7b633ada2 (bug 1528289)
Backed out changeset be5cf87707f9 (bug 1528289)

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Backed out 2 changesets (Bug 1528289) for breaking navigation with clicks after user scrolled by clicking with middle mouse button. a=backout r=emilio

Backed out changeset 33c7b633ada2 (bug 1528289)
Backed out changeset be5cf87707f9 (bug 1528289)

Comment 19

[Micah [:mtigley] (she/her)]

Attachment: Backed out 2 changesets (Bug 1528289) for breaking navigation with clicks after user scrolled by clicking with middle mouse button. a=backout

Backed out changeset 33c7b633ada2 (bug 1528289)
Backed out changeset be5cf87707f9 (bug 1528289)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1528289 - part 3: Make `AutoScrollChild` check `this._scrollable` whether the autoscroller is open or closed r=Gijs!

I was confused by the old flag this._ignoreMouseEvents. It's now set to
true by startAutoscroll, but stopScroll does not set it to false.
Instead, this._scrollable is available for this purpose.

Then, the test does not pass only on 32bit Windows and macOS. The failure is,
when clicking on left mouse button, click event is fired in the content.
The difference from the other platforms, the click is handled by APZ on them.
Therefore, there is no chance to consume click event in AutoScrollParent,
AutoScrollChild nor browser-custom-element.

Depends on D104652

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1528289 - part 4: Prevent `click` event if APZC cancels autoscroll synchronously r=botond!

The automated tests become orange only in macOS and 32bit Windows builds. It
fails when left mouse button down is synthesized in the autoscroller (a XUL
<panel> element). Although I'm not sure why that depends on the platform,
APZ cancels active autoscrolling before dispatching mousedown event.
Therefore, AutoScrollParent nor AutoScrollChild cannot block the following
click event.

Therefore, this patch adds new field into APZEventResult and set it to true
when autoscroll is synchronously canceled by a mouse input. Then,
nsBaseWidget can prevent following click event of eMouseDown and eMouseUp
events as expected before dispatching them into the DOM tree. (FYI: Preventing
click event of either eMouseDown or eMouseUp can prevent the following
click event, etc.)

Depends on D107324

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

botond: ping for review.

Comment 23

[Botond Ballo [:botond]]

Sorry, a bit backed up with reviews. I hope to look at it tomorrow.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

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

Sorry, a bit backed up with reviews. I hope to look at it tomorrow.

No problem, thanks in advance!

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b537cd918f2d
part 1: Move selection at middle button down rather than middle button up r=edgar
https://hg.mozilla.org/integration/autoland/rev/6b4c18f087b7
part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=Gijs,edgar
https://hg.mozilla.org/integration/autoland/rev/f315d81e17f9
part 3: Make `AutoScrollChild` check `this._scrollable` whether the autoscroller is open or closed r=Gijs
https://hg.mozilla.org/integration/autoland/rev/889132330748
part 4: Prevent `click` event if APZC cancels autoscroll synchronously r=botond

Comment 27

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b537cd918f2d
https://hg.mozilla.org/mozilla-central/rev/6b4c18f087b7
https://hg.mozilla.org/mozilla-central/rev/f315d81e17f9
https://hg.mozilla.org/mozilla-central/rev/889132330748

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot