Cannot capture middle-click paste event in a document

RESOLVED FIXED in Firefox 64

Status

()

Product:
Core
Component:
DOM: Events
Type:
defect
Importance:
P2
normal
Status:
RESOLVED FIXED
Opened:
Last year
Updated:
4 months ago

People

(Reporter: fatkasuvayu+mozilla, Assigned: masayuki)

Tracking

({testcase})

Version:
60 Branch
Target:
mozilla64
Platform:
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(9 attachments)

Minimal example demonstrating capture of middle-click paste events
351 bytes, text/html
Details
Bug 1461708 - part 1: Clean up EventStateManager::CheckForAndDispatchClick() with early-return style
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 2: Split EventStateManager::CheckForAndDispatchClick()
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 3: EventStateManager::InitAndDispatchClickEvent() shouldn't overwrite nsEventStatus with nsEventStatus_eIgnore
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 4: Move implementation of UIEvent::GetRangeParent() and UIEvent::RangeOffset() to nsLayoutUtils
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 5: Move EditorEventListener::HandleMiddleClickPaste() to EventStateManager
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 6: Make EditorEventListener::MouseClick() use WidgetMouseEvent instead of dom::MouseEvent
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 7: Make EventStateManager::HandleMiddleClickPaste() dispatch ePaste event by itself
46 bytes, text/x-phabricator-request
Details | Review
Bug 1461708 - part 8: Make EventStateManager handle middle click paste as a default action of mouseup event
46 bytes, text/x-phabricator-request
Details | Review
Reporter

Description

•
Last year 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20100101

Steps to reproduce:

