1328023 - Caret position and selection in <input> aren't restored correctly after undo/redo

Status: RESOLVED FIXED

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

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161119030204 (2016-11-19)
STR_1:
1. Open url   data:text/html,<input autofocus>
2. Type "h" in input
3. Press BackSpace to delete typed text
4. Press Ctrl+Z

AR:  `[caret]h`
ER:  `h[caret]`


STR_2:
1. Open url   data:text/html,<input autofocus>
2. Type "asdf" in input
3. Press Ctrl+A, then Delete to delete typed text
4. Press Ctrl+Z

AR:  `[caret]asdf`
ER:  `{asdf}`   ({text} means that text is selected)


This is regression from bug 1310912. Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eb3dd588e1afeda3d261078deabaf40fcc77c982&tochange=bac6adefd6a2e423ced72b735471cb6bae7ee741@ Makoto Kato [:m_kato] (PTO 12/28-1/4):
It seems that this is a regresion caused by your change. Please have a look.

Comment 1

[Makoto Kato [:m_kato]]

Sign, although bug 1310912 updates selection via RangeUpdater, deleteTextTransaction doesn't consider restoring current selection.

Comment 2

[Makoto Kato [:m_kato]]

Make sense.  I think that I need back out Part 3 of bug 1310912.  Since CompositionTransaction modifies current selection node, PlaceholderTransaction will save invalid transaction now.  CompositionTransaction::UndoTransaction sets new selection, so PlaceholderTransaction::UndoTransaction shouldn't restore selection.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1328023 - Part 1. Don't use RangeUpdater except to composition transaction.

Part 3 fix of bug 1310912 is incorrect for not composition transaction.  PlaceholderTransation is for saving and restoring current selection for undo. So we shouldn't use range updater to normal transaction.

Composition transaction can modify multiple nodes and it merges text node for ime into single text node. So if current selection is into IME text node, it might be failed to restore selection by UndoTransaction. So we need update selection by range updater to work UndoTransaction.

Also, CompositionTransaction::UndoTransaction will set selection after committed text. So at finally, selection will set correct position that composition transaction wants.

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

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1328023 - Part 2. Add test for undo.

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

Comment 5

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

Comment on attachment 8831629 [details]
Bug 1328023 - Part 1. Don't use RangeUpdater except to composition transaction.

https://reviewboard.mozilla.org/r/108178/#review109508

::: editor/libeditor/EditorBase.cpp:935
(Diff revision 1)
> +      // Composition transaction can modify multiple nodes and it merges text
> +      // node for ime into single text node.
> +      // So if current selection is into IME text node, it might be failed
> +      // to restore selection by UndoTransaction.
> +      // So we need update selection by range updater.
> +      if (mPlaceHolderName == nsGkAtoms::IMETxnName) {

Hmm, I really don't like this kind of check. However, I have no idea of a good method name to check this since we need to create it in much wide scope class like EditorBase.

I think that we should create a nested class to manage mPlaceHolder* and which should have |bool NeedsToUseRangeUpdater() cosnt| or something. But of course, this is out scope of this bug.

Comment 6

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

Comment on attachment 8831630 [details]
Bug 1328023 - Part 2. Add test for undo.

https://reviewboard.mozilla.org/r/108180/#review109510

::: editor/libeditor/tests/test_bug1328023.html:35
(Diff revision 1)
> +  synthesizeKey("Z", { accelKey: true });
> +  is(elm.value, "AB", "AB is input.value now");
> +
> +  synthesizeKey("C", {});
> +  is(elm.value, "ABC", "ABC is input.value now");

Could you check "Redo" case after here? E.g., Accel+Z -> Accel+Shift+Z -> "D", then, "ABCD". (FYI: You need to use Accel+Shift+Z for redo because Accel+Y doesn't work on macOS.)

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] from comment #5)
> Comment on attachment 8831629 [details]
> Bug 1328023 - Part 1. Don't use RangeUpdater except to composition
> transaction.
> 
> https://reviewboard.mozilla.org/r/108178/#review109508
> 
> ::: editor/libeditor/EditorBase.cpp:935
> (Diff revision 1)
> > +      // Composition transaction can modify multiple nodes and it merges text
> > +      // node for ime into single text node.
> > +      // So if current selection is into IME text node, it might be failed
> > +      // to restore selection by UndoTransaction.
> > +      // So we need update selection by range updater.
> > +      if (mPlaceHolderName == nsGkAtoms::IMETxnName) {
> 
> Hmm, I really don't like this kind of check. However, I have no idea of a
> good method name to check this since we need to create it in much wide scope
> class like EditorBase.
> 
> I think that we should create a nested class to manage mPlaceHolder* and
> which should have |bool NeedsToUseRangeUpdater() cosnt| or something. But of
> course, this is out scope of this bug.

Yes, I will file a new bug for placeholdertransaction should handle committed text selection (Now, we handle selection on Composition::UndoTransaction too, but I think that now might not be good approach.)

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8831630 [details]
Bug 1328023 - Part 2. Add test for undo.

https://reviewboard.mozilla.org/r/108180/#review109510

> Could you check "Redo" case after here? E.g., Accel+Z -> Accel+Shift+Z -> "D", then, "ABCD". (FYI: You need to use Accel+Shift+Z for redo because Accel+Y doesn't work on macOS.)

I will add it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2993a59de6
Part 1. Don't use RangeUpdater except to composition transaction. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/c87bbf779eda
Part 2. Add test for undo. r=masayuki

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e2993a59de6
https://hg.mozilla.org/mozilla-central/rev/c87bbf779eda

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8831629 [details]
Bug 1328023 - Part 1. Don't use RangeUpdater except to composition transaction.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1310912

[User impact if declined]:
User wants to Undo such as ctrl+Z into text box after pushing [Backspace] key, caret isn't set previous position.

[Is this code covered by automated tests?]:
Yes.  test is added

[Has the fix been verified in Nightly?]:
Yes with the latest m-c.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
No

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
Bug 1310912 requires for IME composition only.  So except to IME composition, I back to original behavior for caret.

[String changes made/needed]:
No

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8831629 [details]
Bug 1328023 - Part 1. Don't use RangeUpdater except to composition transaction.

editor regression fix, aurora53+, beta52+

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e235e56f062a
https://hg.mozilla.org/releases/mozilla-aurora/rev/46497fa848f7

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6698c9784e3f
https://hg.mozilla.org/releases/mozilla-beta/rev/47cff7384496