Closed Bug 1528289 Opened 3 years ago Closed 8 months ago

Middle click-hold-release fires paste event after autoscrolling

Categories

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

67 Branch
Desktop
All
defect

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox89 --- fixed

People

(Reporter: gwarser, Assigned: masayuki)

References

(Regressed 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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

Component: Untriaged → DOM: Events
Product: Firefox → Core
Flags: needinfo?(masayuki)
OS: Unspecified → Linux

Sorry for the delay.

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

Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(masayuki)

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.

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?

Flags: needinfo?(bugs)

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

Flags: needinfo?(bugs)

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.

Flags: needinfo?(bugs)

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.

Flags: needinfo?(bugs)
Priority: -- → P3

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

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

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.

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

Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Assignee: nobody → masayuki
Blocks: 1686662
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: Unspecified → Desktop

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

Attachment #9201055 - Attachment is obsolete: true
No longer blocks: 1686662
Attachment #9201055 - Attachment is obsolete: false

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

Attachment #9202264 - Attachment description: Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=smaug!,Gijs! → Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=edgar!,Gijs!
Pushed by masayuki@d-toybox.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27850 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

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).
Status: RESOLVED → REOPENED
Flags: needinfo?(masayuki)
Resolution: FIXED → ---
Target Milestone: 88 Branch → ---
Attachment #9206693 - Attachment is obsolete: true
Attachment #9206752 - Attachment is obsolete: true
Attachment #9207027 - Attachment is obsolete: true
Attachment #9201055 - Attachment description: Bug 1528289 - part 1: Move selection at middle button down rather than middle button up r=edgar! → Bug 1528289 - part 1: Move selection at middle button down rather than middle button up r=edgar
Attachment #9202264 - Attachment description: Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=edgar!,Gijs! → Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=Gijs,edgar

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

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

Attachment #9201055 - Attachment description: Bug 1528289 - part 1: Move selection at middle button down rather than middle button up r=edgar → Bug 1528289 - part 1: Move selection at middle button down rather than middle button up r=edgar!
Attachment #9202264 - Attachment description: Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=Gijs,edgar → Bug 1528289 - part 2: Dispatch same events on the web contents when autoscroll is canceled with a click r=Gijs!,edgar!

botond: ping for review.

Flags: needinfo?(masayuki) → needinfo?(botond)

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

Flags: needinfo?(botond)

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

Upstream PR was closed without merging
Pushed by masayuki@d-toybox.com:
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
Upstream PR was closed without merging
Upstream PR merged by moz-wptsync-bot
Regressions: 1701905
Regressions: 1714810
Regressions: 1715603
Regressions: 1716085
Regressions: 1716068
See Also: → 1716883
Regressions: 1721313
You need to log in before you can comment on or make changes to this bug.