Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2903833002: Reland: Update TextSelection for non-user initiated events

Created:
6 months ago by Peter Varga
Modified:
4 months, 1 week ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su, mac-reviews_chromium.org, aelias_OOO_until_Jul13
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Suppress superfluous non-user initiated text selection events #

Total comments: 9

Patch Set 3 : Add test for JS cursor movement #

Unified diffs Side-by-side diffs Delta from patch set Stats (+506 lines, -101 lines) Patch
M AUTHORS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc View 1 2 4 chunks +330 lines, -16 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.cc View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 1 chunk +6 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 4 chunks +4 lines, -4 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.cc View 1 2 1 chunk +6 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac_unittest.mm View 1 2 4 chunks +6 lines, -6 lines 0 comments Download
M content/browser/renderer_host/text_input_manager.h View 1 3 chunks +10 lines, -4 lines 0 comments Download
M content/browser/renderer_host/text_input_manager.cc View 1 2 3 chunks +7 lines, -5 lines 0 comments Download
M content/common/frame_messages.h View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M content/public/renderer/render_frame.h View 1 1 chunk +0 lines, -5 lines 0 comments Download
M content/public/test/text_input_test_utils.h View 1 2 2 chunks +10 lines, -0 lines 0 comments Download
M content/public/test/text_input_test_utils.cc View 1 2 2 chunks +25 lines, -0 lines 0 comments Download
M content/renderer/input/frame_input_handler_impl.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/input/render_widget_input_handler.h View 1 2 chunks +11 lines, -0 lines 0 comments Download
M content/renderer/input/render_widget_input_handler.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/pepper/pepper_plugin_instance_impl.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 chunks +7 lines, -4 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 8 chunks +59 lines, -37 lines 0 comments Download
M content/renderer/render_widget.cc View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
Trybot results:

Messages

Total messages: 11 (3 generated)
Peter Varga
Changwan is right (last comment of https://codereview.chromium.org/2856093003/), there are more selection changes due to this ...
6 months ago (2017-05-24 12:40:42 UTC) #2
Changwan Ryu
not lgtm for now because of the following reasons: 1) I don't think it's generally ...
6 months ago (2017-05-24 16:02:19 UTC) #3
Peter Varga
I have updated the patch to suppress the superfluous events that most probably made the ...
4 months, 4 weeks ago (2017-06-29 12:05:59 UTC) #5
Peter Varga
ping?
4 months, 1 week ago (2017-07-14 12:49:26 UTC) #6
Changwan Ryu
https://codereview.chromium.org/2903833002/diff/20001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2903833002/diff/20001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2392 content/browser/renderer_host/render_widget_host_view_aura.cc:2392: if (selection->selected_text().length() && selection->user_initiated()) { This CL does more ...
4 months, 1 week ago (2017-07-14 18:44:03 UTC) #7
EhsanK
https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render_frame_impl.cc#newcode4107 content/renderer/render_frame_impl.cc:4107: if (GetRenderWidget()->input_handler().ime_composition_replacement()) > Is there any way we can ...
4 months, 1 week ago (2017-07-14 18:55:21 UTC) #8
Changwan Ryu
On 2017/07/14 18:55:21, EhsanK wrote: > https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render_frame_impl.cc > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2903833002/diff/20001/content/renderer/render_frame_impl.cc#newcode4107 > ...
4 months, 1 week ago (2017-07-14 21:00:47 UTC) #9
Peter Varga
4 months, 1 week ago (2017-07-17 09:36:06 UTC) #10
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?

Powered by Google App Engine
This is Rietveld efc10ee0f