Bugzilla – Full Text Bug Listing
| Summary: | [GTK/X11] middle clicking in Orion should paste the X primary selection | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Mihai Sucan <mihai.sucan> |
| Component: | Editor | Assignee: | Felipe Heidrich <eclipse.felipe> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | eclipse.felipe, mihai.sucan |
| Version: | unspecified | ||
| Target Milestone: | 0.4 M1 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
|
Description
Mihai Sucan
We handled this case in StyledText easily during handleMouseDown and testing button===2. But here we have all the same problems described in bug 361471 comment 2 - we wont be able to access the clipboard during mouse down - we can identify which clipboard to use (In reply to comment #0) > Actual result: nothing happens. on a second thought provided that we are using a contentEditable should we getting a paste event for this case ? (In reply to comment #2) > (In reply to comment #0) > > Actual result: nothing happens. > > on a second thought > provided that we are using a contentEditable > should we getting a paste event for this case ? I didn't get the chance to test this yet, but I expect the answer is yes. We should receive a paste event when the user middle clicks. If yes, then this bug is easy to fix. My only concern is with different configs where middle click does scroll instead of paste. This would need a bit of testing. See bug 361471 comments 6 to 9. The overlayDiv is the main reason why middle click paste is not working on the textView. That said, removing the overlayDiv does not quite fix the problem. When overlayDiv was removed the text was pasted in the view, but the "paste" event was not sent. That means that the view did not see the text being instered (e.g. the text is not inserted in the model). Thus, the next time the view is updated the text is lost. Chrome and Opera have the exact same behaviour. Mihai, in my opnion this is bug in the user agent, what is your opnion ? (In reply to comment #4) > See bug 361471 comments 6 to 9. > > The overlayDiv is the main reason why middle click paste is not working on the > textView. > That said, removing the overlayDiv does not quite fix the problem. > When overlayDiv was removed the text was pasted in the view, but the "paste" > event was not sent. That means that the view did not see the text being > instered (e.g. the text is not inserted in the model). Thus, the next time the > view is updated the text is lost. > > Chrome and Opera have the exact same behaviour. > > Mihai, in my opnion this is bug in the user agent, what is your opnion ? Thanks for your investigation. I also checked things and did some patch work. The paste event is sent when you middle click on the contentEditable element. The overlayDiv is not contentEditable and Firefox won't trigger paste. I am not sure if this is really a bug. Anyhow, I looked into the actual need of overlayDiv in Firefox and I disabled it. This makes paste event show up and middle-click to paste works for me. Did some work to allow middle click to happen at the mouse location. Now selection is also copied to clipboard, but the whitespace is lost, because our native code keeps whitespace only in PRE elements. That got me down the rabbit hole of switching to PRE. I switched clientDiv to PRE and the line DIVs to SPANs (otherwise it won't work...). This also prompted the need of "\r\n" in each SPAN at the end of the line (otherwise no newlines are pasted in other editors). With these problems fixed ... there's only one last problem: when I try to make selections backwards there's something that keeps deselecting the text. When I do forward-selections they work fine - I checked the arguments going through the code path of handleMouseMove (setSelectionTo, setSelection, showCaret, and so on). They all seem to be fine - I haven't noticed what's the difference when I select backward versus forward - hence I don't know what breaks. Here's the code: https://github.com/mihaisucan/orion.client/tree/bug-361474 Thoughts? Also, why is overlayDiv needed for Firefox? I saw some events are handled for overlayDiv, but from a quick check... they should work with clientDiv as well. Thank you! (Please note the patch is really rough, console.log() calls, not tested in other browsers and certainly unpolished) (Additionally, selection that goes beyond the boundary of the visible code which is in the DOM will not go into the PRIMARY clipboard. That was expected and we can't do anything about this issue.) my plan was to call _doPaste(e) from mouse down when button === 2. the overlayDiv is used in Firefox to stop the user agent from selecting text. There is no way to do that in Firefox, at least when we started back in FF 3. I have tested recently and we can prevent mouse down to stop selection and use clientDiv.setCapture() to capture the mouse (so drag scroll selection works when the mouse is outside the client area). Using that we can remove the overlayDiv, but that does not help much nor improves performance... (In reply to comment #6) > my plan was to call _doPaste(e) from mouse down when button === 2. Would this work? If I am not mistaken it could work only if execCommand("paste") is allowed. I tried to do clientDiv.focus() or clipboardDiv.focus() in mousedown for button === 2 and that didn't work. It seems Firefox only triggers paste if the element it generated mousedown for has contentEditable=true. So, as long as that element is overlayDiv or some other contentEditable=false element, paste doesn't happen. > the overlayDiv is used in Firefox to stop the user agent from selecting text. > There is no way to do that in Firefox, at least when we started back in FF 3. > I have tested recently and we can prevent mouse down to stop selection and use > clientDiv.setCapture() to capture the mouse (so drag scroll selection works > when the mouse is outside the client area). Using that we can remove the > overlayDiv, but that does not help much nor improves performance... I don't expect the removal of overlayDiv to improve perf either. It would just be one less hack for Firefox - IIRC, overlayDiv is only for Firefox. The added bonus would be that middle click would also work. Thank you for looking into this! With the latest changes in Orion, the removal of overlayDiv, middle click works. The paste event is generated and it works almost as expected. The only remaining problem is that the pasted content is always inserted at the caret location, instead of the mouse location. I have updated my previous patch from my github branch (see comment 5). I took only the code that deals with the mouse location on-middle-click. This is a minimal patch that I hope is safe to land: https://github.com/mihaisucan/orion.client/tree/bug-361474-2 Please review it and test. I only tested with Firefox. Thank you! I reviewed the code. I only saw one problem maybe 2. a) it compares x,y,time of the previous mouse down to the current mouse down but does not check it it happens on the same button. For example, click on button 3 then on button 1 within click time. The view will select the word under the cursor instead of placing the caret. b) I think that in _getClipboardText() the code that fixes the insertion position should only run on Linux. I could not make this happen (my mouse has a clumsy wheel for button 2), but theoretically one could generate a mouse down event with button equals to 2 followed by a paste event (generate by keyboard) in last the clickTime. If that happens the paste would occur in the wrong place for windows and mac. Anyhow, I can make the small changes above. The one thing I would like to ask you is about the behaviour. Your code the paste text is inserted at the mouse location without changing the selection. as far as I tested only gedit has that behaviour (and likely all other gtk based applications). Firefox textarea and contentEditable don't do that (they move the selection to the offset where the text is being inserted). Chrome is like Firefox. Personally I find the gedit behaviour bad, what do you think ? Hello Felipe! Thank you for the code review! (In reply to comment #9) > I reviewed the code. I only saw one problem maybe 2. > > a) it compares x,y,time of the previous mouse down to the current mouse down > but does not check it it happens on the same button. > For example, click on button 3 then on button 1 within click time. The view > will select the word under the cursor instead of placing the caret. > > b) I think that in _getClipboardText() the code that fixes the insertion > position should only run on Linux. I could not make this happen (my mouse has a > clumsy wheel for button 2), but theoretically one could generate a mouse down > event with button equals to 2 followed by a paste event (generate by keyboard) > in last the clickTime. If that happens the paste would occur in the wrong place > for windows and mac. > > Anyhow, I can make the small changes above. Those are good points, but edge cases I did not try back when I wrote the code. Thank you for making the changes needed. > The one thing I would like to ask > you is about the behaviour. Your code the paste text is inserted at the mouse > location without changing the selection. > as far as I tested only gedit has that behaviour (and likely all other gtk > based applications). > Firefox textarea and contentEditable don't do that (they move the selection to > the offset where the text is being inserted). > Chrome is like Firefox. > > Personally I find the gedit behaviour bad, what do you think ? I asked others in my team as well. They agree that we should follow the Firefox behavior. So, please do that. (I do have some bias towards following the gedit behavior - since that's what I did in the patch - but, hey, the majority makes the rules! So I don't mind. ;) ) Looking forward for the fix to land. Thank you very much! pushed http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=640b52a43a1625555b4ca58358698c1db1db7e97 note that I changed the code a bit in order to get all the cases (firefox with privilege and chrome). please review the changes. thank you fixed (In reply to comment #11) > pushed > http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=640b52a43a1625555b4ca58358698c1db1db7e97 > > note that I changed the code a bit in order to get all the cases (firefox with > privilege and chrome). please review the changes. thank you Thank you very much! I didn't test yet - I will test it once I get to push the updated code into a local branch of mine. Just a quick problem: new Date().getTime() is not the same format as event.timeStamp. So the time diff there is broken. Firefox has a bug about event.timeStamp being wrong. See: https://bugzilla.mozilla.org/show_bug.cgi?id=77992 This is why my initial patch did use new Date().getTime() for the middle button as the lastMouseTime. (In reply to comment #13) > This is why my initial patch did use new Date().getTime() for the middle button > as the lastMouseTime. I preserved that part of your code (I hope). Did I break anything ? (In reply to comment #14) > (In reply to comment #13) > > > This is why my initial patch did use new Date().getTime() for the middle button > > as the lastMouseTime. > > I preserved that part of your code (I hope). Did I break anything ? Ah, I didn't see you included my commit as well! Yes, you kept that part. Thanks! In Firefox with chrome privileges Orion tries document.execCommand("paste") during the handling of the middle-click event. This works and we end up with the clipboard text, not the primary selection. The fix was trivial. I pushed it:
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=d8a6dc01d9c561d6eb99f03b64c8c78ce785c59d
I hope this is fine. Thank you!
>
> I hope this is fine. Thank you!
looks good to me, thanks!
|