1. Open a compose window. 2. Type a few words and hit Enter. 3. Type a few more words and hit Enter again. 4. Instead of deleting the final word of the previous paragraph, the cursor jumps to the beginning of the previous paragraph.

9:11.83 INFO: Got as far as we can go bisecting nightlies... 9:11.83 INFO: Last good revision: 02115be839cfcd0ba3b44f7fe480974496f0a86d (2016-08-22) 9:11.83 INFO: First bad revision: fca2074018806ec55ddca94109bbeafd9a993720 (2016-08-23) 9:11.83 INFO: Pushlog: https://hg.mozilla.org/comm-central/pushloghtml?fromchange=02115be839cfcd0ba3b44f7fe480974496f0a86d&tochange=fca2074018806ec55ddca94109bbeafd9a993720

Looks like your STR are incomplete. Maybe there is 3a missing. Press Ctrl-backspace. Also note that the editor is an M-C module, so this bug should be in Core::Editor. If the regression is between the 22nd and 23rd of Aug. 2016, then it was most likely caused by bug 1265800. Aryeh, can you take a look, oops, we just missed him.

Sorry, yes, I don't know how I screwed that up. Yes, you need to hit ctrl-backspace between step 3 and step 4.

OK, let's copy Makoto's test case from bug 1306488 comment #8 here: data:text/html,<div contenteditable=true style="width:200px"><p>ABC DEF<p>GHI JKL<p>MNO PQR</div>

[Tracking Requested - why for this release]: This issue (bug 1306488) occurs on outlook.com when using Firefox 51+.

As I mentioned elsewhere, I'm not planning to work on bugs that involve behavior like this unless it's demonstrated to affect real-world sites in Firefox, because I understand that these behaviors are generally overridden by real-world sites due to lack of browser interop. I might make an exception for this one if it's a regression due to other changes by me, though.

As per comment #6 and bug 1306488 comment #3 (there's even an animated GIF), this issue is demonstrated to affect a very popular real-world site Outlook.com.

I checked this out on Outlook.com. They do indeed capture the into paragraphs. The problem is not present using FF 49 (current at time of writing), however, <ctrl><backspace> does already not behave very consistently in FF 49. Sadly the problem reported and fixed in bug 1265800 can be seen in FF 49 at Outlook.com, so it is good that it was fixed. Backing that bug out is not an option to get rid of this regression. Altogether it's good to know that Thunderbird isn't the only user of paragraph composition since it's also used on Outlook.com.

Jorg, is it right that next Thunderbird uses Gecko 52? If so, I think that we should back out bug 1265800 on Gecko 51 tree and then we (I or Nakano-san) should fix this.

Looks like that it happens only when selection is collapsed at (<p>, 0). If child of <p> has selection, Ctrl+Backspace works fine.