In documents capturing (Linux) middle-click paste event is not working.  This is used in the code editor CodeMirror (https://github.com/codemirror/CodeMirror/issues/931), which is in turn used in Jupyter notebooks.

To reproduce the issue:
1. open the attached html file, 
2. select any text with your mouse anywhere on the desktop (say, a terminal),
3. try to middle-click paste in the html document


Actual results:

Nothing happens


Expected results:

The document should print "You've pasted something of types text/plain"

As far as I can tell, there was a very old similar issue (bug 846674).  It was resolved last year, and yet the issue persists (I also see this on Nightly).  You can try the above recipe on Chrome (or Midori), and it works as expected.

Updated

•
Last year
Has STR: --- → yes
Component: Untriaged → DOM: Events
Flags: needinfo?(nika)
Keywords: testcase
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
See Also: → https://github.com/codemirror/CodeMirror/issues/931

Comment 1

•
Last year 
This is almost certainly because pasting from the selection clipboard takes a different codepath internally from a normal paste event. I'm guessing that the fix to make the paste event always enabled wasn't set for pasting from selections...

:overholt, is there someone who has spare cycles to poke at this? I'm a bit overloaded right now.
Flags: needinfo?(nika) → needinfo?(overholt)

Comment 2

•
Last year 
(In reply to :Nika Layzell from comment #1)
> This is almost certainly because pasting from the selection clipboard takes
> a different codepath internally from a normal paste event. I'm guessing that
> the fix to make the paste event always enabled wasn't set for pasting from
> selections...
> 
> :overholt, is there someone who has spare cycles to poke at this? I'm a bit
> overloaded right now.

No, but let's keep it on the "soon" backlog.
Flags: needinfo?(overholt)
Priority: -- → P2

Comment 3

•
10 months ago 
Probably related to Bug 953389
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
 Assignee

Comment 4

•
10 months ago 
According to Chromium for Linux's behavior, we need to fix:

- event target of paste should be an element
- paste event should be fired after all mouse button events such as mousedown/mouseup/auxclick
 Assignee

Comment 10

•
10 months ago 
My patches will be in review process soon. However, I realized that there is a big problem. We have odd UI, that is "middlemouse.contentLoadURL". So, even after we start to dispatch "paste" event without HTMLEditor, on non-editable contents, middle click causes loading URL or searching something on the tab...

Comment 14

•
9 months ago 
FWIW, I used your comment #10 to solve a long standing issue for me. Thanks. It's here:

https://support.mozilla.org/en-US/questions/1233215
 Assignee

Comment 16

•
9 months ago 
Hmm, my patches are blocked by really rough and wrong "auxclick" event implementation...
Blocks: 953389
 Assignee

Comment 32

•
9 months ago 
I confirmed that with the last push's patches, I can paste with middle click in CodeMirror!
QA Contact: overholt
 Assignee

Comment 34

•
9 months ago 
I'll request review to smaug. Before that, I'd like to explain how my patches change middle click paste implementation.

I mention sometimes in the patches about our broken "auxclick" event implementation. If it's implemented ideally (including modifying "click" event handlers in our UI which handles non-primary buttons), middle click paste should be a default action of  all of mouseup/click(/dbleclick)/auxclick.  However, unfortunately, if we'd handle it after dispatching all click events, another click event handler might handle before the middle click paste handler.  For example, if user click an <a href> element in contenteditable, contentAreaUtils handles the click as "click on a link" even if it's editable.  When we fix this, we need risky changes, however, we need to fix this in 64 cycle because DevTools start to use CodeMirror (bug 953389 comment 22). So, we need to fix this bug without risks as far as possible.

Therefore, although the last patch ugly, we need to keep handling click event in editor with EditorEventListener.  However, if user clicks on non-editable element, we need to fire "paste" event.  My patches move middle click event handler from EditorEventListener to EventStateManager, but keep using it from EditorEventListener, and when middle click hasn't been handled by any editors, EventStateManager kicks the handler. This must be a good way to fix the "auxclick" implementation later.
 Assignee

Comment 36

•
9 months ago 
This patch splits EventStateManager::CheckForAndDispatchClick().  One is for
handling default action of eMouseUp, the other is for dispatching click events.

This makes it easier to add other default actions after dispatching click
events.
 Assignee

Comment 37

•
9 months ago 
EventStateManager::InitAndDispatchClickEvent() sends given nsEventStatus to
nsIPresShell::HandleEventWithTarget().  Then, it sends the status to
EventStateManager::PreHandleEvent() before dispatching the event.  At this
time, EventStateManager::PreHandleEvent() resets the state to
nsEventStatus_eIgnore.  Therefore, for example, if eMouseClick event is
consumed but eMouseAuxClick is ignored, the event status result is
nsEventStatus_eIgnore.  So, neither DispatchClickEvents() nor
PostMouseUpEventHandler() cannot check whether at least one click event
is consumed.

This patch makes EventStateManager::InitAndDispatchClickEvent() sends
local variable of nsEventStatus to nsIPresShell::HandleEventWithTarget().
Then, merge the result with current status.

If we'd change nsEventStatus to enum class, we could make this change as
custom "operator|=" or something.
 Assignee

Comment 38

•
9 months ago 
We need to move EditorEventListener::HandleMiddleClickPaste() into
EventStateManager to handle middle click paste after all click events are
dispatched.  This is preparation of the change.

HandleMiddleClickPaste() uses UIEvent::GetRangeParent() and
UIEvent::RangeOffset() to collapse Selection at clicked point.  However,
EventStateManager cannot access them since EventStateManager can handle it
with WidgetMouseEvent.  Fortunately, only WidgetMouseEvent is necessary for
implementing them.  Therefore, we can move the implementation into
nsLayoutUtils and merge them.
 Assignee

Comment 39

•
9 months ago 
EventStateManager needs to handle middle click paste without editor.
Therefore, the handler should be in EventStateManager.
 Assignee

Comment 40

•
9 months ago 
The event argument of only EditorEventListener::MouseClick() can be replaced
with WidgetMouseEvent simply.  So, for avoiding unnecessary RefPtr in
EditorEventListener::HandleEvent(), we should fix this now.
 Assignee

Comment 41

•
9 months ago 
Chromium dispatches "paste" event and pastes text with middle click even when
preceding mouse events are consumed.  On the other hand, our traditional
behavior is, neither dispatching "paste" event nor pasting clipboard content
is canceled only when click event is consumed before EditorEventListener
(EditorEventListener listens capturing phase of the event target).

This patch changes our behavior as:
- Dispatch "paste" event even if the preceding "click" event is consumed.
- Stop pasting clipboard content when the preceding "click" event is consumed.

The former must be important for compatibility with web apps.  The latter must
be better behavior than Chromium and application can cancel one of default
action properly (as we've already done).

Currently, ePaste event is dispatched from each implementation of handling
paste in editor.  However, it's complicated if we send event status for each
of them.  Therefore, this patch makes it optional whether they dispatch
ePaste event before pasting clipboard content.  So, only when middle click,
EventStateManager dispatches ePaste event instead of editor.
 Assignee

Comment 42

•
9 months ago 
This patch makes EventStateManager handle middle click paste as a default
action.

Unfortunately, we cannot remove the call of HandleMiddleClickPaste() in
EditorEventListener because it's important to consume middle click event
before any elements in the editor.  For example, if clicked HTMLEditor has
non-editable <a href> element, middle click event needs to be handled by the
editor rather than contentAreaUtils which handles click events of <a href>
elements.  The cause of this kind of issues is, any click event handlers
which handle non-primary button events still listen to "click" events.
Therefore, this patch makes HandleMiddleClickPaste() do nothing if the mouseup
event is fired on an editor.
QA Contact: overholt
Attachment #9014768 - Attachment description: Bug 1461708 - part 7: Make EventStateManager dispatch ePaste event even if preceding mouse events are consumed, but don't paste into our editor → Bug 1461708 - part 7: Make EventStateManager dispatch ePaste event even if preceding mouse events are consumed, but don't paste into our editor for backward compatibility

Comment 44

•
8 months ago 
So why we want "Stop pasting clipboard content when the preceding "click" event is consumed." ?
Why to fire 'paste' when nothing is actually pasted? Or am I misunderstanding something?
 Assignee

Comment 45

•
8 months ago 
(In reply to Olli Pettay [:smaug] from comment #44)
> So why we want "Stop pasting clipboard content when the preceding "click"
> event is consumed." ?

Since we've behaved so. I'm not trying to fix the issue of pasting behavior in this bug.

> Why to fire 'paste' when nothing is actually pasted?

For compatibility with Chromium. Chromium fires "paste" event even when there is no editor (i.e., even when Chromium does not paste it). So, that means that Chromium just exposes that user tries to paste something at that time.

On the other hand, I want to fix this bug for CodeMirror. So, if CodeMirror never consumes middle click, we can not fire "paste" event when preceding "click" event is consumed. I'll check this.
 Assignee

Comment 46

•
8 months ago 
I confirmed that even if we don't dispatch ePaste event if preceding click event is consumed, they handle "paste" event as expected. So, for now, not dispatching ePaste event nor not pasting clipboard content must be better. I'll update the patch.
Attachment #9014768 - Attachment description: Bug 1461708 - part 7: Make EventStateManager dispatch ePaste event even if preceding mouse events are consumed, but don't paste into our editor for backward compatibility → Bug 1461708 - part 7: Make EventStateManager::HandleMiddleClickPaste() dispatch ePaste event by itself

Comment 48

•
8 months ago 

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #46)
> I confirmed that even if we don't dispatch ePaste event if preceding click
> event is consumed, they handle "paste" event as expected. So, for now, not
> dispatching ePaste event nor not pasting clipboard content must be better.
> I'll update the patch.

Er, ok, now I'm getting lost with the changes.
I thought Gecko's current behavior is to not dispatch paste nor paste clipboard content if click is canceled.



I'm mostly worried about changing Gecko's behavior to something which doesn't map with what Blink does
(or any other browser engine?).
 Assignee

Comment 49

•
8 months ago 
(In reply to Olli Pettay [:smaug] from comment #48)
> 
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #46)
> > I confirmed that even if we don't dispatch ePaste event if preceding click
> > event is consumed, they handle "paste" event as expected. So, for now, not
> > dispatching ePaste event nor not pasting clipboard content must be better.
> > I'll update the patch.
> 
> Er, ok, now I'm getting lost with the changes.
> I thought Gecko's current behavior is to not dispatch paste nor paste
> clipboard content if click is canceled.

Yes, it's correct.

I'm trying to dispatch "paste" event even when there is no editor. I.e., Chromium dispatches "paste" event even if it does NOT paste anything. Therefore, I tried to take Chromium behavior only for the former since similar behavior reduces compatibility with the other browsers. On the other hand, about the default action behavior, i.e., for the latter, I think that backward compatibility is important especially when Chromium's behavior does not make sense. And also we have a comment pointing bug 70698. So, at least this would be regressed if we took the Chromium's behavior:
https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/editor/libeditor/EditorEventListener.cpp#662-664

> I'm mostly worried about changing Gecko's behavior to something which
> doesn't map with what Blink does
> (or any other browser engine?).

I understand. And I also worry about some web apps may change behavior with UA string or something which does NOT related to the changing point... (like keyCode/charCode/keypress dispatching issues)

Comment 50

•
8 months ago 
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #49)
> (In reply to Olli Pettay [:smaug] from comment #48)
> > 
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #46)
> > > I confirmed that even if we don't dispatch ePaste event if preceding click
> > > event is consumed, they handle "paste" event as expected. So, for now, not
> > > dispatching ePaste event nor not pasting clipboard content must be better.
> > > I'll update the patch.
> > 
> > Er, ok, now I'm getting lost with the changes.
> > I thought Gecko's current behavior is to not dispatch paste nor paste
> > clipboard content if click is canceled.
> 
> Yes, it's correct.
> 
> I'm trying to dispatch "paste" event even when there is no editor. I.e.,
> Chromium dispatches "paste" event even if it does NOT paste anything.

Ok. But the comment above says 
"So, for now, not dispatching ePaste event nor not pasting clipboard content must be better."
 Assignee

Comment 51

•
8 months ago 
(In reply to Olli Pettay [:smaug] from comment #50)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #49)
> > (In reply to Olli Pettay [:smaug] from comment #48)
> > > 
> > > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #46)
> > > > I confirmed that even if we don't dispatch ePaste event if preceding click
> > > > event is consumed, they handle "paste" event as expected. So, for now, not
> > > > dispatching ePaste event nor not pasting clipboard content must be better.
> > > > I'll update the patch.
> > > 
> > > Er, ok, now I'm getting lost with the changes.
> > > I thought Gecko's current behavior is to not dispatch paste nor paste
> > > clipboard content if click is canceled.
> > 
> > Yes, it's correct.
> > 
> > I'm trying to dispatch "paste" event even when there is no editor. I.e.,
> > Chromium dispatches "paste" event even if it does NOT paste anything.
> 
> Ok. But the comment above says 
> "So, for now, not dispatching ePaste event nor not pasting clipboard content
> must be better."

Yes, I meant that when one of preceding click events is consumed, we should not dispatch ePaste event nor paste clipboard content even if there is editor.

Comment 53

•
8 months ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/973ea1694606
part 1: Clean up EventStateManager::CheckForAndDispatchClick() with early-return style r=smaug
https://hg.mozilla.org/integration/autoland/rev/588859b2d5cc
part 2: Split EventStateManager::CheckForAndDispatchClick() r=smaug
https://hg.mozilla.org/integration/autoland/rev/d61721276fcc
part 3: EventStateManager::InitAndDispatchClickEvent() shouldn't overwrite nsEventStatus with nsEventStatus_eIgnore r=smaug
https://hg.mozilla.org/integration/autoland/rev/219527f4c312
part 4: Move implementation of UIEvent::GetRangeParent() and UIEvent::RangeOffset() to nsLayoutUtils r=smaug
https://hg.mozilla.org/integration/autoland/rev/95d15775e123
part 5: Move EditorEventListener::HandleMiddleClickPaste() to EventStateManager r=smaug
https://hg.mozilla.org/integration/autoland/rev/a22e64bb83ed
part 6: Make EditorEventListener::MouseClick() use WidgetMouseEvent instead of dom::MouseEvent r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0f2549de4e47
part 7: Make EventStateManager::HandleMiddleClickPaste() dispatch ePaste event by itself r=smaug
https://hg.mozilla.org/integration/autoland/rev/9e84f8d5b086
part 8: Make EventStateManager handle middle click paste as a default action of mouseup event r=smaug
Depends on: 1521396
You need to log in before you can comment on or make changes to this bug.