HTMLTextAreaElement.setSelectionRange should not change focus.

HTMLTextAreaElement.setSelectionRange should not change focus.

HTMLTextAreaElement.setSelectionRange(start,end) changes focus if |start| is not 
|end|.
However, HTMLTextAreaElement.setSelectionRange should not change focus in any 
condition.
FireFox follows such behavior at least. 

Source/core/html/HTMLTextFormControlElement.cpp/h:
- Let setSelectionRange not change focus without the ChangeFocus option.
- SelectionOption option is used to change text selection like select() calling 
on disabled input field.
- 278L: We must set 'forward' direction if direction is 'none' on not Mac.
- FrameSelection.setSelection is called when selectionOption is ChangeSelection 
or this TextFormControl element is already focused.

Source/core/html/HTMLTextAreaElement.cpp/h:
- Change setValueCommon to call setSelectionRange except for initialize.
- setSelectionRange calls update layout, which needs updated validity so call 
setNeedsValidityCheck earlier.


LayoutTests/fast/forms/selection-setSelectionRange-focusing.html:
- Added test. Check focus not to be changed.

LayoutTests/fast/forms/textarea-selection-preservation.html:
- Add test cases resetting selection. This is following the FireFox behavior.

(Other layout tests):
- Some tests expect setSelectionRange to change focus, thus I add extra focus.
- Some tests expect resetting value of textarea to change selection to 0, thus I 
add extra setSelectionRange(0, 0);

TEST=LayoutTests/fast/forms/selection-setSelectionRange-focusing.html, textarea-
selection-preservation.html
BUG=393504
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178822

