Load an image to the clipboard. Type a message in a font that is not the default font. Paste the image into the composition. Keep typing. Font changes to default display font. This was originally reported in bug 756984, comment #14. Over in the other bug there is also a video showing the problem. Further observations: An image can be pasted into a <div contenteditable> without losing the type-in font. In the midas demo (http://www-archive.mozilla.org/editor/midasdemo/) the type-in style DOES get lost. So it's not clear yet, where the problem lies. Therefore I'll log the bug against Thunderbird for the time being.

As stated in the description, this can be reproduced on the Midas demo: http://www-archive.mozilla.org/editor/midasdemo/ Set the font to Arial, type something, paste a picture from the clipboard, keep typing. Font is wrong. I managed to reproduce it in a simple <div contenteditable>. Follow the description given. Note: You must click into the first line so get the correct font context. If you click to the right, the font is wrong due to bug 756984.

Comment on attachment 8575532 [details] Very simple test to reproduce the problem in a <div contenteditable> Not sure what kind of feedback you're requesting for. The test case looks fine.

Well, a rough idea of the possibility to fix this would be good, somewhere between easy and impossible. And perhaps a way forward, if any, you know, the mentoring thing ;-) It looks like a new image element is inserted, and after that a new text element. When this happens, the properties of the text element that precede the image are forgotten. Over in bug 756984, comment #25 you said: > Note that the type-in state code is mostly used to handle sequences such as > calling document.execCommand("bold") and starting to type. It doesn't know > how to handle anything more complicated (for example remembering the styles > to be used for an arbitrary element at an arbitrary DOM position.) Given this comment, I wouldn't expect an easy fix.

I don't think it really makes sense to try to fix this before bug 756984. I have no idea what the fix should be before debugging which I don't have time to do right now. If you want to debug this, you should probably start with looking at what happens to nsHTMLEditor's mTypeInState member as you're reproducing, to determine if it is in fact the TypeInState information that gets forgotten or if it's something else.

Is this really a core bug? What is the desired behaviour? Say the user selects a font, types some text, then pastes and image from the clipboard (I do this all the time). Is it fair to expect that the typing should continue in the previously chosen style, just like that? Chrome and IE can't insert images in <div contenteditable>, so is it reasonable to expect that FF will do it "out of the box" including continuing the font afterwards? As Ehsan said in bug 756984 comment #25: > I _think_ to fix that part you need to get Thunderbird tell Gecko about > how to format the new paragraph. I'm asking because I need to decide on whether to continue that as a core bug or whether to look for a TB specific fix, that would be: listen to the image insertion, then set the font again.

This is a core bug. The expected behavior should be the font staying the same. Please don't focus on this bug, as the fix to bug 756984 may fix this one too, since they're both the same underlying issue.

According to my testing, the fix in bug 756984 doesn't fix this one. After inserting the image, the selection is NOT set to the text preceding the image (how could it be?) and thus the font is lost. I don't think there is the same underlying issue: In bug 756984 the fix was to "reinterpret" the click to mean the text to the left of the click (skipping brFrame). IMHO it is not possible here to select the text preceding the image since the image is between that text and the current insertion point.

We still don't have a complete fix for bug 756984. The stuff that you're working on is just the tip of the iceberg. Can you please just trust me on this and wait for bug 756984 to be finished before worrying about this? Thanks!

Jorg, with bug 756984 being fixed now, have you retested this? Is this still an issue?

