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

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mstange, Assigned: m_kato)

Tracking

({regression})

Trunk
mozilla55
All
Mac OS X
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

a year ago
Created attachment 8872434 [details]
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.
Flags: needinfo?(m_kato)
(Assignee)

Updated

a year ago
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Priority: -- → P1
(Assignee)

Comment 1

a year ago
Created attachment 8872480 [details]
test case

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.
tracking-firefox55: ? → +
(Assignee)

Comment 3

a year ago
why we check HandleInlineSpellCheck's error on AfterEdit...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8872580 - Flags: review?(masayuki) → review+
(Assignee)

Comment 8

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 10

a year ago
(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.
(Assignee)

Updated

a year ago
Attachment #8872579 - Flags: review?(masayuki)
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!
Attachment #8872579 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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?
Attachment #8872579 - Flags: review?(masayuki) → review+
(Assignee)

Comment 15

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
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

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0144d912dcf0
https://hg.mozilla.org/mozilla-central/rev/85449d8e83da
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.