This bug is a spin-off of BlueGriffon bug 360 [1], it affects all instances of the HTML editor including Thunderbird. To reproduce in Seamonkey Composer or BlueGriffon: 1. open the attachment 2 [review]. place caret at end at the first list item, after the trailing whitespace 3. hit the backspace key EXPECTED RESULT: whitespace deleted ACTUAL RESULT: whitespace AND last "a" char deleted To reproduce in Thunderbird: 1. create a blank HTML message 2. open the attachment in Seamonkey Composer or BlueGriffon 3. select all, copy to clipboard 4. paste clipboard into HTML message 5. continue at step 2 of the Composer/BlueGriffon steps above The bug is in nsWSRunObject::PriorVisibleNode that does not deal at all with eTrailingWS. [1] http://bugzilla.bluegriffon.org/show_bug.cgi?id=360

mike could you launch a try build so Joe could test the patch ?

Builds will be coming in here: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0317db920159

Seems to work fine with the try build Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120221 Thunderbird/13.0a1 Note that normal composition in TB does not leave whitespace at the end of the list item. So minimal impact on TB html compose. I can't imagine this having an adverse affect elsewhere, but I'll run with it for awhile.

Comment on attachment 599206 [details] [diff] [review] proposed fix Review of attachment 599206 [details] [diff] [review]: ----------------------------------------------------------------- The fix looks good, although I'm curious to know whether this breaks any existing tests. The patch should also have a unit test which should be fairly easy to write. Let me know if you need help to figure out how to write the test. Thanks!

Autoland Patchset: Patches: 599206 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=7279698f9128 Try run started, revision 7279698f9128. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=7279698f9128

Try run for 7279698f9128 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7279698f9128 Results (out of 216 total builds): exception: 1 success: 163 warnings: 38 failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-7279698f9128

