Reland: Update TextSelection for non-user initiated events

Reland: Update TextSelection for non-user initiated events

This makes Chromium Content API to be able to notify about text
selection changes triggered by non-user events. The source of the
selection event is held by |TextSelection.user_initiated| and this
information is propagated from renderers to the browser process.

Reason for reland:
Non-user initiated text selection events are suppressed for the
following corner cases:
- Ignore subsequent empty non-user initiated text selection events.
- Ignore text selection events during replacement by IME composition.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
> --gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed: https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb7afe71b7bca4

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromium.org,changwan@chromium.org
BUG=671986
TEST=
interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.TextSelectionOnFocusChange
interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.ImeSetCompositionTextReplacement
interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.JSCursorMovement
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Peter Varga, 2017-05-24 12:38:06]

Description was changed from

==========
Reland: Update TextSelection for non-user initiated events

Reason for reland:
Non-user initiated text selection events have been disabled in this
patch on Android until the flaky Android ImeTest is adapted.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
>
--gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed:
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromiu...
BUG=671986
TEST=interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
==========

to

==========
Reland: Update TextSelection for non-user initiated events

Reason for reland:
Non-user initiated text selection events have been disabled in this
patch on Android until the flaky Android ImeTest is adapted.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
>
--gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed:
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromiu...
BUG=671986
TEST=interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Peter Varga, 2017-05-24 12:40:42]

Changwan is right (last comment of https://codereview.chromium.org/2856093003/),
there are more selection changes due to this patch and this causes the Android
test failures.

These extra selection changes should be tested in the Android tests and possibly
check whether they are user initiated. Ignoring the non-user initiated events in
the tests would be an even simpler solution.

The problem with this is that the selection changes are tracked via the
TextInputState changes in Android tests. TextInputState doesn't hold the user
initiated information because I don't see any use case to store it there. (Only
use case that I've found for differentiate user and non-user initiated text
selection is the selection clipboard feature which is AFAIK is not available on
Android.)

Since I don't have capacity (and neither device to test) to adapt these Android
IME tests to this user initiated patch, I would rather disable it and skip the
SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection test on
Android. This would keep the original behavior on Android and possibly someone
else could adapt it later if there is a need.

I did that Android guarding in this reland patch. See 
content/renderer/render_frame_impl.cc and
chrome/browser/renderer_host/site_per_process_text_input_borwsertest.cc.
Is this an acceptable way to solve this issue?

[Changwan Ryu, 2017-05-24 16:02:19]

not lgtm for now because of the following reasons:

1) I don't think it's generally a good idea to fork call paths unless sufficient
justification is given.

2) Android's test coverage for end-to-end IME is quite high, if not the highest
among platforms. Therefore, I cannot tell at this point whether or not the
selection values are problematic only for Android or for all the platforms. We
may well be ignoring an important alert.

That said, I think the test failures were, in all the cases I observed, caused
by additional 0 selection value after the focus change.

Here are some theories I can think of regarding the test failures:

A. setUp() produced an additional selection update which did not get reset in
resetUpdateSelectionList(), and selection update from focusElement() in
focusElementAndWaitForStateUpdate() was seen in the test.

B. focus change produced an additional selection update, which did not get reset
in resetUpdateSelectionList(), was seen in the test.

I suspect that B seems to be the case because all of the failures I saw happened
after focusElementAndWaitForStateUpdate() call.

Then I think potential culprit would be selection update in the middle of focus
change that did not have the correct input type (TEXT_INPUT_TYPE_NONE), thus
bypassed all the checks in RenderWidget::UpdateTextInputState() and
onUpdateSelection() in ImeActivityTestRule.java.

If this is the case, I don't think we should land this as is because this type
of intermediate selection update is usually misleading.

[Peter Varga, 2017-06-29 12:02:28]

Description was changed from

==========
Reland: Update TextSelection for non-user initiated events

Reason for reland:
Non-user initiated text selection events have been disabled in this
patch on Android until the flaky Android ImeTest is adapted.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
>
--gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed:
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromiu...
BUG=671986
TEST=interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland: Update TextSelection for non-user initiated events

Reason for reland:
Non-user initiated text selection events are suppressed for the
following corner cases:
- Ignore subsequent empty non-user initiated text selection events.
- Ignore text selection events during replacement by IME composition.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
>
--gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed:
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromiu...
BUG=671986
TEST=
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.TextSelectionOnFocusChange
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.ImeSetCompositionTextReplacement
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Peter Varga, 2017-06-29 12:05:59]