(In reply to Makoto Kato [:m_kato] from comment #10) > Jorg, is it right that next Thunderbird uses Gecko 52? Yes, the next official release of TB 52 is an ESR and will use Gecko 52. > If so, I think that we should back out bug 1265800 on Gecko 51 tree and then > we (I or Nakano-san) should fix this. I don't think, as I said, that backing out bug 1265800 is an option as it is even more severe since it invisibly causes undesired HTML. Please try attachment 8750251 [details]. If you follow the instructions and press <enter> twice after the x, then backspace, then type, the text you type doesn't go into a paragraph. Since Outlook applies styling via CSS to the paragraph, this is quite fatal at the receiving end especially since the sender doesn't notice it. IMHO not having <backspace> fail invisibly is worse then having the much lesser used <ctrl><backspace> fail visibly. Of course it would be best to fix the regression properly.

If we are looking for a "real fix" for this soon, given that Aryeh is not available until the spring, should we get Nakano-san to look for this fix, that doesn't involve the backout?

The culprit is here: https://hg.mozilla.org/mozilla-central/rev/27c7692478d9#l1.20 Reverting this change makes <ctrl><backspace> work again. As far as I can see, what we wanted to fix in bug 1265800 is still working. The attached patch is not meant to be the fix, it's just meant to localise the problem and start the discussion ;-)

(In reply to Milan Sreckovic [:milan] from comment #13) > If we are looking for a "real fix" for this soon, given that Aryeh is not > available until the spring, should we get Nakano-san to look for this fix, > that doesn't involve the backout? This is not one I want to fix in a hurry, given the bug's age (per 1306488 comment 5,) and how regression-prone all of this code is. The first thing I'd like to see is a test landed. I'll speak with the Japan team in person about these editor bugs next week.

Comment on attachment 8809092 [details] [diff] [review] 1314790.patch Reverting this part of bug 1265800 doesn't seem to cause any problems. If my testing was right, it fixes the problem and doesn't regress bug 1265800. https://treeherder.mozilla.org/#/jobs?repo=try&revision=55ab5fd481d3129c23396e99a744356e42161569 Nakano-san, what do you think? BTW, what is your first name? Nakano? So should it be Nakano-san or Masayuki-san?

Track 51+ as regression.

Comment on attachment 8809092 [details] [diff] [review] 1314790.patch Sorry, this is risky. I need more time to check this. So, I'll try tomorrow again. FYI: Please don't declare variables at outer block except when it's necessary. Smaller scope makes other developers understand easier. > BTW, what is your first name? Nakano? So should it be Nakano-san or Masayuki-san? "Masayuki" is my first name but either is okay, and don't mind to append "-san" post-fix so seriously. If you can speak Japanese language *fluently* and you don't append it, most Japanese people think you're not polite. However, otherwise, nobody (probably) don't think so.

GetGoodSelPointForNode has bug when aAction is ePreviousWord :-<

Comment on attachment 8809092 [details] [diff] [review] 1314790.patch (In reply to Jorg K (GMT+1) from comment #16) > Reverting this part of bug 1265800 doesn't seem to cause any problems. If my > testing was right, it fixes the problem and doesn't regress bug 1265800. Okay, that looks fine to me except the block scope of |nsresult rv|. However, Makoto-san might try to fix this, we should take this patch as the last resort.

(In reply to Makoto Kato [:m_kato] from comment #19) > GetGoodSelPointForNode has bug when aAction is ePreviousWord :-< Surely it would be better to fix that bug ;-) I see that Makoto-san assigned the bug to himself.

Comment on attachment 8809092 [details] [diff] [review] 1314790.patch A correct fix is on the way.

Comment on attachment 8810253 [details] Bug 1314790 - Part 1. GetGoodSelPointForNode doesn't work with ePrevousWord action. https://reviewboard.mozilla.org/r/92622/#review92900

Comment on attachment 8810254 [details] Bug 1314790 - Part 2. Add test. https://reviewboard.mozilla.org/r/92624/#review92912 ::: editor/libeditor/tests/test_bug1314790.html:43 (Diff revision 1) > + ok(elm.childNodes[0].textContent == "pen pineapple", > + "'pen pineapple' is first elment"); > + ok(elm.childNodes[1].textContent == "apple pen", > + "'apple pen' is second elment"); Please use is(). ::: editor/libeditor/tests/test_bug1314790.html:50 (Diff revision 1) > + ok(elm.childNodes[1].textContent == "apple pen", > + "'apple pen' is second elment"); > + > + SpecialPowers.doCommand(window, "cmd_deleteWordBackward"); > + SpecialPowers.doCommand(window, "cmd_deleteWordBackward"); > + ok(elm.childNodes[0].textContent == "pen pineapple", And here. ::: editor/libeditor/tests/test_bug1314790.html:54 (Diff revision 1) > + SpecialPowers.doCommand(window, "cmd_deleteWordBackward"); > + ok(elm.childNodes[0].textContent == "pen pineapple", > + "'pen pineapple' is first elment"); > + > + SpecialPowers.doCommand(window, "cmd_deleteWordBackward"); > + ok(elm.childNodes[0].textContent == "pen pineapple", And here. ::: editor/libeditor/tests/test_bug1314790.html:58 (Diff revision 1) > + SpecialPowers.doCommand(window, "cmd_deleteWordBackward"); > + ok(elm.childNodes[0].textContent == "pen pineapple", > + "'pen pineapple' is first elment"); > + > + SpecialPowers.doCommand(window, "cmd_deleteWordBackward"); > + ok(elm.childNodes[0].textContent == "pen ", "'pen ' is first elment"); And here.

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/b008ef304e34 Part 1. GetGoodSelPointForNode doesn't work with ePrevousWord action. r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b391690334 Part 2. Add test. r=masayuki

https://hg.mozilla.org/mozilla-central/rev/b008ef304e34 https://hg.mozilla.org/mozilla-central/rev/d2b391690334

I'm sure either Makoto-san or Masayuki-san (or is it Kato-san and Nakano-san?) will handle the uplift requests, right?

(In reply to Jorg K (GMT+1) from comment #30) > I'm sure either Makoto-san or Masayuki-san (or is it Kato-san and > Nakano-san?) will handle the uplift requests, right? I will do it.

Comment on attachment 8810253 [details] Bug 1314790 - Part 1. GetGoodSelPointForNode doesn't work with ePrevousWord action. Approval Request Comment [Feature/regressing bug #]: Bug 1248971 [User impact if declined]: When user uses outlook.com (web mail service), CTRL+BACKSPACE (remove previous word) doesn't wok on beginning of line. [Describe test coverage new/current, TreeHerder]: Landed in m-c with test [Risks and why]: Low. Add parameter check for direction. Bug 1248971 mistakes direction when using previous word direction. [String/UUID change made/needed]: None

Hi Florin, Can you help find someone in your team to verify if this issue is fixed as expected on the latest Nightly build?

I've talked to Brindusa to have her team verify this.

In Thunderbird's Mozmill testing we see the run time abort you added here: https://hg.mozilla.org/mozilla-central/rev/b008ef304e34#l1.98 INFO - [1750] ###!!! ABORT: CheckForEmptyBlock doesn't support this action yet: file /builds/slave/tb-try-c-cen-m64-d-00000000000/build/mozilla/editor/libeditor/HTMLEditRules.cpp, line 4885 Makoto-san, can you please take a look. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a854a856131554c2b0aaa273d57d1769cd547b6d&selectedJob=25669 I will see whether I can reproduce this locally an get you a stack dump.

This is our test that fails with a crash: mozmake SOLO_TEST=composition/test-image-insertion-dialog.js mozmill-one It just inserts an image into a compose window. You can do this manually in TB and it crashes straight away. Here is the stack: xul.dll!NS_DebugBreak(unsigned int aSeverity, const char * aStr, const char * aExpr, const char * aFile, int aLine) Line 403 C++ xul.dll!mozilla::HTMLEditRules::CheckForEmptyBlock(nsINode * aStartNode, mozilla::dom::Element * aBodyNode, mozilla::dom::Selection * aSelection, short aAction, bool * aHandled) Line 4889 C++ xul.dll!mozilla::HTMLEditRules::WillDeleteSelection(mozilla::dom::Selection * aSelection, short aAction, short aStripWrappers, bool * aCancel, bool * aHandled) Line 1845 C++ xul.dll!mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection * aSelection, mozilla::RulesInfo * aInfo, bool * aCancel, bool * aHandled) Line 617 C++ xul.dll!mozilla::TextEditor::DeleteSelection(short aAction, short aStripWrappers) Line 689 C++ xul.dll!mozilla::HTMLEditor::InsertElementAtSelection(nsIDOMElement * aElement, bool aDeleteSelection) Line 1552 C++ HTMLEditRules::WillDeleteSelection() and CheckForEmptyBlock() get called with aAction==0. Call comes from case EditAction::deleteSelection: return WillDeleteSelection(aSelection, info->collapsedAction, info->stripWrappers, aCancel, aHandled); in HTMLEditRules::WillDoAction() So this is not ready for prime-time use yet.

Looks like there is eNone == 0 https://dxr.mozilla.org/comm-central/rev/5e76768327660437bf3486554ad318e4b70276e1/mozilla/editor/nsIEditor.idl#31 and that is in fact used ... https://dxr.mozilla.org/mozilla-central/search?q=nsIEditor%3A%3AeNone&redirect=false ... particularly when calling DeleteSelection(). So maybe just remove the over-zealous assert ;-)

What method (of nsIEditor?) does thunderbird call?

(for adding test case)

OK, maybe, inserting image causes this assertion... (from code). But it is too hard to create test case...

Comment on attachment 8810253 [details] Bug 1314790 - Part 1. GetGoodSelPointForNode doesn't work with ePrevousWord action. cancel approval request until adding new fix.

Hmm, it's a case of collapsed selection but aAction is eNone? Sounds odd to me, what should be removed? It seems that doing nothing might be correct. However, it tries to do something explicitly... https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/HTMLEditRules.cpp#1839,1844,1859,1862-1865 # Anyway, I don't understand well what CheckForEmptyBlock() should does because the name and implementation is mismatch but it's been maintained for hiding symptoms...

Neither mHTMLEditor->ExtendSelectionForDelete() nor CheckBidiLevelForDeletion() (called after CheckForEmptyBlock() and before quitting from it when aAction is eNone) removes anything. So, I feel CheckForEmptyBlock() should do nothing. I checked the change history, but CheckForEmptyBlock() exists in first revision, so, it was written by Netscape at closed source.

Insert images might use eNone for delete selection. So we should be ignore for eNone to set selection.

As you are aware, Japan (and Australia/NZ) are ahead of Europe. So I just got back to the screen and missed answering your questions. The fix looks good, please get it landed. Thanks for the quick response.

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #34) > I've talked to Brindusa to have her team verify this. Added myself to QA Contact to verify when new fix is available.

(In reply to Jorg K (GMT+1) from comment #45) > The fix looks good, please get it landed. Thanks for the quick response. On second thought: Do you really need a NS_RUNTIMEABORT("CheckForEmptyBlock doesn't support this action yet"); ? Wouldn't a MOZ_ASSERT be enough? Do you really want to crash user sessions should that ever trigger? On the other hand, you have now covered all seven values, so I don't see how that would ever trigger.

> On the other hand, you have now covered all seven values, so I don't see how that would ever trigger. If it's an named enum, we can detect new value isn't handled by switch statement, however, IIRC, it might be impossible when it's a non-named enum: https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dist/include/nsIEditor.h#62

This bug is severely impacting Thunderbird development, since our Mozmill test suite crashes on all build types (opt/debug) and platforms. That means that tests later in the suite don't get run. Which means we don't detect regressions and we can't add new tests or improve existing ones. IMHO a MOZ_ASSERT would detect the unlikely case that a new value is ever added to the enum since all thinkable cases are already covered: Backwards one character/word or to the beginning of the line. Forward one character/word or to the end of the line. None. Seven cases in total. But anyway, you're the module owner, so please land anything you deem fit as soon as possible.

(In reply to Jorg K (GMT+1) from comment #49) > This bug is severely impacting Thunderbird development, since our Mozmill > test suite crashes on all build types (opt/debug) and platforms. That means > that tests later in the suite don't get run. Which means we don't detect > regressions and we can't add new tests or improve existing ones. > > IMHO a MOZ_ASSERT would detect the unlikely case that a new value is ever > added to the enum since all thinkable cases are already covered: > Backwards one character/word or to the beginning of the line. > Forward one character/word or to the end of the line. > None. Seven cases in total. > > But anyway, you're the module owner, so please land anything you deem fit as > soon as possible. Tree is close now due to infra issue. After reopening this, I will land it

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3511bf4d144 Part 3. Ignore nsIEditor::eNone case. r=masayuki

Comment on attachment 8810253 [details] Bug 1314790 - Part 1. GetGoodSelPointForNode doesn't work with ePrevousWord action. Approval Request Comment See comment #32. Repeating here: [Feature/regressing bug #]: bug 1265800 [User impact if declined]: When user uses outlook.com (web mail service), CTRL+BACKSPACE (remove previous word) doesn't wok on beginning of line. [Describe test coverage new/current, TreeHerder]: Has test. [Risks and why]: Low. Add parameter check for direction. Bug 1265800 mistakes direction when using previous word direction. [String/UUID change made/needed]: None.

Comment on attachment 8812064 [details] [diff] [review] Bug 13134790 - Part 3. Ignore nsIEditor::eNone case Approval Request Comment See comment #53. This additional changeset fixes a problem introduced in part 1.

Ready to verify now.

Reproduced the initial issue on Nightly 52 (Build ID: 20161102030205). I can confirm that the initial issue is no longer reproducible - Ctrl-Backspace at beginning of paragraph doesn't make the cursor jump to beginning of previous paragraph. Verified using the latest Nightly 53.0a2 (Build ID: 20161123030208) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11 (used Alt+Backspace) But on https://jsfiddle.net/d_toybox/uoz4xytt/ - Ctrl+Backspace at the beginning of paragraph does not delete the last word from the previous paragraph instead positions the cursor after the last word from the previous paragraph. For e.g. If I write: 1. "First paragraph" hit enter 2. "second one" hit enter 3. hit Ctrl+backspace After hitting Ctrl+Backspace in step 3, the cursor is positioned right after "one" in the previous paragraph ("one" is not deleted as it would be if I follow the same steps in a Gmail compose window). Please see the attached screen cast for more details. Makoto, should this behave differently on the 2 sites (jsfiddle and gmail)?

The Gmail compose Window most likely doesn't use paragraphs. Have you looked with the DOM Inspector? Also it is possible that Gmail use a JS editor behind the scenes. The Outlook/Hotmail compose Window does use paragraphs, that's where this bug was first reported in duplicate bug 1306488. Please repeat the experiment there.

FWIW, the behavior described above is what I see in recent Thunderbird daily builds: Ctrl-Backspace moves to the end of the previous paragraph rather than deleting the last word in the previous paragraph. If that's not considered the correct behavior, then I suppose we need to open a separate bug about that. I believe that problem actually predates the problem reported in this bug.

Verified on Firefox 50 release and I see the same behavior as on Nightly 53.0a1 (on both jsfiddle and gmail). Related to the Gmail issue - if I use paragraphs (edited html using the Inspector) I see the same behavior (Ctrl+Backspace moves the cursor to the end of the previous paragraph) I can confirm the fix also on Outlook - verified using Nightly 53.0a1 (Build ID: 20161123030208) on Mac OS X 10.11, Windows 10 and Ubuntu 16.04. Based on the above and on Comments 56-58, setting the status of this issue to Verified Fixed.

Comment on attachment 8810253 [details] Bug 1314790 - Part 1. GetGoodSelPointForNode doesn't work with ePrevousWord action. Fix a regression issue related editor and was verified. Beta51+ and Aurora52+. Should be in 51 beta 3.

https://hg.mozilla.org/releases/mozilla-aurora/rev/86def315a579 https://hg.mozilla.org/releases/mozilla-aurora/rev/7b4599e0220c https://hg.mozilla.org/releases/mozilla-aurora/rev/12a7e9d627b5

Makoto, the test part has problems to apply to beta (the other 2 worked fine and will uplift this) but could you take a look at part 2 for beta ? thanks!

https://hg.mozilla.org/releases/mozilla-beta/rev/ba5384798a4d https://hg.mozilla.org/releases/mozilla-beta/rev/881b37dc7f94

Here you go. Just a merge conflict on the mochitest.ini file.

Backed out the test uplift to Beta because it uses SpecialPowers.doCommand, which is only available on 52+ (added in bug 1271119). Doesn't seem worth the effort trying to backport that or to rewrite the test to avoid it, so let's just drop it and focus on manual verification on 51. https://hg.mozilla.org/releases/mozilla-beta/rev/b48e9c3fa1709a5af6b66ea9404876970350c822

I managed to reproduce this issue on Firefox 52.0a1 (2016-11-02) and on Windows 10x64. The issue is no longer reproducible on Firefox 51.0b10, or on Firefox 52.0a2 (2016-12-29). The tests were performed under Windows 10x64, Mac OS X 10.12.1 and under Ubuntu 16.04x64.