Community
Participate
Working Groups
STR: 1. open a page with rich-text formatting, select and copy. 2. open orion and paste using the Edit > Paste option. Expected result: text is pasted without rich formatting. Actual result: text is pasted with rich formatting and when the user tries to interact with Orion, the pasted text disappears. (cross-posted from https://bugzilla.mozilla.org/show_bug.cgi?id=694767 )
This is a side effect of the fix for Bug 355230 This is a happening because we removed the paste event handler from the clientDiv. (in the fix for Bug 355230 we used the key down handler to assign focus to textArea when Ctrl+V is pressed). We can put back the paste handler to clientDiv, but it will run into the problem described in: https://bugzilla.mozilla.org/show_bug.cgi?id=689590 where I say: - Switching the focus to the textArea during clipboard events is problematic (puts the textArea in a bad state and the paste gets the wrong content - possibly a bug in Firefox 4 as this used to work on Firefox 3). (see also comment 1 and 3 - unfortunately I was never able to isolate the problem in simple test case). I'm afraid we won't be able to fix this problem without https://bugzilla.mozilla.org/show_bug.cgi?id=689590
For your information, if I add back the paste handler to the clientDiv I get the following error in the console when using menu->edit->paste: Error: An error occurred executing the cmd_paste command: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIController.doCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96" data: no] Source File: chrome://global/content/globalOverlay.js Line: 100 Mihai, does that mean anything to you ? To reproduce this problem, go to textView#_hookEvents and find: if (!isFirefox) { handlers.push({target: clientDiv, type: "paste", handler: function(e) { return self._handlePaste(e);}}); } Change it to: handlers.push({target: clientDiv, type: "paste", handler: function(e) { return self._handlePaste(e);}});
(In reply to comment #1) > This is a side effect of the fix for Bug 355230 > > This is a happening because we removed the paste event handler from the > clientDiv. > (in the fix for Bug 355230 we used the key down handler to assign focus to > textArea when Ctrl+V is pressed). Yep, I noticed that. > We can put back the paste handler to clientDiv, but it will run into the > problem described in: > https://bugzilla.mozilla.org/show_bug.cgi?id=689590 > where I say: > - Switching the focus to the textArea during clipboard events is problematic > (puts the textArea in a bad state and the paste gets the wrong content - > possibly a bug in Firefox 4 as this used to work on Firefox 3). > (see also comment 1 and 3 - unfortunately I was never able to isolate the > problem in simple test case). True. I am not sure if it's really a bug, I'd have to know what changed in the Firefox code to break this case. I could assume it's a protection against stealing the clipboard content - a script could take focus away and put the result where you would not expect (as a user). Anyhow, that would need more testing before I can really tell why it no longer works. > I'm afraid we won't be able to fix this problem without > https://bugzilla.mozilla.org/show_bug.cgi?id=689590 I do have more hope. :) (In reply to comment #2) > For your information, if I add back the paste handler to the clientDiv I get > the following error in the console when using menu->edit->paste: > Error: An error occurred executing the cmd_paste command: [Exception... > "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) > [nsIController.doCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" > location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand > :: line 96" data: no] > Source File: chrome://global/content/globalOverlay.js > Line: 100 > > Mihai, does that mean anything to you ? That's code that executes commands, but I can't point you to the cmd_paste implementation - I'd have to search for it myself. (that's the command name) > To reproduce this problem, go to textView#_hookEvents and find: > if (!isFirefox) { > handlers.push({target: clientDiv, type: "paste", handler: function(e) { > return self._handlePaste(e);}}); > } > > Change it to: > handlers.push({target: clientDiv, type: "paste", handler: function(e) { return > self._handlePaste(e);}}); Yep, I've been already playing with this. I do have some comments that might help you find the working solution: - In getClipboardText() the document.execCommand("paste") can't work. document is view._frameDocument which is different from view._textarea.ownerDocument (see view._parentDocument). The code is trying to paste from one document into a different document. This should work with the clipboard access granted, but it fails (tested). The failure is the same error as the one you just pasted above. - I played with doing textarea.ownerDocument.execCommand("paste"), but that fails (same error). It works only if you set textarea.ownerDocument.designMode=on, and that's not acceptable (since we shouldn't turn designMode on for the parent document :) ). - We can't allow the native paste event handler to happen in clientDiv because it breaks our rendering. The solution I have in mind: (I didn't get to test this theory!) - add a "clipboardDiv" hidden somewhere in frameDocument (similar to the textArea). - add handlePaste to clientDiv, as you suggested. - make getClipboardText focus clipboardDiv then, optionally do frameDocument.execCommand("paste"). just like now. - now one can read clipboardDiv.textContent. I have a hunch: it's not the focusing of the textarea that breaks the code now. It's the problem of the element being in a different owning document, which has no designMode. Maybe I am wrong, but what worked in Firefox 3 was "undocumented" behavior, something that I am not surprised to have changed.
(In reply to comment #3) > The solution I have in mind: > (I didn't get to test this theory!) > - add a "clipboardDiv" hidden somewhere in frameDocument (similar to the > textArea). > - add handlePaste to clientDiv, as you suggested. > - make getClipboardText focus clipboardDiv then, optionally do > frameDocument.execCommand("paste"). just like now. > - now one can read clipboardDiv.textContent. > I have a hunch: it's not the focusing of the textarea that breaks the code now. > It's the problem of the element being in a different owning document, which has > no designMode. > Maybe I am wrong, but what worked in Firefox 3 was "undocumented" behavior, > something that I am not surprised to have changed. This is similar to code we had before fixing bug#355230. What is hard with this solution is how to convert the pasted HTML into good text content. I do not think clipboardDiv.textContent will handle line breaks, tabs, etc.
Proposed fix: https://github.com/mihaisucan/orion.client/tree/bug-361180-2 Explanation: - yesterday I didn't notice that for Firefox the textarea is inside frameDocument (it's only for iPads inside parentDocument). - discussed with other colleagues and ... as expected execCommand("paste") was undocumented behavior that was subject to change (wrt. textarea.focus()). - the execCommand("paste") call always fails when one focuses a textarea, irrespective if it's during paste, keydown or some other event. - execCommand("paste") needs to have a contentEditable element, for it to work. - then the focus() change can work even during the paste event. Similar to what I expected yesterday, the fix I am proposing is: - handlePaste for clientDiv. - change from <textarea> to <div contentEditable> - now the focus() change can work (won't fail), and execCommand("paste") also works when the page is granted clipboard access. In either case, paste works (with or without the permission) in the newly focused element. - use newDiv.textContent to read the pasted output. This is ugly because new lines come from the markup source code. To get pretty output I added a method which uses the getSelection().toString() hack. This gives us pretty text/plain conversion of the rich-text the user pastes. Please let me know if further changes are needed before this patch can land. Thank you!
(In reply to comment #4) > (In reply to comment #3) > > The solution I have in mind: > > (I didn't get to test this theory!) > > - add a "clipboardDiv" hidden somewhere in frameDocument (similar to the > > textArea). > > - add handlePaste to clientDiv, as you suggested. > > - make getClipboardText focus clipboardDiv then, optionally do > > frameDocument.execCommand("paste"). just like now. > > - now one can read clipboardDiv.textContent. > > I have a hunch: it's not the focusing of the textarea that breaks the code now. > > It's the problem of the element being in a different owning document, which has > > no designMode. > > Maybe I am wrong, but what worked in Firefox 3 was "undocumented" behavior, > > something that I am not surprised to have changed. > > This is similar to code we had before fixing bug#355230. What is hard with this > solution is how to convert the pasted HTML into good text content. I do not > think clipboardDiv.textContent will handle line breaks, tabs, etc. Interesting. I just tested bug 355230. It works with the above proposed solution, but that prompted me to find a problem with plain/text copied from other apps: the whitespace is lost. Heh. If we use a PRE we get back to the problem you mentioned: PREs can't hold any type of element and we see breakage. Going to try something...
Just pushed a fix to the branch: https://github.com/mihaisucan/orion.client/tree/bug-361180-2 Now we avoid the PRE breakage and we also keep the prettified output (browser conversion of richtext to plain/text). Also, text coming from plain text editors has its whitespace unchanged. Ctrl-V and Paste from the menu both work. (hopefully I nailed all the problems!) Fix explanation: let the browser paste in a PRE, let it overflow, let it break out of the PRE. We copy the text from its parent element. Looking forward for comments. Thank you!
This is great, I tested the main cases and it seems that everything is working. I'm happy you got this to work, but I still think we should not give up on https://bugzilla.mozilla.org/show_bug.cgi?id=689590 I have a few questions about the code: -This line textArea.innerHTML = "<pre contenteditable=''></pre>"; do we really need that contenteditable='' to fix the problem with the whitespaces ? Is that doing anything ? -In hookEvents, we are hooking "paste" on the clientDiv, and on _textArea (only for FF). I suspect that we don't need the listener in the _textArea as it should not have the focus (except when the magic is happening). -In handleKeyDown, can we remove all this code: /* * Bug in Firefox. The paste operation on Firefox is done by switching * focus into a textarea, let the user agent paste the text into the * textarea and retrieve the text pasted from it. This works as expected * in Firefox 3.x, but fails in Firefox 4 and greater. The fix is to * switch focus to the textarea during the key down event that triggers * the paste operation. */ if (isFirefox) { var ctrlKey = isMac ? e.metaKey : e.ctrlKey; if (ctrlKey && e.keyCode === 86 /*Ctrl+v*/) { this._textArea.value = ""; this._textArea.focus(); } } -At some point, we should probably rename _textArea to _clipboardDiv or something.
(In reply to comment #8) > This is great, I tested the main cases and it seems that everything is working. > I'm happy you got this to work, but I still think we should not give up on > https://bugzilla.mozilla.org/show_bug.cgi?id=689590 I am glad it works. :) Yes, of course, we don't have to give up the better solution (Mozilla bug 689590). > I have a few questions about the code: > > -This line > textArea.innerHTML = "<pre contenteditable=''></pre>"; > do we really need that contenteditable='' to fix the problem with the > whitespaces ? Is that doing anything ? Yes, this is needed. The PRE is needed such that when we paste the browser keeps the whitespace. The PRE contenteditable attribute is needed to allow us to focus the PRE and do the actual paste there. Otherwise, we can't paste there. > -In hookEvents, we are hooking "paste" on the clientDiv, and on _textArea (only > for FF). > I suspect that we don't need the listener in the _textArea as it should not > have the focus (except when the magic is happening). > > -In handleKeyDown, can we remove all this code: > /* > * Bug in Firefox. The paste operation on Firefox is done by switching > * focus into a textarea, let the user agent paste the text into the > * textarea and retrieve the text pasted from it. This works as expected > * in Firefox 3.x, but fails in Firefox 4 and greater. The fix is to > * switch focus to the textarea during the key down event that triggers > * the paste operation. > */ > if (isFirefox) { > var ctrlKey = isMac ? e.metaKey : e.ctrlKey; > if (ctrlKey && e.keyCode === 86 /*Ctrl+v*/) { > this._textArea.value = ""; > this._textArea.focus(); > } > } The textarea.focus() in keydown trick is no longer needed. (I hope so) > -At some point, we should probably rename _textArea to _clipboardDiv or > something. Sure, I actually recommend the rename. Do you want me to make the changes? Thank you for testing!
what about this question (from my previous comment) ? >> -In hookEvents, we are hooking "paste" on the clientDiv, and on _textArea (only >> for FF). >> I suspect that we don't need the listener in the _textArea as it should not >> have the focus (except when the magic is happening). >Do you want me to make the changes? Sure, once you are done you can: -- push the code to git.eclipse.org (get to use your commit rights) -- assign the bug to you and close as fixed I will pull the changes, review everything again and test again. Thank you
Pushed: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=a815e2a47ed6eeb97b60ee6a4ad127aeefef8ca2 http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=2c0de2a5227399e1d9fb6b242a255fec5216bcf5 http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=aaca41f1749169e21e67ebf080890f60f95587d4 I hope I didn't break anything! Thank you for your approval to land this fix!
