Closed Bug 1328023 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: arni2033, Assigned: m_kato)

References

Details

(Keywords: regression)

Attachments

(2 files)

>>>   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.
No longer blocks: 1277113
Component: Untriaged → Editor
Product: Firefox → Core
Priority: -- → P2
Assignee: nobody → m_kato
Sign, although bug 1310912 updates selection via RangeUpdater, deleteTextTransaction doesn't consider restoring current selection.
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 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.
Attachment #8831629 - Flags: review?(masayuki) → review+
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.)
Attachment #8831630 - Flags: review?(masayuki) → review+
(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 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.
Pushed by m_kato@ga2.so-net.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/1e2993a59de6
https://hg.mozilla.org/mozilla-central/rev/c87bbf779eda
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8831629 - Flags: approval-mozilla-beta?
Attachment #8831629 - Flags: approval-mozilla-aurora?
Attachment #8831630 - Flags: approval-mozilla-beta?
Attachment #8831630 - Flags: approval-mozilla-aurora?
Comment on attachment 8831629 [details]
Bug 1328023 - Part 1. Don't use RangeUpdater except to composition transaction.

editor regression fix, aurora53+, beta52+
Attachment #8831629 - Flags: approval-mozilla-beta?
Attachment #8831629 - Flags: approval-mozilla-beta+
Attachment #8831629 - Flags: approval-mozilla-aurora?
Attachment #8831629 - Flags: approval-mozilla-aurora+
Attachment #8831630 - Flags: approval-mozilla-beta?
Attachment #8831630 - Flags: approval-mozilla-beta+
Attachment #8831630 - Flags: approval-mozilla-aurora?
Attachment #8831630 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.