[Tracking Requested - why for this release]: regression Steps to reproduce: 1. Log in to IRCCloud, either on https://irccloud.mozilla.com/ or on https://www.irccloud.com/ 2. Go to any channel. 3. Type some text in the message textbox and press Enter. Expected results: The text should disappear and the caret should move to the start of the textbox. Actual results: The text disappears and the caret moves to the *end* of the textbox. This is a regression from bug 1358025.

We seem to move to caret position after bagus node. If empty document, we should set caret position before bagus node.

tracking as new regression.

why we check HandleInlineSpellCheck's error on AfterEdit...

Comment on attachment 8872579 [details] Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. https://reviewboard.mozilla.org/r/144104/#review147850 ::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:149 (Diff revision 1) > deleted = !prevNode->IsInComposedDoc(); > } > + if (aAction == EditAction::setText) { > + // setText may remove the previous node when value is empty. > + deleted = !prevNode->IsInComposedDoc() || > + static_cast<int32_t>(prevNode->Length()) < aPreviousOffset; Is this really correct? Why other transaction types cannot use this condition for using the simple result?

Comment on attachment 8872580 [details] Bug 1368544 - Part 2. Add setting empty value test whether creating trailing node. https://reviewboard.mozilla.org/r/144106/#review147854 ::: editor/libeditor/tests/test_bug1368544.html:74 (Diff revision 1) > + synthesizeKey("A", {}); > + synthesizeKey("A", {}); > + synthesizeKey("A", {}); > + is(textarea.value, "AAA", "value is AAA"); > + textarea.addEventListener("keyup", (e) => { > + if (e.keyCode == 13) { nit: |e.key == "Enter"| or |e.keyCode == KeyboardEvent.DOM_VK_RETURN| is easier to read.

Comment on attachment 8872579 [details] Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. https://reviewboard.mozilla.org/r/144104/#review147850 > Is this really correct? Why other transaction types cannot use this condition for using the simple result? HandleInlineSpellCheck returns NS_ERROR_DOM_INDEX_SIZE_ERR because previous selection (caret position) becomes invalid. I think that best way is that we should ignore error of HandleInlineSpellCheck into AfterEdit. How do you think? If transaction is deleteSelection, "deleted" is always set to true. So no handled.

Comment on attachment 8872579 [details] Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. https://reviewboard.mozilla.org/r/144104/#review148164 ::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:149 (Diff revision 1) > deleted = !prevNode->IsInComposedDoc(); > } > + if (aAction == EditAction::setText) { > + // setText may remove the previous node when value is empty. > + deleted = !prevNode->IsInComposedDoc() || > + static_cast<int32_t>(prevNode->Length()) < aPreviousOffset; Ah, I missed this line: > bool deleted = aAction == EditAction::deleteSelection; So, I think that only > delete = !prevNode->IsInComposedDoc(); is correct. However, I still suspect > static_cast<int32_t>(prevNode->Length()) < aPreviousOffset; If it's deleted, looks like that spell checker will check only the last word. https://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/extensions/spellcheck/src/mozInlineSpellChecker.cpp#291,305,307,313 https://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/extensions/spellcheck/src/mozInlineSpellChecker.cpp#410,414,418,421-422 However, setting value may replace two or more words. For example, "abc def" may be replaced with "uvw xyz". Then, all words should be checked rather than the last word at the anchor (I don't know where to set caret after setting value, though). So, I guess that when aAction is EditAction::setText but there is a text node, we need to do something more. Perhaps, mAnchorRange should be cleared and mNoCheckRange is set to all range of the editor? Or you should add new Operation value, eOpSetText and handle it in mozInlineSpellStatus::FinishInitOnEvent() specifically.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #9) > So, I guess that when aAction is EditAction::setText but there is a text > node, we need to do something more. Perhaps, mAnchorRange should be cleared > and mNoCheckRange is set to all range of the editor? Or you should add new > Operation value, eOpSetText and handle it in > mozInlineSpellStatus::FinishInitOnEvent() specifically. Humm, setText replace all text, So I think that we shouldn't use mCachedSelection for HandleInlineSpellCheck. HandleInlineSpellCheck generates range for spellchecker. When using setText, the range for spellchecker becomes all text. new fix is coming, so please r- for current fix.

Comment on attachment 8872579 [details] Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. https://reviewboard.mozilla.org/r/144104/#review148200 Okay. Thank you for your work!

Comment on attachment 8872579 [details] Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. https://reviewboard.mozilla.org/r/144104/#review148230 ::: commit-message-34ac1:5 (Diff revision 2) > +Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. r?masayuki > + > +textarea element will use fallback code (using TextEditor::DeleteSelection) to set empty value for SetText. > + > +Actually ,if EditorBase::HandleInlineSpellCheck returns error in AfterEdit, we might not create bogus node and trailing br node. Since SetText's fallback uses DeleteSelection, SpellCheckAfterEditorChange will return error when previous selection is invalid on SetText transaction. s/Actually ,if/Actually, if... But is this comment still valid?

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #14) > > +Actually ,if EditorBase::HandleInlineSpellCheck returns error in AfterEdit, we might not create bogus node and trailing br node. Since SetText's fallback uses DeleteSelection, SpellCheckAfterEditorChange will return error when previous selection is invalid on SetText transaction. > > s/Actually ,if/Actually, if... > > But is this comment still valid? This comment is about current behavior before this is fix.

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/0144d912dcf0 Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction. r=masayuki https://hg.mozilla.org/integration/autoland/rev/85449d8e83da Part 2. Add setting empty value test whether creating trailing node. r=masayuki

https://hg.mozilla.org/mozilla-central/rev/0144d912dcf0 https://hg.mozilla.org/mozilla-central/rev/85449d8e83da