Thanks for remembering this bug ;-) Yes, this is still an issue. In bug 756984 we added code to ignore brFrames at the end of the line. This is a different problem. After the image is pasted, we're no longer in the text frame we were in before the insertion. So the type-in style gets lost. As stated in comment #5, I'm not 100% convinced that this this is a core bug. Yes, it would be nice to maintain the type-in style, but hey, other browsers can't paste images into <div contenteditable>, so FF is already ahead of the game. There used to be a hotly contest problem, where the type-in style got lost upon injection (bug 590640). But here we don't have an injection. We're just typing, pasting, typing some more. Anyway, if you have more hints than what you stated in comment #4, please let me know. One other thing. Over in bug 756984 comment #64 you said: > TypeInState can only remember the properties set while the selection has not changed, so it > is never the right fix for bugs that occur when you move the selection around in the document. Since in this case, the selection has changed after the image pasting (or has it not?), according to your words, we can't even expect to maintain the type-in state. Please give me further instructions and I'll look into it.

Where does the selection go after we paste? And what does the DOM look like then?

Sorry, I'm not a JS programmer at all. In bug 756984 I learned how to dump out the selection. Over in that bug you also said (quote): "The simplest way to create such a test case is to set it up to print the selection after 10 seconds or so." I don't know how to wait 10 seconds and I also don't know how to look at the DOM. Please point me to an example. I know my way around a bit in the debugger, so I can tell you what the HTML looks like after the event. <div contenteditable=""> <font face="Arial">arial line</font> <img alt="" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAB0AAAAOCAIAAABcs4BtAAAAGklEQVQ4jWOwzltKC8Qwau6ouaPmjppLZ3MBQHoR07iOaJYAAAAASUVORK5CYII="> font lost </div>

In a web page? setTimeout(function() { dump(window.getSelection()); }, 10000);

Yep, in the web page. Thanks. What about the (quote): "And what does the DOM look like then?" How do I look at it? Anyway, the test shows that the selection ends up in the DIV. Not a surprise.

(In order to look at the DOM you can use the Inspector panel of the developer tools.) Hmm, so what's happening here is that the pasted image ends up outside of the <font> inline element even though the selection before pasting the image is inside it. That's weird, I don't know off hand why that would happen. You should see what's happening under the debugger. The place to start is nsHTMLEditor::InsertFromTransferable. I think we'd end up calling InsertObject from there. You should keep going through this code until you get to the point where we inject the nodes into the DOM, and then see if the selection is misplaced at that point, or if we end up injecting the element somewhere other than where the selection is...

Finally got around to looking into it. Thanks for the tips, they were 100% correct. Call stack: nsHTMLEditor::DoInsertHTMLWithContext() Line 340 C++ nsHTMLEditor::InsertObject() Line 1096 C++ nsHTMLEditor::InsertFromTransferable() Line 1129 C++ nsHTMLEditor::Paste() Line 1400 C++ Now take a look at lovely line 340: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#340 if (aClearStyle) { // pasting does not inherit local inline styles Sure enough the function is called with aClearStyle==true. If I change it to false in the debugger, the style is maintained after the insert. The caller calls DoInsertHTMLWithContext with 9 arguments only, therefore the 10th argument defaults to "true": https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditor.h#742 The solution is a one line change. Let's see whether it does other damage: https://treeherder.mozilla.org/#/jobs?repo=try&revision=170d15b92428 Ehsan, can you please tell me how I can write a test for this. The test needs to paste an image from the clipboard. What functions do I use to simulate this?

Clean try run.

(In reply to Jorg K from comment #16) > Ehsan, can you please tell me how I can write a test for this. The test > needs to paste an image from the clipboard. What functions do I use to > simulate this? See test_bug599322.html for example, which copies an image by simulating Ctrl+C and pastes it by simulating Ctrl+V. You also want to use SimpleTest.waitForClipboard similar to that test in order to avoid platform specific clipboard asynchronicity issues.

Hmm, pasting an image and pasting an image can be two different things. What you suggest is selecting and image somewhere in an HTML document and then copying and pasting. Sadly that still loses the font when continuing to type after the insertion even with my fix. When an image from an HTLM document is copied, it goes through this call: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1185 I confirmed in the debugger that adding the "false" parameter here also maintains the style. What my patch fixes is pasting an image that got placed onto the clipboard by a different program, a graphic editor (GIMP) or a screen capture tool. This goes through this call https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1091 where a kJPEGImageMime (etc.) image is inserted. So therefore two questions: 1) Do you want to fix the case where the image is copied from the HTML document, the case that still doesn't work but can be tested easily? 2) How can I write a test for the case I did manage to fix? The picture comes from some other program.

