1368544 - Textbox caret / cursor moves to the right on IRCCloud after pressing enter

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Markus Stange [:mstange]]

Attachment: screenshot of misplaced caret

[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.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: test case

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

Comment 2

[Julien Cristau [:jcristau]]

tracking as new regression.

Comment 3

[Makoto Kato [:m_kato]]

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

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction.

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 shouldn't return error when previous selection is invalid on SetText transaction.

Also, we might not have to check HandleInlineSpellCheck's return error to always adjust bogus node and trailing node...

Review commit: https://reviewboard.mozilla.org/r/144104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144104/

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1368544 - Part 2. Add setting empty value test whether creating trailing node.

Review commit: https://reviewboard.mozilla.org/r/144106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144106/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 8

[Makoto Kato [:m_kato]]

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 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 10

[Makoto Kato [:m_kato]]

(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 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 12

[Makoto Kato [:m_kato]]

Comment on attachment 8872579 [details]
Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144104/diff/1-2/

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8872580 [details]
Bug 1368544 - Part 2. Add setting empty value test whether creating trailing node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144106/diff/1-2/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 15

[Makoto Kato [:m_kato]]

(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.

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8872579 [details]
Bug 1368544 - Part 1. Use root node for mozInlineSpellChecker::SpellCheckAfterEditorChange on setText trasaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144104/diff/2-3/

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8872580 [details]
Bug 1368544 - Part 2. Add setting empty value test whether creating trailing node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144106/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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