I have updated the patch to suppress the superfluous events that most probably
made the android IME tests flaky. I've added tests for these cases for make it
possible to test them on desktop.

I tried ImeTest on Android and all passed for me with this fix.

[Peter Varga, 2017-07-14 12:49:26]

ping?

[Changwan Ryu, 2017-07-14 18:44:03]

https://codereview.chromium.org/2903833002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2903833002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2392: if
(selection->selected_text().length() && selection->user_initiated()) {
This CL does more than commit description as follows:
1) It propagates new information to the browser process.
2) We check the value to fix clipboard case.

Could you file a dependent crbug and update your description?

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:1423: // change as user initiated.
I'm not too familiar with the pepper and desktop use cases in general, but can't
this be caused both by IME change and by pepper's own commandline? Is this an
intermediate state or a final implementation?

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4103: if (is_empty_selection &&
selection_text_.empty())
What happens if move the cursor from one position to another from JS?
Could you add a test case to ensure that we still update text input state and
sync selection in that case?

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4107: if
(GetRenderWidget()->input_handler().ime_composition_replacement())
Hmm... This approach is quite brittle. SetComposingText() is not the only place
where intermediate selection changes can be made - see
InputMethodController.cpp. You will need to apply it to every IME operation.

When I come to think about it, it's quite unfortunate that Android's and
desktop's selection update paths are completely different.

On Android, ImeEventGuard + UpdateTextInputState() should already prevent
propagation of all the intermediate selection updates.

Is there any way we can remove SyncSelectionIfRequired() and merge it into
UpdateTextInputState path? Then we wouldn't need this sort of divergence at all.

Failing that, is it possible to call SyncSelectionIfRequired() inside
UpdateTextInputState()?

[EhsanK, 2017-07-14 18:55:21]

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4107: if
(GetRenderWidget()->input_handler().ime_composition_replacement())

> Is there any way we can remove SyncSelectionIfRequired() and merge it into
> UpdateTextInputState path? Then we wouldn't need this sort of divergence at
all.
> 

I think this is something worth pursing. The potential problems I see are:

1- SyncSelectionIfRequired is a frame concept while TextInputState is a
WebFrameWidget thing. So if a frame is not focused, it does not contribute to
TextInputState but it can still update its selection. This is something to
consider.

2- Minor issue but selection uses string16 whereas text input state uses
std::string. I am not sure if there is any complications to migrate one to
another.

3- Are TextInputState's selection values still relevant when we have highlighted
text in an non-editable region?

[Changwan Ryu, 2017-07-14 21:00:47]

On 2017/07/14 18:55:21, EhsanK wrote:
>
https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
> content/renderer/render_frame_impl.cc:4107: if
> (GetRenderWidget()->input_handler().ime_composition_replacement())
> 
> > Is there any way we can remove SyncSelectionIfRequired() and merge it into
> > UpdateTextInputState path? Then we wouldn't need this sort of divergence at
> all.
> > 
> 
> I think this is something worth pursing. The potential problems I see are:
> 
> 1- SyncSelectionIfRequired is a frame concept while TextInputState is a
> WebFrameWidget thing. So if a frame is not focused, it does not contribute to
> TextInputState but it can still update its selection. This is something to
> consider.
Good point. But what exactly is the distinction WebFrameWidget and WebWidget
with regard of focus?
I've asked about this on blink-dev, but I would appreciate it if you could
explain...

> 
> 2- Minor issue but selection uses string16 whereas text input state uses
> std::string. I am not sure if there is any complications to migrate one to
> another.
> 
> 3- Are TextInputState's selection values still relevant when we have
highlighted
> text in an non-editable region?

No, they are not. I think that's where the main complexity would come from if we
pursue it.

[Peter Varga, 2017-07-17 09:36:06]

https://codereview.chromium.org/2903833002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2903833002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2392: if
(selection->selected_text().length() && selection->user_initiated()) {
On 2017/07/14 18:44:03, Changwan Ryu wrote:
> This CL does more than commit description as follows:
> 1) It propagates new information to the browser process.
I thought the description is clear enough but I will update this.