OK, I think I can answer 2). I use this to copy from: <img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAB0AAAAOCAIAAABcs4BtAAAAGklEQVQ4jWOwzltKC8Qwau6ouaPmjppLZ3MBQHoR07iOaJYAAAAASUVORK5CYII="> (see comment #12). As for 1): We might want do fix it as well for consistency, what do you think?

Re 1): Or maybe not, since at https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1185 all sorts of things can get pasted, not just images, and the original author may have wanted to clear inline styles.

Sorry, this bug is getting longer than desired. Here another summary: There are two cases of pasting an image: Option 1) Copy an image from somewhere in the document. InsertFromTransferable calling DoInsertHTMLWithContext: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1185 Now addressed with this new patch with a test. New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d0daab230f7 Shows all green (apart from some failures elsewhere). Further manual testing reveals: The new fix to option 1) is not a good idea. Pasting the image into the <font face="Arial"><b>12345</b></font>, then copying it again from there, than pasting it again, will also paste the <font face="Arial"><b> information, so when continuing to type, the bold is now even bolder. If you repeat, it gets even bolder, and so on. IMHO there is no point fixing option 1. Do you agree? I'm a little surprised that even with that tweak I got a clean try run. Option 2) Copy an image from the clipboard what has been placed there by another program (image editor, screen capture). InsertObject calling DoInsertHTMLWithContext: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1091 Was already fixed in the original patch, but is missing a test. Sadly what I said in comment #21 is NOT correct: <img src="data:image/png;base64,iVBOANS[...stuff deleted...]JYAII="> This does NOT produce an image that gets pasted via option 2). The question remains: How can I test the fix to option 2), so how do I place an image onto the clipboard that will get pasted via InsertObject and line 1091 (kJPEGImageMime/kJPGImageMime/kPNGImageMime/kGIFImageMime)?

test_bug490879.xul uses cmd_copyImageContents to copy the image itself to the clipboard.

Comment on attachment 8601395 [details] [diff] [review] Two line change for discussion, also with test (sorry about the extra empty line in mochitest.ini, will remove with final patch) You should not change the paste code path here. That is wrong for the reasons you have already discovered.

Can you please lend me a hand with the test. I did a copy/paste job, but it doesn't work. This is the error: JavaScript error: chrome://mochitests/content/chrome/editor/libeditor/tests/test _bug1140617.html, line 45: NS_ERROR_FAILURE: Component returned failure code: 0x 80004005 (NS_ERROR_FAILURE) [nsIController.isCommandEnabled] What am I doing wrong? Also, selecting the source image like this: var src = document.getElementById("src"); src.focus(); synthesizeMouseAtCenter(src, {}); synthesizeKey("VK_LEFT", { shiftKey: true }); may be to clumsy. Surely it could be selected directly via its ID (assuming that I move the ID from the containing div to the image itself).

