Closed
Bug 1461708
Opened 4 years ago
Closed 4 years ago
Cannot capture middle-click paste event in a document
Categories
(Core :: DOM: Events, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: fatkasuvayu+mozilla, Assigned: masayuki)
References
Details
(Keywords: testcase)
Attachments
(9 files)
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 | |
46 bytes,
text/x-phabricator-request
|
Details | Review | |
46 bytes,
text/x-phabricator-request
|
Details | Review | |
46 bytes,
text/x-phabricator-request
|
Details | Review | |
46 bytes,
text/x-phabricator-request
|
Details | Review | |
46 bytes,
text/x-phabricator-request
|
Details | Review | |
46 bytes,
text/x-phabricator-request
|
Details | Review | |
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.
Updated•4 years ago
|
Has STR: --- → yes
Component: Untriaged → DOM: Events
Flags: needinfo?(nika)
Keywords: testcase
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
Comment 1•4 years ago
|
||
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•4 years ago
|
||
(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 | ||
Updated•4 years ago
|
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 4•4 years 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 5•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4ef182c477ea118e195de52387d2164465d5767
Assignee | ||
Comment 6•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c375509d3b5da0aa5a877a20638dd01e06a00870
Assignee | ||
Comment 7•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44eb0ad3e04a630874f9b567e272685af121182
Assignee | ||
Comment 8•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9a92e49b48392b339ffc72748f6571241a2627
Assignee | ||
Comment 9•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0f4afc1ba4c0aa5c75d430fd4b577fb62ee6cc8
Assignee | ||
Comment 10•4 years 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...
Assignee | ||
Comment 11•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41484df3d44f6d7d096d2e775fe324a3e5669491
Assignee | ||
Comment 12•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29bc4365d92642b94cb331c16d0eb8767a29efda
Assignee | ||
Comment 13•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=118982059832515b3343a7a88d2beaddf3ebc5d8
Comment 14•4 years 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 15•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c185d5387b546566731eb510ff462e9b0c4224a
Assignee | ||
Comment 16•4 years ago
|
||
Hmm, my patches are blocked by really rough and wrong "auxclick" event implementation...
Assignee | ||
Comment 17•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=703a22a8ef35afb3990714d52ed9b0d7bfae09c4
Assignee | ||
Comment 18•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b555a20dcec2e8f1ef27cde38bca002e803f2dfe
Assignee | ||
Comment 19•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03f9b37b1dc2cb5f7be5f9f8bc295244df9349f8
Assignee | ||
Comment 20•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21f0f2dc73090489fb3a8e79883b931fc3bd7531
Assignee | ||
Comment 21•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a64021b9fc2788953893b9656fe65ad35874e8
Assignee | ||
Comment 22•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=107a95a51e0a7339e3261b2cfeb78565d6c59fd4
Assignee | ||
Comment 23•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42cac92538afbb43226d268be3fb2b9aa4e9106
Assignee | ||
Comment 24•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60dd17227c2eda3ae4afa58487ffa0727c8b217f
Assignee | ||
Comment 25•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb6c67d625354a8281c22fcc64301a1a8b9e29ca
Assignee | ||
Comment 26•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5f40f563ea620570f58b8934ccc82d9784ad30
Assignee | ||
Comment 27•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89788259239d5e9ebdfae1de49f89e4ae2fa2e9b
Assignee | ||
Comment 28•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf9e8068bdec6ed1295e5df8d050e9ccfb81d8e
Assignee | ||
Comment 29•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0f990ee7231fc4c5b9ef41c40062b0b98250d8
Assignee | ||
Comment 30•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f3062878ac42c88e85f43ee460c9a935fd3236c
Assignee | ||
Comment 31•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b64825ec6a71b02a7be7fa264b3ee1be2b0bee33
Assignee | ||
Comment 32•4 years ago
|
||
I confirmed that with the last push's patches, I can paste with middle click in CodeMirror!
QA Contact: overholt
Assignee | ||
Comment 33•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b30b54178327c3f9fc24ce81c5e389d96351126
Assignee | ||
Comment 34•4 years 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 35•4 years ago
|
||
Assignee | ||
Comment 36•4 years 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•4 years 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•4 years 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•4 years ago
|
||
EventStateManager needs to handle middle click paste without editor. Therefore, the handler should be in EventStateManager.
Assignee | ||
Comment 40•4 years 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•4 years 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•4 years 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.
Updated•4 years ago
|
QA Contact: overholt
Assignee | ||
Comment 43•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d70ef3fa750334b53801541c215a529269dc4242
Updated•4 years ago
|
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•4 years 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•4 years 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•4 years 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.
Assignee | ||
Comment 47•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7a9adeb8e4606d6801fa39ce607432029163fc
Updated•4 years ago
|
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•4 years 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•4 years 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•4 years 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•4 years 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.
Assignee | ||
Comment 52•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=425d6b3454938fe98568f99feba9d4e14482c7e6
Comment 53•4 years 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
Comment 54•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/973ea1694606 https://hg.mozilla.org/mozilla-central/rev/588859b2d5cc https://hg.mozilla.org/mozilla-central/rev/d61721276fcc https://hg.mozilla.org/mozilla-central/rev/219527f4c312 https://hg.mozilla.org/mozilla-central/rev/95d15775e123 https://hg.mozilla.org/mozilla-central/rev/a22e64bb83ed https://hg.mozilla.org/mozilla-central/rev/0f2549de4e47 https://hg.mozilla.org/mozilla-central/rev/9e84f8d5b086
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•