Closed Bug 1461708 Opened 3 years ago Closed 2 years ago

Cannot capture middle-click paste event in a document

Categories

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

Product:
Core
Component:
DOM: Events
Version:
60 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: fatkasuvayu+mozilla, Assigned: masayuki)

References

Details

(Keywords: testcase)

Attachments

(9 files)

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 
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.
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
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)
(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
Probably related to Bug 953389
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
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...
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
Hmm, my patches are blocked by really rough and wrong "auxclick" event implementation...
Blocks: 953389
I confirmed that with the last push's patches, I can paste with middle click in CodeMirror!
QA Contact: overholt
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.
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.
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.
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.
EventStateManager needs to handle middle click paste without editor.
Therefore, the handler should be in EventStateManager.
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.
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.
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
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?
(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.
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

(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?).
(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)
(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."
(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.
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.