(In reply to Jorg K from comment #26) > Created attachment 8601515 [details] [diff] [review] > Back to the one line change, with a non-working test. > > Can you please lend me a hand with the test. You need to copy this code: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/test_bug490879.xul#48>. This is essentially simulating right clicking on the image and selecting "Copy Image". Note that I'm assuming that the original bug reproduces if you copied an image like that manually.

I copied the whole function into my test, still no luck, same error: JavaScript error: chrome://mochitests/content/chrome/editor/libeditor/tests/test _bug1140617.html, line 76: NS_ERROR_FAILURE: Component returned failure code: 0x 80004005 (NS_ERROR_FAILURE) [nsIController.isCommandEnabled] Also, what is "Copy Image"? That's not an option on the right-click menu on Windows, so I can't try that manually. There is only "Copy" and that's the same as Control+C which is the code path we don't want to modify.

(In reply to Jorg K from comment #29) > Created attachment 8601619 [details] [diff] [review] > Still no working test > > I copied the whole function into my test, still no luck, same error: > JavaScript error: > chrome://mochitests/content/chrome/editor/libeditor/tests/test > _bug1140617.html, line 76: NS_ERROR_FAILURE: Component returned failure > code: 0x > 80004005 (NS_ERROR_FAILURE) [nsIController.isCommandEnabled] You need to make this a mochitest-chrome test, similar to the test I suggested to use. :-) > Also, what is "Copy Image"? That's not an option on the right-click menu on > Windows, so I can't try that manually. There is only "Copy" and that's the > same as Control+C which is the code path we don't want to modify. Try right clicking on an image, for example on http://imgur.com/.

Comment on attachment 8601619 [details] [diff] [review] Still no working test Can you please ask for review once the test works on the try server? There is no point in providing feedback in every iteration. (I'm *very* busy these days, so I appreciate if you gave me these requests once the work is finally done.) Thanks!

The problem is that I don't have a test. I need your help to produce one. The work can't do done without further input from someone who knows. I didn't ask for review, I accidentally asked for feedback instead of "needinfo". Let's clarify a few things: I tried to copy an image from a <div contenteditable>, in this case you only see "Copy", not "Copy Image". Once I changed that so "Copy Image" was available, I managed to use "Copy Image" but sadly, the pasted images goes through the code path that we won't fix as agreed. The question remains: How do I place an image onto the clipboard in the way that external applications (image editor, screen snapshot) do so that InsertObject is exercised. If you look at the patch for 10 seconds longer, you will see that the test I tried to work on *is* in fact a mochitest-chrome test, since it's registered in chrome.ini. It does however live in a html file and not a xul file. I don't know why some tests are in xul files others in html files and why some are chrome and some are simple. Anyway, this one is chrome/html. We can leave this until you find ten minutes to tell me what I need to do.

|top.document.commandDispatcher| will probably return the wrong thing in an HTML document. Please *copy* the test test_bug490879.xul and modify it accordingly. But if the image pasted this way doesn't exercise the code path you are interested in, that would not help anyway. I don't know what the contents of the clipboard should be for this specific code path to be taken. Based on everything that I know off the top of my head, <https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLDataTransfer.cpp#1128> should be hit when the contents of the clipboard is a mime-type matching one of the ones we're checking in the if condition above. If your test is not hitting it, or if that is not hit when testing manually, you need to look at the code under the debugger and figure out what needs to change in the test. It is not something that I can tell you by spending just 10 minutes -- as I would need to also debug to figure out what goes wrong. Note that bug 490879 also was hitting this same code path, which is why I keep suggesting copy exactly what that test does. I'd be *very* surprised if that test doesn't hit that code path. You might want to double check that.

The test fails when started with: mach mochitest editor/libeditor/tests/test_bug490879.xul It succeeds when started with mach mochitest-chrome editor/libeditor/tests/test_bug490879.xul (You said in bug 756984 comment #101 that the system can figure it out itself, well obviously not) I can confirm that this test exercises the code path we are interested in. I will therefore start with a copy of this test to prepare a test for this bug and contact you once I'm done - or hit another problem ;-)

I'm trying to adapt test_bug490879.xul to this bug here and I ran into problems. As I said, I'm a C++ programmer, not a JS or XUL programmer. The difference is that test_bug490879.xul uses an iframe, http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/test_bug490879.xul#18 (which has a contentDocument on which to execCommand), I'm trying to use a simple <div contenteditable>. Can you please tell me what I'm doing wrong. Here are the errors: JavaScript error: chrome://mochitests/content/chrome/editor/libeditor/tests/test_bug1140617.xul, line 63: TypeError: document.execCommand is not a function JavaScript error: chrome://mochitests/content/chrome/editor/libeditor/tests/test_bug1140617.xul, line 41: ReferenceError: synthesizeMouseAtCenter is not defined So I don't know how to call execCommand. I tried various variations, but none worked. The next problem is that synthesizeMouseAtCenter and synthesizeKey are not defined in this context. Maybe I don't need the former, but I need the latter to simulate typing after the paste. Or is there something else to use in XUL tests? Note that the function notReady is not ready. Surely I don't need the ctrl+C, since the pasting happened elsewhere, but I do need to type a letter.

You need to do this first, so that the document object would expose document.execCommand doc.designMode = "on";

Thanks. I put: document.designMode = "on"; document.execCommand("paste", false, null); JavaScript error: chrome://mochitests/content/chrome/editor/libeditor/tests/test_bug1140617.xul, line 64: TypeError: document.execCommand is not a function

(In reply to Jorg K from comment #35) > Created attachment 8602048 [details] > Need help with this > > I'm trying to adapt test_bug490879.xul to this bug here and I ran into > problems. As I said, I'm a C++ programmer, not a JS or XUL programmer. > > The difference is that test_bug490879.xul uses an iframe, > http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/ > test_bug490879.xul#18 > (which has a contentDocument on which to execCommand), > I'm trying to use a simple <div contenteditable>. > > Can you please tell me what I'm doing wrong. > > Here are the errors: > JavaScript error: > chrome://mochitests/content/chrome/editor/libeditor/tests/test_bug1140617. > xul, line 63: TypeError: document.execCommand is not a function The execCommand API only exists in HTML documents, not in XUL documents. To repeat myself again, you really need to copy what that test does already: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/test_bug490879.xul#36>. Note that the contents of the iframe is an HTML document, which is why the pasteInto function grabs the contentDocument of the iframe and calls execCommand on it. > JavaScript error: > chrome://mochitests/content/chrome/editor/libeditor/tests/test_bug1140617. > xul, line 41: ReferenceError: synthesizeMouseAtCenter is not defined This function is defined in EventUtils.js. You need to include the script at chrome://mochikit/content/tests/SimpleTest/EventUtils.js in your test.

Man, you need to give exact instructions ;-) You could have saved yourself and me so much time. Exact instructions: Copy test_bug490879.xul, keep the iframe, edit will happen in there. Use execCommand("font"/"bold"/"insertText", ... to set up the test case just before the paste. You can insert more text after the paste in the same way. Hopefully last question: How do I get the editor in this case. The getEditor() function copied from other tests ain't working. I also tried to use doc.queryCommandValue("fontname") without trying to get to the editor, but that returns "serif" instead of "Arial".

My mistake, it's doc.execCommand("fontname", false, "Arial"); not doc.execCommand("font", false, "Arial"); Will submit patch and try run soon.

Works now. Test fails with old code, succeeds with new code. Will request review once try run shows green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1042c2b66985

Comment on attachment 8602382 [details] [diff] [review] fix and test Try run is green (apart from the usual random failures), so please review the one-line change which already has feedback+ and the test which is a complete copy of another test with six additional lines ;-)

You can't be assignee and QA contact at the same time. ;-)

Can we please still land this on Mozilla40, so Mozilla40 will have all the editor changes we made which are important for Thunderbird. This is the last one of these changes.

(In reply to Jorg K from comment #45) > Can we please still land this on Mozilla40, so Mozilla40 will have all the > editor changes we made which are important for Thunderbird. This is the last > one of these changes. What's the point? I thought that Thunderbird can now take Gecko changes. I don't think we want to backport this to Aurora for Firefox.

The point is that Thunderbird has a special branch for TB38, not for 39 or 40. So the people on the beta channel won't get it, until TB41. Since all the other core improvements went into Mozilla40, it would be nice to have it there, too. It's a one line change in Gecko and really low risk, since not many people will paste images in FF. I asked for the review of a one line change (which already had feedback+) and a test, which was a copy of an existing test with six additional lines, on May 6th. Just because it missed the cut-off date by one day (when was it?) doesn't really make it a backport.

(In reply to Jorg K from comment #47) > The point is that Thunderbird has a special branch for TB38, not for 39 or > 40. So the people on the beta channel won't get it, until TB41. Since all > the other core improvements went into Mozilla40, it would be nice to have it > there, too. It's a one line change in Gecko and really low risk, since not > many people will paste images in FF. I don't know if it's that low risk, since this area of the code is essentially completely untested. But I do agree that it's not a super extensive change... (Also, FWIW, the riskiness of a patch doesn't really depend on how many lines of code it changes...) > I asked for the review of a one line change (which already had feedback+) > and a test, which was a copy of an existing test with six additional lines, > on May 6th. > > Just because it missed the cut-off date by one day (when was it?) doesn't > really make it a backport. Well, this is not exact science. :-) But as things stand, backporting this to Aurora will mean that the change will receive 6 fewer weeks of testing, and for the wrong reasons (so that it gets included in Thunderbird beta.) Anyway, if you feel strongly about this, feel free to ask for Aurora approval on the patch, providing an actual risk analysis. It is ultimately the release drivers' call. :-)

Guess why I asked for review on May 6th even with a personalised message. To avoid wasting more time on this than the review took ;-((

Comment on attachment 8602382 [details] [diff] [review] fix and test Approval Request Comment [String/UUID change made/needed]: None [Risks and why]: A few core editor improvements have been made in bug 1100966, bug 1154791 and bug 756984. These were all landed on mozilla39 or mozilla40. This is one of the core editor improvements which belongs into this group, sadly it has missed the cut-off date by a couple of days. The change is minimal and of low risk. It would be good to have all the fixes mentioned available in mozilla40. One beneficiary would be Thunderbird from version 40, where the core editor is used a lot to write e-mail. Therefore I request to have this landed on mozilla40, currently in Aurora.

Who do I need to contact to get this approved? Every day we wait longer is a day less testing.

(In reply to Jorg K from comment #54) > Who do I need to contact to get this approved? I don't know who triages Aurora requests this cycle. You can always ping lmandel.

Lawrence: Hi, I hope I can get this approved sometime soon ;-) Thanking you in advance.

Comment on attachment 8602382 [details] [diff] [review] fix and test Simple change and has tests. Just missed the merge date. Aurora+

Jorg, is this actually fixed for you in current Firefox? I'm trying to port the test from mochitest-chrome to mochitest-plain for bug 1269209, but when I do, it starts to fail. When I try to execute the steps to reproduce in a contenteditable div, formatting is lost.

I'm not sure how I can answer this. As you can imagine, the problem came from TB. I was writing an e-mail with some font or colour applied, I took a screen shot, I pasted it into my e-mail, I hit return to continue typing, and the font/colour was lost. With the patch, I can continue to type my e-mail with the "style" I had before. Now turning to FF, we can visit www-archive.mozilla.org/editor/midasdemo/. The same thing happens: I choose Courier, bold, red, type, paste, continue typing and it's all good. I remember having a real hard time with that test. (I'm afraid I didn't help you.)

With current Firefox, in Midas or whatever, does the problem still occur for you, or is it solved?

Solved, as I said: Midas: I choose Courier, bold, red, I type, I paste and image, I continue typing and *it's all good* since the Courier/bold/red sticks after the paste. Try it yourself, it takes one minute.

Interesting. It works if I paste a screenshot, but not if I right-click on an image on a web page (say the bold button on the Midas demo) and paste that. It seems that's treated as inserting HTML, not image data. I wonder what the right thing to do is here -- HTML that's just an image should preserve the styles, but if it contains text it clearly shouldn't. Anyway, that's a separate bug.