[yosin_UTC9, 2014-07-16 07:09:15]

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:79: bool setValueCommon(const String&,
TextFieldEventBehavior);
nit: Could you add comment about |bool| return value? It is hard to guess what
value returned by |setValueCommon()|.

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:298: LocalFrame* frame =
document().frame();
nit: You can move |!frame| before L286.

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.h (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.h:47: Default,
nit: Could you change |Default| to another what it does? I guess it is
|NotChangeFocus|.

[tkent, 2014-07-16 07:11:34]

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:184: setSelectionRange(0,
std::numeric_limits<int>::max(), SelectionHasNoDirection, MustFocus);
Is this compatible with other browsers?
Can we add focus() before setSelectionRange, and remove MustFocus flag?

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:323:
frame->selection().setSelection(newSelection);
We should pass DoNotSetFocus option, IMO.

[yoichio, 2014-07-16 08:07:12]

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.h (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.h:79: bool setValueCommon(const String&,
TextFieldEventBehavior);
On 2014/07/16 07:09:14, Yosi_UTC9 wrote:
> nit: Could you add comment about |bool| return value? It is hard to guess what
> value returned by |setValueCommon()|.

Done.

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:184: setSelectionRange(0,
std::numeric_limits<int>::max(), SelectionHasNoDirection, MustFocus);
On 2014/07/16 07:11:34, tkent wrote:
> Is this compatible with other browsers?
Both FF and IE do.

> Can we add focus() before setSelectionRange, and remove MustFocus flag?
To add focus() and call FrameSelection::setSelection with DoNotSetFocus doesn't
work.
If we should do, we need extra investigation.

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:298: LocalFrame* frame =
document().frame();
On 2014/07/16 07:09:14, Yosi_UTC9 wrote:
> nit: You can move |!frame| before L286.

Done.

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:323:
frame->selection().setSelection(newSelection);
On 2014/07/16 07:11:34, tkent wrote:
> We should pass DoNotSetFocus option, IMO.

Acknowledged.

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.h (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.h:47: Default,
On 2014/07/16 07:09:14, Yosi_UTC9 wrote:
> nit: Could you change |Default| to another what it does? I guess it is
> |NotChangeFocus|.

Done.

[tkent, 2014-07-16 23:34:16]

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/140001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:184: setSelectionRange(0,
std::numeric_limits<int>::max(), SelectionHasNoDirection, MustFocus);
On 2014/07/16 08:07:12, yoichio wrote:
> On 2014/07/16 07:11:34, tkent wrote:
> > Is this compatible with other browsers?
> Both FF and IE do.
> 
> > Can we add focus() before setSelectionRange, and remove MustFocus flag?
> To add focus() and call FrameSelection::setSelection with DoNotSetFocus
doesn't
> work.
> If we should do, we need extra investigation.

Please have an investigation.
Adding no flag is simpler and better.

[tkent, 2014-07-16 23:35:19]

https://codereview.chromium.org/392573002/diff/160001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:289: return;
Is this compatible with other browsers?  Shouldn't we cache the specified values
even if the document has no frame?

[yoichio, 2014-07-17 07:16:01]

Change setSelectionRange not to change focus.
However we still should manage text selection via 
FrameSelection, see SelectionOption.

https://codereview.chromium.org/392573002/diff/160001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/160001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:289: return;
On 2014/07/16 23:35:19, tkent wrote:
> Is this compatible with other browsers?  Shouldn't we cache the specified
values
> even if the document has no frame?
I'm not sure and this change is not related to this CL.
Then I revert this.

[yoichio, 2014-07-18 05:52:07]

Ping?

[yosin_UTC9, 2014-07-18 06:52:20]

LGTM

[tkent, 2014-07-18 07:24:16]

> Source/core/html/HTMLTextAreaElement.cpp/h: 
> - Change the setSelectionRange calling to call from only setValue function,
not 
> default value setting. 
> - This call doesn't change text selection. 

Why is this change necessary?  Is it related to the main goal of this CL?

[tkent, 2014-07-18 07:24:24]

https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:188: if (isFocusable())
Doesn't focus() work?

https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:189:
document().page()->focusController().setFocusedElement(this,
document().frame());
This dispatches events, and arbitrary JavaScript code can run.  Please add
|RefPtrWillBeRawPtr<HTMLTextFormControlElement> protector(this);|

[yoichio, 2014-07-18 09:07:37]

On 2014/07/18 07:24:16, tkent wrote:
> > Source/core/html/HTMLTextAreaElement.cpp/h: 
> > - Change the setSelectionRange calling to call from only setValue function,
> not 
> > default value setting. 
> > - This call doesn't change text selection. 
> 
> Why is this change necessary?  Is it related to the main goal of this CL?
We have 2 mistakes in the current implementation.
1. setSelectionRange changes focus, you know.
2. HTMLTextAreaElement::setValueCommon calls the setSelectionRange 
only if the textarea element has focus, see L374:
In this situation, following code looks correct except the textarea has focus:
textarea.setSelectionRange(1,2);
textarea.value = "text";
shouldBe(4, textarea.selectionStart);
(see
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...)

Thus when we want to fix the setSelectionRange problem(#1), we should fix
problem #2.

A textarea element should do
1. In initialized with text, set both selectionStart/End to 0.
2. if new text is set, set  selectionStart/End to end of the text.

Thus we should call setSelectionRange, which doesn't focus, only when text.value
= 'new text' is called in javascript.

[yoichio, 2014-07-18 09:07:48]

https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:188: if (isFocusable())
On 2014/07/18 07:24:24, tkent wrote:
> Doesn't focus() work?
focus() causes SelectionChange event and setSelectionRange() also does.
The event should be kicked once.

https://codereview.chromium.org/392573002/diff/220001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:189:
document().page()->focusController().setFocusedElement(this,
document().frame());
On 2014/07/18 07:24:24, tkent wrote:
> This dispatches events, and arbitrary JavaScript code can run.  Please add
> |RefPtrWillBeRawPtr<HTMLTextFormControlElement> protector(this);|

Done.

[yoichio, 2014-07-22 01:08:14]

Ping.

[tkent, 2014-07-22 01:51:54]

On 2014/07/18 09:07:37, yoichio wrote:
> On 2014/07/18 07:24:16, tkent wrote:
> > > Source/core/html/HTMLTextAreaElement.cpp/h: 
> > > - Change the setSelectionRange calling to call from only setValue
function,
> > not 
> > > default value setting. 
> > > - This call doesn't change text selection. 
> > 
> > Why is this change necessary?  Is it related to the main goal of this CL?
> We have 2 mistakes in the current implementation.
> 1. setSelectionRange changes focus, you know.
> 2. HTMLTextAreaElement::setValueCommon calls the setSelectionRange 
> only if the textarea element has focus, see L374:
> In this situation, following code looks correct except the textarea has focus:
> textarea.setSelectionRange(1,2);
> textarea.value = "text";
> shouldBe(4, textarea.selectionStart);
> (see
>
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...)
> 
> Thus when we want to fix the setSelectionRange problem(#1), we should fix
> problem #2.
> 
> A textarea element should do
> 1. In initialized with text, set both selectionStart/End to 0.
> 2. if new text is set, set  selectionStart/End to end of the text.
> 
> Thus we should call setSelectionRange, which doesn't focus, only when
text.value
> = 'new text' is called in javascript.

In the following scenario, should we change the selection, or not?

textarae.blur();
textarea.value = "abcd";
textarea.setSelectionRange(1, 1);
textarea.value = "abcd";
// Should the selection be (1, 1) or (0, 0)?

[yoichio, 2014-07-22 02:16:06]

On 2014/07/22 01:51:54, tkent wrote:
> On 2014/07/18 09:07:37, yoichio wrote:
> > On 2014/07/18 07:24:16, tkent wrote:
> > > > Source/core/html/HTMLTextAreaElement.cpp/h: 
> > > > - Change the setSelectionRange calling to call from only setValue
> function,
> > > not 
> > > > default value setting. 
> > > > - This call doesn't change text selection. 
> > > 
> > > Why is this change necessary?  Is it related to the main goal of this CL?
> > We have 2 mistakes in the current implementation.
> > 1. setSelectionRange changes focus, you know.
> > 2. HTMLTextAreaElement::setValueCommon calls the setSelectionRange 
> > only if the textarea element has focus, see L374:
> > In this situation, following code looks correct except the textarea has
focus:
> > textarea.setSelectionRange(1,2);
> > textarea.value = "text";
> > shouldBe(4, textarea.selectionStart);
> > (see
> >
>
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...)
> > 
> > Thus when we want to fix the setSelectionRange problem(#1), we should fix
> > problem #2.
> > 
> > A textarea element should do
> > 1. In initialized with text, set both selectionStart/End to 0.
> > 2. if new text is set, set  selectionStart/End to end of the text.
> > 
> > Thus we should call setSelectionRange, which doesn't focus, only when
> text.value
> > = 'new text' is called in javascript.
> 
> In the following scenario, should we change the selection, or not?
> 
> textarae.blur();
> textarea.value = "abcd";
> textarea.setSelectionRange(1, 1);
> textarea.value = "abcd";
> // Should the selection be (1, 1) or (0, 0)?
If value is updated, the selection should be end of the text. Thus it Should be
(4, 4):
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-te...

It is confirmed in an existing test, in which a textarea is never focused:
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...
L25	    ta.value = "abc123";
L26	    shouldBe('ta.selectionStart', '6');
L27	    shouldBe('ta.selectionEnd', '6');

[tkent, 2014-07-22 02:22:44]

On 2014/07/22 02:16:06, yoichio wrote:
> > textarae.blur();
> > textarea.value = "abcd";
> > textarea.setSelectionRange(1, 1);
> > textarea.value = "abcd";
> > // Should the selection be (1, 1) or (0, 0)?
> If value is updated, the selection should be end of the text. Thus it Should
be
> (4, 4):
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-te...
> 
> It is confirmed in an existing test, in which a textarea is never focused:
>
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...
> L25	    ta.value = "abc123";
> L26	    shouldBe('ta.selectionStart', '6');
> L27	    shouldBe('ta.selectionEnd', '6');

It doesn't set the identical string.  My question is about the last one in the
test.

38	    debug("- set value to same value");
39	    ta.value = "abc123";
40	    shouldBe('ta.selectionStart', '3');
41	    shouldBe('ta.selectionEnd', '4');

Does this compatible with other browsers? Does this conform to the standard?

[tkent, 2014-07-22 02:58:04]

On 2014/07/22 02:22:44, tkent wrote:
> On 2014/07/22 02:16:06, yoichio wrote:
> > > textarae.blur();
> > > textarea.value = "abcd";
> > > textarea.setSelectionRange(1, 1);
> > > textarea.value = "abcd";
> > > // Should the selection be (1, 1) or (0, 0)?
> > If value is updated, the selection should be end of the text. Thus it Should
> be
> > (4, 4):
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-te...
> > 
> > It is confirmed in an existing test, in which a textarea is never focused:
> >
>
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...
> > L25	    ta.value = "abc123";
> > L26	    shouldBe('ta.selectionStart', '6');
> > L27	    shouldBe('ta.selectionEnd', '6');
> 
> It doesn't set the identical string.  My question is about the last one in the
> test.
> 
> 38	    debug("- set value to same value");
> 39	    ta.value = "abc123";
> 40	    shouldBe('ta.selectionStart', '3');
> 41	    shouldBe('ta.selectionEnd', '4');
> 
> Does this compatible with other browsers? Does this conform to the standard?

Note that element focus doesn't matter.  I'm wondering if we need |isUpdated|
flag.

[yoichio, 2014-07-22 03:01:28]

On 2014/07/22 02:22:44, tkent wrote:
> On 2014/07/22 02:16:06, yoichio wrote:
> > > textarae.blur();
> > > textarea.value = "abcd";
> > > textarea.setSelectionRange(1, 1);
> > > textarea.value = "abcd";
> > > // Should the selection be (1, 1) or (0, 0)?
> > If value is updated, the selection should be end of the text. Thus it Should
> be
> > (4, 4):
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-te...
> > 
> > It is confirmed in an existing test, in which a textarea is never focused:
> >
>
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/textarea-se...
> > L25	    ta.value = "abc123";
> > L26	    shouldBe('ta.selectionStart', '6');
> > L27	    shouldBe('ta.selectionEnd', '6');
> 
> It doesn't set the identical string.  My question is about the last one in the
> test.
> 
> 38	    debug("- set value to same value");
> 39	    ta.value = "abc123";
> 40	    shouldBe('ta.selectionStart', '3');
> 41	    shouldBe('ta.selectionEnd', '4');
> 
> Does this compatible with other browsers? Does this conform to the standard?
I didn't found specification about setting same value to textarea.
FireFox does same and IE sets (0, 0);

[tkent, 2014-07-22 03:36:55]

On 2014/07/22 03:01:28, yoichio wrote:
> > Does this compatible with other browsers? Does this conform to the standard?

> I didn't found specification about setting same value to textarea.

It means we don't need the special handling for setting same value.
I recommend to make another CL to remove this special handling before this CL.

[yoichio, 2014-07-22 04:06:06]

On 2014/07/22 03:36:55, tkent wrote:
> On 2014/07/22 03:01:28, yoichio wrote:
> > > Does this compatible with other browsers? Does this conform to the
standard?
> 
> > I didn't found specification about setting same value to textarea.
> 
> It means we don't need the special handling for setting same value.
> I recommend to make another CL to remove this special handling before this CL.

Do you mean that, in short, we should delete lines from 361 to 364 in
HTMLTextAreaElement ?
http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLTextAreaEleme...
It is:
// Return early because we don't want to move the caret or trigger other side
effects
// when the value isn't changing. This matches Firefox behavior, at least.
if (normalizedValue == value())
    return;

[yoichio, 2014-07-22 08:34:14]

I know that this CL still leave some un-standard behaviors.
However I'm working for a textarea performance issue and this CL is needed.
Thus let me leave the problem about assigning same value.

I suggest to file a bug and add FIXME to HTMLTextAreaElement.cpp:L361.

[tkent, 2014-07-22 09:50:08]

On 2014/07/22 08:34:14, yoichio wrote:
> I know that this CL still leave some un-standard behaviors.
> However I'm working for a textarea performance issue and this CL is needed.
> Thus let me leave the problem about assigning same value.
> 
> I suggest to file a bug and add FIXME to HTMLTextAreaElement.cpp:L361.

It's acceptable.  However I have a concern with this issue because moving
setSelectionRange() call out of setValueCommon needs more work.  setValueCommon
is called in setNonDirtyValue, and setNonDirtyValue is called in
childrenChanged, resetImpl, and setDefaultValue. Don't you need to call
setSelectionRange in these cases?

[yoichio, 2014-07-22 10:15:31]

On 2014/07/22 09:50:08, tkent wrote:
> On 2014/07/22 08:34:14, yoichio wrote:
> > I know that this CL still leave some un-standard behaviors.
> > However I'm working for a textarea performance issue and this CL is needed.
> > Thus let me leave the problem about assigning same value.
> > 
> > I suggest to file a bug and add FIXME to HTMLTextAreaElement.cpp:L361.
> 
> It's acceptable.  However I have a concern with this issue because moving
> setSelectionRange() call out of setValueCommon needs more work. 
setValueCommon
> is called in setNonDirtyValue, and setNonDirtyValue is called in
> childrenChanged, resetImpl, and setDefaultValue. Don't you need to call
> setSelectionRange in these cases?

I guess all these functions are called for 'initialization' static HTML tree
like
<body><textarea>text</textarea></body>

What I like to do through moving setSelectionRange out from setValueCommon is 
to call setSelectionRange(text.length(), text.length()) only when a value of
textarea 
is dynamically changed.
That is because in the given html, following should be:
document.getElementsByTagName('textarea')[0].selectionStart == 0;
document.getElementsByTagName('textarea')[0].selectionEnd == 0;

Originally we archive that in deleted lines in setValueCommon, or
if (document().focusedElement() == this){...};
However it is not correct, you know, the problem #2.

[tkent, 2014-07-22 11:04:12]

On 2014/07/22 10:15:31, yoichio wrote:
> On 2014/07/22 09:50:08, tkent wrote:
> > On 2014/07/22 08:34:14, yoichio wrote:
> > > I know that this CL still leave some un-standard behaviors.
> > > However I'm working for a textarea performance issue and this CL is
needed.
> > > Thus let me leave the problem about assigning same value.
> > > 
> > > I suggest to file a bug and add FIXME to HTMLTextAreaElement.cpp:L361.
> > 
> > It's acceptable.  However I have a concern with this issue because moving
> > setSelectionRange() call out of setValueCommon needs more work. 
> setValueCommon
> > is called in setNonDirtyValue, and setNonDirtyValue is called in
> > childrenChanged, resetImpl, and setDefaultValue. Don't you need to call
> > setSelectionRange in these cases?
> 
> I guess all these functions are called for 'initialization' static HTML tree
> like
> <body><textarea>text</textarea></body>
> 
> What I like to do through moving setSelectionRange out from setValueCommon is 
> to call setSelectionRange(text.length(), text.length()) only when a value of
> textarea 
> is dynamically changed.
> That is because in the given html, following should be:
> document.getElementsByTagName('textarea')[0].selectionStart == 0;
> document.getElementsByTagName('textarea')[0].selectionEnd == 0;
> 
> Originally we archive that in deleted lines in setValueCommon, or
> if (document().focusedElement() == this){...};
> However it is not correct, you know, the problem #2.

All of them can be used to update editing text dynamically.
We need to investigate compatibility for each of code paths.

[yoichio, 2014-07-23 02:16:13]

Use the (renderer() && valid()) statement instead of
(document().focusedElement() == this)
to call setSelectionRange regardless of fucusing but not to call in
initialization.

[tkent, 2014-07-23 02:25:24]

https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:374: if (renderer() && valid()) {
I don't understand this code at all.  Why valid() is involved?

[yoichio, 2014-07-23 10:38:58]

In Firefox:
selectionStart/End are initialized 0 even if a textarea has a default value.
All of setting new value, form.reset(), updating textarea.defaultValue and
textarea.appendChild()
set the selection to end of text.

In IE:
selectionStart/End are initialized 0 even if a textarea has default value.
Setting new value and form.reset is reset both selections to 0.
Both of updating textarea.defaultValue and textarea.appendChild() preserve the
selection.

I added test cases to textarea-selection-preservation.html following the Firefox
behavior.

[yoichio, 2014-07-23 10:39:07]

https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:374: if (renderer() && valid()) {
On 2014/07/23 02:25:24, tkent wrote:
> I don't understand this code at all.  Why valid() is involved?
Move validity check to setSelectionRange.

HTMLTextFormControlElement::setSelectionRange calls layout.
It suppose textform's validity is updated.
textarea-live-pseudo-selectors tests it in L71.
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/resources/t...

[tkent, 2014-07-24 00:58:27]

On 2014/07/23 10:38:58, yoichio wrote:
> In Firefox:
> selectionStart/End are initialized 0 even if a textarea has a default value.
> All of setting new value, form.reset(), updating textarea.defaultValue and
> textarea.appendChild()
> set the selection to end of text.
> 
> In IE:
> selectionStart/End are initialized 0 even if a textarea has default value.
> Setting new value and form.reset is reset both selections to 0.
> Both of updating textarea.defaultValue and textarea.appendChild() preserve the
> selection.
> 
> I added test cases to textarea-selection-preservation.html following the
Firefox
> behavior.

Thank you for the investigation.  I agree that we should follow the Firefox
behavior.  It is simple and reasonable.

[tkent, 2014-07-24 01:04:22]

https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/392573002/diff/280001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:374: if (renderer() && valid()) {
On 2014/07/23 10:39:07, yoichio wrote:
> On 2014/07/23 02:25:24, tkent wrote:
> > I don't understand this code at all.  Why valid() is involved?
> Move validity check to setSelectionRange.
> 
> HTMLTextFormControlElement::setSelectionRange calls layout.
> It suppose textform's validity is updated.
> textarea-live-pseudo-selectors tests it in L71.
>
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/forms/resources/t...

ok, I understand that setNeedsValidityCheck() in setValue is too late.  Probably
we should move it to setValueCommon.  Putting it to setSelectionRange looks
weird.

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:224: setSelectionRange(endOfString,
endOfString, SelectionHasNoDirection, NotChangeSelection);
Why only resetImpl() has this though childrenChanged and setDefaultValue don't?

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:376: if (renderer()) {
Checking renderer() is not correct.  We should call setSelectionRange even if
the element has display:none.  We should check isFinishedParsingChildren().

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:189:
RefPtrWillBeRawPtr<HTMLTextFormControlElement> protector(this);
This should be moved to the outside of the |if|.

[yoichio, 2014-07-24 03:06:07]

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:224: setSelectionRange(endOfString,
endOfString, SelectionHasNoDirection, NotChangeSelection);
On 2014/07/24 01:04:22, tkent wrote:
> Why only resetImpl() has this though childrenChanged and setDefaultValue
don't?
I found also childrenChanged and setDefaultValue needs set selection to end even
if resetting a same value.
Thus I add extra setSelectionRange calling into the early return point in
setValueCommon.

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:376: if (renderer()) {
On 2014/07/24 01:04:22, tkent wrote:
> Checking renderer() is not correct.  We should call setSelectionRange even if
> the element has display:none.  We should check isFinishedParsingChildren().

Done.

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextFormControlElement.cpp (right):

https://codereview.chromium.org/392573002/diff/300001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextFormControlElement.cpp:189:
RefPtrWillBeRawPtr<HTMLTextFormControlElement> protector(this);
On 2014/07/24 01:04:22, tkent wrote:
> This should be moved to the outside of the |if|.

Done.

https://codereview.chromium.org/392573002/diff/320001/Source/core/html/HTMLTe...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/392573002/diff/320001/Source/core/html/HTMLTe...
Source/core/html/HTMLTextAreaElement.cpp:382: setNeedsValidityCheck();
Moved from setSelectionRange, setValue and setNonDirtyValue.

[tkent, 2014-07-24 04:31:27]

Looks good, but some tests are failing.

[yoichio, 2014-07-24 06:34:48]

On 2014/07/24 04:31:27, tkent wrote:
> Looks good, but some tests are failing.

Fixed.
virtual/softwarecompositing/repaint/invalidations-on-composited-layers.html
remains but it does not contain textareas and looks flaky.

[tkent, 2014-07-24 06:36:38]

lgtm

[yoichio, 2014-07-24 06:37:18]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2014-07-24 06:38:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/392573002/340001

[commit-bot: I haz the power, 2014-07-24 07:23:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18663)

[commit-bot: I haz the power, 2014-07-24 08:06:37]

Change committed as 178822