INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after blur.

INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

After this CL, selection values are cached in webkitEditableContentChanged event
handling code too.  It is called on every DOM mutation in text control editor.


BUG=714425
Review-Url: https://codereview.chromium.org/2878613002
Cr-Commit-Position: refs/heads/master@{#471290}
Committed: https://chromium.googlesource.com/chromium/src/+/e71aa45ab3098edca75725a658d3624f5ed47108

[tkent, 2017-05-11 07:19:57]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[tkent, 2017-05-11 07:20:18]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-11 07:20:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-05-11 09:07:39]

tkent@chromium.org changed reviewers:
+ yosin@chromium.org

[tkent, 2017-05-11 09:07:40]

yosin@, would you review this please?

[commit-bot: I haz the power, 2017-05-11 09:09:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 09:09:46]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-05-11 09:41:55]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-05-11 09:41:56]

+ xiaochengh@ and +yoichio@, for new name of
NotifyLayoutObjectOfSelectionChange()

https://codereview.chromium.org/2878613002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2878613002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:225:
NotifyLayoutObjectOfSelectionChange(
Could you rename NotifyLayoutObjectOfSelectionChange() to
NotifySelectionChangeToTextControl() or another?
NotifySetSelectionToTextControl() is better?

Since NotifyLayoutObjectOfSelectionChange() notifies to text control instead of
layout object.

Also, at glance, this change is hard to understand because a comment and newly
added function call doesn't
match.

WDYT?


void FrameSelection::NotifyLayoutObjectOfSelectionChange(
    EUserTriggered user_triggered) {
  TextControlElement* text_control =
      EnclosingTextControl(GetSelectionInDOMTree().Base());
  if (!text_control)
    return;
  text_control->SelectionChanged(user_triggered == kUserTriggered);
}

[Xiaocheng, 2017-05-11 18:37:18]

https://codereview.chromium.org/2878613002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2878613002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:225:
NotifyLayoutObjectOfSelectionChange(
On 2017/05/11 at 09:41:55, yosin_UTC9 wrote:
> Could you rename NotifyLayoutObjectOfSelectionChange() to
NotifySelectionChangeToTextControl() or another?
> NotifySetSelectionToTextControl() is better?
> 
> Since NotifyLayoutObjectOfSelectionChange() notifies to text control instead
of layout object.
> 
> Also, at glance, this change is hard to understand because a comment and newly
added function call doesn't
> match.
> 
> WDYT?
> 
> 
> void FrameSelection::NotifyLayoutObjectOfSelectionChange(
>     EUserTriggered user_triggered) {
>   TextControlElement* text_control =
>       EnclosingTextControl(GetSelectionInDOMTree().Base());
>   if (!text_control)
>     return;
>   text_control->SelectionChanged(user_triggered == kUserTriggered);
> }

+1 for renaming.

[tkent, 2017-05-12 00:46:35]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 00:47:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-05-12 01:23:21]

ok, patch set 2 renames it to NotifyTextControlOfSelectionChange().

[tkent, 2017-05-12 01:23:49]

Description was changed from

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

We should cache them on blur.

BUG=714425
==========

to

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.


BUG=714425
==========

[tkent, 2017-05-12 01:25:09]

Description was changed from

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.


BUG=714425
==========

to

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

This CL adds NotifyTextControlOfSelectionChange(), which is renamed from
NotifyLayoutObjectOfSelectionChange(), call even if SelectionInDOMTree is not
changed.

BUG=714425
==========

[yosin_UTC9, 2017-05-12 01:37:00]

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:225:
NotifyTextControlOfSelectionChange(
Can we update cached selection in "webkitEditableContentChanged" event handler
in
TextControlElement::DefaultEventHandler()?

void TextControlElement::DefaultEventHandler(Event* event) {
  if (event->type() == EventTypeNames::webkitEditableContentChanged &&
      GetLayoutObject() && GetLayoutObject()->IsTextControl()) {
    last_change_was_user_edit_ = !GetDocument().IsRunningExecCommand();
    SubtreeHasChanged();
    return;
  }
  ...

It seems SubtreeHasChange() is the right place to sync cached selection.

[commit-bot: I haz the power, 2017-05-12 01:48:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 01:48:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoichio, 2017-05-12 01:54:00]

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html
(right):

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html:17:
assert_equals(element.selectionStart, null);
It doesn't make sense of "selectionStart/End on button element".
Is that speced or interop?

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:224: // changed.
Could you explain why?

[tkent, 2017-05-12 01:55:21]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 01:56:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-05-12 02:01:01]

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:224: // changed.
On 2017/05/12 at 01:54:00, yoichio wrote:
> Could you explain why?

e.g.

DIV id=inner-editor
  #text "line-1\n"
  #text "line-2\n"

The caret is on offset=0 of the second Text node, and type Backspace will make
the DOM:

DIV id=inner-editor
  #text "line-1"
  #text "line-2\n"

but the caret position is still offset=0 of the second Text node.

Patch Set 3 has a comment on this.

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:225:
NotifyTextControlOfSelectionChange(
On 2017/05/12 at 01:37:00, yosin_UTC9 wrote:
> Can we update cached selection in "webkitEditableContentChanged" event handler
in
> TextControlElement::DefaultEventHandler()?
> 
> void TextControlElement::DefaultEventHandler(Event* event) {
>   if (event->type() == EventTypeNames::webkitEditableContentChanged &&
>       GetLayoutObject() && GetLayoutObject()->IsTextControl()) {
>     last_change_was_user_edit_ = !GetDocument().IsRunningExecCommand();
>     SubtreeHasChanged();
>     return;
>   }
>   ...
> 
> It seems SubtreeHasChange() is the right place to sync cached selection.

Patch Set 3 follows it.

Yes, handling in TextControl is better than adding notification to the
no-selection-change path.

[tkent, 2017-05-12 02:01:46]

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html
(right):

https://codereview.chromium.org/2878613002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html:17:
assert_equals(element.selectionStart, null);
On 2017/05/12 at 01:54:00, yoichio wrote:
> It doesn't make sense of "selectionStart/End on button element".
> Is that speced or interop?

It's a spec'ed behavior.

[yosin_UTC9, 2017-05-12 04:03:52]

lgtm w/ nits

Please update summary and description. It seems they describe PS#1 and PS#2
instead of PS#3

https://codereview.chromium.org/2878613002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html
(right):

https://codereview.chromium.org/2878613002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html:11:
var element = document.querySelector('input');
nit: Please add: |assert_exists(window, 'eventSender');|

https://codereview.chromium.org/2878613002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html:23:
setTimeout(t.step_func_done(() => {
nit: How about using t.step_timeout()[1]?

[1] "Timeouts in Tests"
http://web-platform-tests.org/writing-tests/testharness-api.html

[tkent, 2017-05-12 05:03:45]

Description was changed from

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

This CL adds NotifyTextControlOfSelectionChange(), which is renamed from
NotifyLayoutObjectOfSelectionChange(), call even if SelectionInDOMTree is not
changed.

BUG=714425
==========

to

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

After this CL, selection values are cached in webkitEditableContentChanged event
handling code too.  It is called on every DOM mutation in text control editor.


BUG=714425
==========

[tkent, 2017-05-12 05:06:39]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 05:06:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 06:03:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 06:03:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tkent, 2017-05-12 08:41:07]

> Please update summary and description. It seems they describe PS#1 and PS#2
instead of PS#3

Done.

https://codereview.chromium.org/2878613002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html
(right):

https://codereview.chromium.org/2878613002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html:11:
var element = document.querySelector('input');
On 2017/05/12 at 04:03:52, yosin_UTC9 wrote:
> nit: Please add: |assert_exists(window, 'eventSender');|

Done.

https://codereview.chromium.org/2878613002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/text/text-selection-after-type-change.html:23:
setTimeout(t.step_func_done(() => {
On 2017/05/12 at 04:03:52, yosin_UTC9 wrote:
> nit: How about using t.step_timeout()[1]?
> 
> [1] "Timeouts in Tests"
http://web-platform-tests.org/writing-tests/testharness-api.html

Done.

[tkent, 2017-05-12 08:41:13]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-05-12 08:41:14]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2878613002/#ps80001
(title: "Improve a test")

[commit-bot: I haz the power, 2017-05-12 08:41:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 09:51:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 09:51:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tkent, 2017-05-12 10:11:49]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-05-12 10:12:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 11:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 11:09:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tkent, 2017-05-12 12:55:03]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-05-12 12:55:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 12:59:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494593703980720,
"parent_rev": "545f7be4401875ef9842de21144da3561281b2db", "commit_rev":
"e71aa45ab3098edca75725a658d3624f5ed47108"}

[commit-bot: I haz the power, 2017-05-12 12:59:41]

Description was changed from

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

After this CL, selection values are cached in webkitEditableContentChanged event
handling code too.  It is called on every DOM mutation in text control editor.


BUG=714425
==========

to

==========
INPUT/TEXTAREA elements: Fix incorrect selectionStart/selectionEnd values after
blur.

We failed to update selectionStart/selectionEnd cache because
TextControlElement::SelectionChanged() is not called in some cases after
crrev.com/450370.

After this CL, selection values are cached in webkitEditableContentChanged event
handling code too.  It is called on every DOM mutation in text control editor.


BUG=714425

Review-Url: https://codereview.chromium.org/2878613002
Cr-Commit-Position: refs/heads/master@{#471290}
Committed:
https://chromium.googlesource.com/chromium/src/+/e71aa45ab3098edca75725a658d3...
==========

[commit-bot: I haz the power, 2017-05-12 12:59:43]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e71aa45ab3098edca75725a658d3...