Clear undo stack when text control is set programatically

Clear undo stack when text control is set programatically

This patch is concerned with 348505, 277302 issues.
UndoStack isn't updated currently when text control is set programatically.
In that case, undo stack may be broken and then something unexpected can happen.
Therefore we should change to be able to clear undo stack. (similar to IE, Safari browser)

BUG=348505, 277302

[eseidel, 2014-03-04 02:08:16]

I don't feel like I have context on this issue sufficient to review it.

I've cc'd some other editing folks in hopes that they do.

https://codereview.chromium.org/184103011/diff/1/Source/core/editing/UndoStac...
File Source/core/editing/UndoStack.cpp (right):

https://codereview.chromium.org/184103011/diff/1/Source/core/editing/UndoStac...
Source/core/editing/UndoStack.cpp:126: if (canUndo())
Why check canUndo here?

[zino, 2014-03-04 04:12:16]

On 2014/03/04 02:08:16, eseidel wrote:
> I don't feel like I have context on this issue sufficient to review it.
> 
> I've cc'd some other editing folks in hopes that they do.
> 
>
https://codereview.chromium.org/184103011/diff/1/Source/core/editing/UndoStac...
> File Source/core/editing/UndoStack.cpp (right):
> 
>
https://codereview.chromium.org/184103011/diff/1/Source/core/editing/UndoStac...
> Source/core/editing/UndoStack.cpp:126: if (canUndo())
> Why check canUndo here?

Thank you for comment :)

Deque.clear() is very expensive function because memory reallocation.
So, I think we can reduce calling Deque.clear() function if we check isEmpty.

[yosin_UTC9, 2014-03-04 05:02:57]

I think we should record programtical changes into undo stack rather than clear
undo stack. It is easy to do so for text field and textarea. Although, it is
difficult to do so for contenteditable without performance degradation.

Clearing all entries of undo stack isn't good for users. Users can't undo for
non-porgramatically changed controls and editables.

[ojan, 2014-03-04 22:23:05]

On 2014/03/04 05:02:57, Yoshi wrote:
> I think we should record programtical changes into undo stack rather than
clear
> undo stack.

+1. IE's behavior of clearing the undo stack is not user or developer friendly.

[zino, 2014-03-05 05:42:50]

On 2014/03/04 22:23:05, ojan wrote:
> On 2014/03/04 05:02:57, Yoshi wrote:
> > I think we should record programtical changes into undo stack rather than
> clear
> > undo stack.
> 
> +1. IE's behavior of clearing the undo stack is not user or developer
friendly.

@Yoshi, @ojan

Thank you for comments :)

I agree with your opinion. But I think it looks difficult in current
chromium/blink
Becasue one frame has only one undo stack. one text box doesn't have undo stack.
So, It can be a problem if there are many text box.

For example, let's assume there are two text boxes. (A-box and B-box)
A-box is updated by user. (non-programatically)
B-box is updated by developer. (programatically)
If we record programtical changes into undo stack, B-box's changes are stored
into undo stack, too.

A user typed "abcd" to A-box.
** UndoStack **
----------------------------- top
A-box - "abcd"
----------------------------- bottom

Value of B-box is updated three times by some programatic logic.
** UndoStack **
----------------------------- top
B-box - "mnop"
B-box - "ijkl"
B-box - "efgh"
A-box - "abcd"
----------------------------- bottom

A user re-typed "hello" to A-box again.
** UndoStack **
----------------------------- top
A-box - "hello"
B-box - "mnop"
B-box - "ijkl"
B-box - "efgh"
A-box - "abcd"
----------------------------- bottom

And then a user performed (undo) operation.
Maybe, value of A-box would be replaced as "abcd".
** UndoStack **
----------------------------- top
B-box - "mnop"
B-box - "ijkl"
B-box - "efgh"
A-box - "abcd"
----------------------------- bottom

A user performed (undo) operation again.
A user would expect to replace "abcd" with "" in A-box.
In contrast to expectations, It would change only value of B-box.
** UndoStack **
----------------------------- top
B-box - "ijkl"
B-box - "efgh"
A-box - "abcd"
----------------------------- bottom

Users may not be interested in programatic changes.
They will be interested in only user(non-programatic) changes.

Currently, IE and Safari clear undo stack when input text box is set
programatically.
Firefox doesn't clear. But firefox is different with chromium.
In firefox, one editable box has one undo stack. It doesn't share undo stack.

I don't think clearing undo stack is good solution too.
But most other browsers usually do so.
If there is a better way, always welcome!

[yosin_UTC9, 2014-03-05 08:08:49]

I think recording programatic changes into undo stack works without surprising.
Let's do record programatic changes into undo stack for input and textarea.
It can be done in |HTMLTextFormControlElement::setInnerTextValue()|.
It seems introducing new class |TextFormControlChange| derived from |UndoStep|
is easier than reusing existing UndoStep derived classes,
|EditCommandComposition|.

[ojan, 2014-03-05 18:08:14]

On 2014/03/05 05:42:50, zino wrote:
> Firefox doesn't clear. But firefox is different with chromium.
> In firefox, one editable box has one undo stack. It doesn't share undo stack.

Perhaps this is something we should change. Having an undo stack shared across
textareas always struck me as really strange. Of course, that's a big enough
change that we probably want to discuss it on blink-dev before changing it.

[eseidel, 2014-05-30 00:26:09]

This is really up to Yosin as to if the current editing/ owners want this.