Sorry, I totally forgot about this bug. :( Pushed to try again: http://tbpl.mozilla.org/?tree=Try&rev=2af2b18943ba I'll land the patch if the results of the try push look good.

Try run for 2af2b18943ba is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2af2b18943ba Results (out of 218 total builds): exception: 6 success: 159 warnings: 53 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-2af2b18943ba

There's one test failure on all platforms with this patch: https://tbpl.mozilla.org/php/getParsedLog.php?id=11139229&tree=Try TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_reftests_with_caret.html | Reftest http://mochi.test:8888/tests/layout/base/tests/bug644428-1.html FAILED Daniel, do you think you can look into this?

(In reply to Ehsan Akhgari [:ehsan] from comment #10) > There's one test failure on all platforms with this patch: > > https://tbpl.mozilla.org/php/getParsedLog.php?id=11139229&tree=Try > TEST-UNEXPECTED-FAIL | > /tests/layout/base/tests/test_reftests_with_caret.html | Reftest > http://mochi.test:8888/tests/layout/base/tests/bug644428-1.html FAILED > > Daniel, do you think you can look into this? Amazing, I never received the bugmail for your comment, sigh. Yes, I'll look into that. Not right now because I have two conf calls now before my night but tomorrow morning my time.

(In reply to Ehsan Akhgari [:ehsan] from comment #10) > There's one test failure on all platforms with this patch: > > https://tbpl.mozilla.org/php/getParsedLog.php?id=11139229&tree=Try > TEST-UNEXPECTED-FAIL | > /tests/layout/base/tests/test_reftests_with_caret.html | Reftest > http://mochi.test:8888/tests/layout/base/tests/bug644428-1.html FAILED > > Daniel, do you think you can look into this? Yeah, the test is wrong. After two backspace at the end of <div>a<span>b</span>c</div> The editor will leave an empty span with the caret placed inside that span. That was made on purpose as a response to user feedback back in 2000/2001: users wanted to be able to delete all contents of an inline element, for instance a <b>, and start again typing in bold. An extra backspace will delete the empty span AND the char immediately preceding it. So I suppose the reftest has to be adapted (empty span added, selection changed). Please note the JS console complains about the lack of <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> inside both documents. Proposing a patch to fix test, seems ok seen from here, please let me know.

Hmm, I think leaving the empty span is against the spec. Aryeh?

(In reply to Ehsan Akhgari [:ehsan] from comment #13) > Hmm, I think leaving the empty span is against the spec. Aryeh? If this is against the spec, then I respectfully suggest to change the spec because the current behaviour of the editor came from user panels that expressed very clear results, consistent with the behaviour of all text editing tools like MS Word and consorts. What our editor does right now _is_ the right thing to do.

Yes, I agree that the behavior makes sense, but I think we should try to not put this information in the DOM if possible.

(In reply to Ehsan Akhgari [:ehsan] from comment #15) > Yes, I agree that the behavior makes sense, but I think we should try to not > put this information in the DOM if possible. Hmmm. That could be possible using a TypeInState but won't be that simple since TypeInState does not carry more information than stylistic ones. What about ID, classes and other attributes? What about CSS styles triggered by such attributes or even element context? Since TypeInState was already there when the current behaviour was implemented, I guess the editor team (namely Joe Francis and manager Beth Epperson) considered it as overkill compared to the preservation of an empty span. Pragmatism, really. But is it really related to our issue here? Is it related to the bug resolved here?

(In reply to Daniel Glazman from comment #16) > (In reply to Ehsan Akhgari [:ehsan] from comment #15) > > > Yes, I agree that the behavior makes sense, but I think we should try to not > > put this information in the DOM if possible. > > Hmmm. That could be possible using a TypeInState but won't be that simple > since > TypeInState does not carry more information than stylistic ones. What about > ID, > classes and other attributes? What about CSS styles triggered by such > attributes > or even element context? Since TypeInState was already there when the > current behaviour was implemented, I guess the editor team (namely Joe > Francis > and manager Beth Epperson) considered it as overkill compared to the > preservation > of an empty span. Pragmatism, really. I don't feel really strongly either way. I'd really like to know what Aryeh thinks here. > But is it really related to our issue here? Is it related to the bug > resolved here? Wait, I thought that the behavior change is caused by your patch. Is it not?

(In reply to Ehsan Akhgari [:ehsan] from comment #17) > > But is it really related to our issue here? Is it related to the bug > > resolved here? > > Wait, I thought that the behavior change is caused by your patch. Is it not? Oh no, the editor has _always_ left an empty span in that case. My patch is totally unable to trigger that behaviour, it only deals with trailing whitespaces.

Yes, the spec says to remove the tag and add any style to type-in state. Consider the following scenario for motivation: "foo<b>bar</b>baz". User puts cursor at end of "bar" and backspaces over it, then moves the cursor somewhere else, then moves it back between "foo" and "baz" and types "quz". Expected: "quz" is not bold; if you move the cursor away and then back, the editor doesn't remember that that position is bold. This means that as soon as the cursor moves away, we want to forget about the bold. The best way to do this is type-in state, and that's what both Gecko and WebKit do AFAICT. If we left the empty <b></b> in this case, then it would stay around forever and do nothing -- it's just clutter. At a glance, it looks like Gecko behaves differently for "foo<span id=x>bar</span>baz". In this case it can't put the tag in type-in state, so in fact it does leave the empty tag. This means that if you move the cursor away, the empty tag stays there forever. This isn't good. It's also not enough to just remove the tag as soon as the cursor position changes, because something might read the innerHTML (e.g., to save the message) when the empty tag is present, and again it would remain forever. What are the use-cases here? If we want to preserve the span in this case, I think the way to do it is to expand type-in state to allow for tags. Specifically, if you delete all the contents of a tag, then remove the tag from the DOM but keep a reference. If text is inserted again when the cursor position hasn't changed, reinsert the same tag before inserting the text. But I'd want to know what the use-cases are. What's a real-world case where people are relying on/desire Gecko's current behavior (which WebKit does not agree with)? Currently the editing spec is intended to only support WYSIWYG styles, not really anything else. If it's important to some users that it better support classes and id's and such, I'm very interested in hearing the details.

(In reply to Aryeh Gregor from comment #19) > This isn't good. But it works, and it's rather complex case. Getting rid of empty elements in an editor is a VERY complex decision. We extensively discussed it when we designed the editor. The problem is about two empty and consecutive spans. First one is empty on purpose, second one is empty because the user pressed backspace. Implementing a behaviour that removes the latter one is easy; implementing a behaviour that removes the latter one w/o removing the former one is almost undecidable. And since editors Should Never Do Unpredictable Things On User's Original Markup... > But I'd want to know what the use-cases are. What's a real-world case where > people are relying on/desire Gecko's current behavior (which WebKit does not > agree with)? Currently the editing spec is intended to only support WYSIWYG > styles, not really anything else. If it's important to some users that it > better support classes and id's and such, I'm very interested in hearing the > details. The "WYSIWYG styles" you mention can come from very complex contextual selectors, attribute selectors, etc. But structure and context can and even sometimes must be ignored by the user who only expects a "Word-like" behaviour... For the time being, and given getting rid of empty elements here implies major revamping of that code in our editor, I don't support a change. Furthermore, there is certainly code in Seamonkey, BlueGriffon, even Thunderbird and possibly inline web editors relying on the current behaviour because we have always done that. Be very cautious if you change anything here, please.

(In reply to Daniel Glazman from comment #20) > But it works, and it's rather complex case. Getting rid of empty elements in > an editor is a VERY complex decision. We extensively discussed it when we > designed > the editor. The problem is about two empty and consecutive spans. First one > is > empty on purpose, second one is empty because the user pressed backspace. > Implementing a behaviour that removes the latter one is easy; implementing a > behaviour that removes the latter one w/o removing the former one is almost > undecidable. And since editors Should Never Do Unpredictable Things On User's > Original Markup... I don't follow. What exact input markup are you talking about, and what user actions, and what results? If you have foo<span class=a></span><span class=b>bar</span> and the user backspaces over "bar" then types "baz", it should be foo<span class=a></span><span class=b>baz</span> If they backspace over "bar" and then move the cursor away, it should be just foo<span class=a></span> What's the problem here? For the standard web editing use case -- blog posts/webmail/etc. that are written exclusively using contenteditable -- we definitely don't want to leave empty tags lying around under any conditions I can think of.

(In reply to Daniel Glazman from comment #18) > (In reply to Ehsan Akhgari [:ehsan] from comment #17) > > > > But is it really related to our issue here? Is it related to the bug > > > resolved here? > > > > Wait, I thought that the behavior change is caused by your patch. Is it not? > > Oh no, the editor has _always_ left an empty span in that case. My patch is > totally unable to trigger that behaviour, it only deals with trailing > whitespaces. OK, so the thing I don't understand is that what caused your patch to make these tests fail?

(In reply to Aryeh Gregor from comment #19) > What are the use-cases here? If we want to preserve the span in this case, > I think the way to do it is to expand type-in state to allow for tags. > Specifically, if you delete all the contents of a tag, then remove the tag > from the DOM but keep a reference. If text is inserted again when the > cursor position hasn't changed, reinsert the same tag before inserting the > text. It occurs to me that this would also eliminate the ugliness of bug 590640 patch 5. DeleteSelection could just strip wrappers in all cases, and then in the case of insertText or such, the same wrappers would naturally be reinserted right before the text is inserted. So we wouldn't need all the aStripWrappers stuff.

Did anyone look at the fix for the test ?