> 2) We check the value to fix clipboard case.
This is not regression. It is already fixed and there is already a corresponding
crbug: https://bugs.chromium.org/p/chromium/issues/detail?id=12392
Actually, the fix for this issue suppresses all non-user initated selection
events that I would like to add back.

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:1423: // change as user initiated.
On 2017/07/14 18:44:03, Changwan Ryu wrote:
> I'm not too familiar with the pepper and desktop use cases in general, but
can't
> this be caused both by IME change and by pepper's own commandline? Is this an
> intermediate state or a final implementation?

I'm also not familiar with pepper, that's why I keep the original behavior here.

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4103: if (is_empty_selection &&
selection_text_.empty())
On 2017/07/14 18:44:03, Changwan Ryu wrote:
> What happens if move the cursor from one position to another from JS?
> Could you add a test case to ensure that we still update text input state and
> sync selection in that case?

I assume you mean to change cursor position by empty text selection, eg.
input.setSelectionRange(pos, pos). Makes sense, I will check this.

https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4107: if
(GetRenderWidget()->input_handler().ime_composition_replacement())
On 2017/07/14 18:44:03, Changwan Ryu wrote:
> Hmm... This approach is quite brittle. SetComposingText() is not the only
place
> where intermediate selection changes can be made - see
> InputMethodController.cpp. You will need to apply it to every IME operation.
I try to handle this in render_widget.cc. Only unnecessary intermediate
selection that I found so far is the replacement of a composing text. Other
cases are already handled as an user input event by
input_handler_->set_handling_input_event(true).

> When I come to think about it, it's quite unfortunate that Android's and
> desktop's selection update paths are completely different.
> 
> On Android, ImeEventGuard + UpdateTextInputState() should already prevent
> propagation of all the intermediate selection updates.
I agree with this. I tried to point to this in a comment when I did the first
reland patch with the android skip.
 
> Is there any way we can remove SyncSelectionIfRequired() and merge it into
> UpdateTextInputState path? Then we wouldn't need this sort of divergence at
all.
> 
> Failing that, is it possible to call SyncSelectionIfRequired() inside
> UpdateTextInputState()?
It might be possible but as far as I see UpdateTextInputState is not called
before SyncSelectionIfRequired in other cases (pepper, OnReplace). I assume that
in other cases UpdateTextInputState is called in another code path. Implementing
this would raise new risks that needs more investigation and testing. Currently
I don't have too much time for this. I consider the current solution less likely
to introduce regressions. Could it be possible to do this "refactor" in a
subsequent patch by someone else?

[Peter Varga, 2017-07-18 09:59:52]

Description was changed from

==========
Reland: Update TextSelection for non-user initiated events

Reason for reland:
Non-user initiated text selection events are suppressed for the
following corner cases:
- Ignore subsequent empty non-user initiated text selection events.
- Ignore text selection events during replacement by IME composition.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
>
--gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed:
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromiu...
BUG=671986
TEST=
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.TextSelectionOnFocusChange
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.ImeSetCompositionTextReplacement
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland: Update TextSelection for non-user initiated events

This makes Chromium Content API to be able to notify about text
selection changes triggered by non-user events. The source of the
selection event is held by |TextSelection.user_initiated| and this
information is propagated from renderers to the browser process.

Reason for reland:
Non-user initiated text selection events are suppressed for the
following corner cases:
- Ignore subsequent empty non-user initiated text selection events.
- Ignore text selection events during replacement by IME composition.

Original issue's description:
> Update TextSelection for non-user initiated events
>
> This makes Chromium Content API to be able to notify about text
> selection changes triggered by non-user events (eg. JavaScript, IME,
> autofill).
>
> R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org
> BUG=671986
> TEST=interactive_ui_tests
>
--gtest_filter=SitePerProcessTextInputManagerTest.NonUserInitiatedTextSelection
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2856093003
> Cr-Commit-Position: refs/heads/master@{#473834}
> Committed:
https://chromium.googlesource.com/chromium/src/+/d48794b9728276c1183b0c56f8cb...

R=creis@chromium.org,nasko@chromium.org,ekaramad@chromium.org,thestig@chromiu...
BUG=671986
TEST=
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.*UserInitiatedTextSelection
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.TextSelectionOnFocusChange
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.ImeSetCompositionTextReplacement
interactive_ui_tests
--gtest_filter=SitePerProcessTextInputManagerTest.JSCursorMovement
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========