Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(400)

Issue 2370663002: Remove logic to reset input method more than needed (Closed)

Created:
4 years, 2 months ago by Changwan Ryu
Modified:
3 years, 10 months ago
CC:
blink-reviews, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, kinuko+watch, kochi, kojii, Seigo Nonaka, nona+watch_chromium.org, piman+watch_chromium.org, shuchen+watch_chromium.org, sievers+watch_chromium.org, James Su, yabinh, yusukes+watch_chromium.org, amaralp
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove logic to reset input method more than needed The current logic is to reset input method whenever selection is outside the composition. For example, if you touch outside the composition, or JavaScript removes the composition and text preceding it, then the composition will be finished and then browser will reset input method. This logic was originally added to fix an Android bug (http://crbug.com/164427), but the bug is not reproducible any more without the fix. Also, the comment http://crbug.com/164427#c16 mentions another Linux bug, but the current logic is a half-baked solution. It fixes this case: http://jsfiddle.net/eEpQz/2/ but it doesn't fix this case: https://jsfiddle.net/galmacky/vct0Lk5y/ On Android, this reset causes unnecessary delay, and keyboard app loses the context and ceases to produce suggestions. Therefore, I think it makes sense to remove the logic to simplify the overall flow. Although it is not explicitly stated in the Android spec, Android's input method apps should finish composition when selection changes unexpectedly, therefore this case should be covered by the IME apps. I have manually tested against numerous apps, and could not find a case where it does not do it. Android's sample input method covers it as well. The changes here include: 1) InputMethodController not to handle selection change notification - not finish composition nor restart input. Now the keyboard app should handle this case. 2) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. Note that this does not fix the general problem of editor-keyboard conflict in any way, but we didn't see a case where the issue worsens with this change. BUG=630137 Review-Url: https://codereview.chromium.org/2370663002 Cr-Commit-Position: refs/heads/master@{#448924} Committed: https://chromium.googlesource.com/chromium/src/+/50a098f28262dca3de4fa9ff3b26f7a14f5d582a

Patch Set 1 #

Total comments: 3

Patch Set 2 : remove aura change #

Total comments: 6

Patch Set 3 : remove didCancelCompositionOnSelectionChange #

Patch Set 4 : rebased #

Patch Set 5 : revived after 3 months... #

Patch Set 6 : fix test, fix IMC #

Total comments: 3

Patch Set 7 : rebased #

Patch Set 8 : fixed handle issue in another CL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -40 lines) Patch
M content/public/android/java/src/org/chromium/content/browser/input/Range.java View 1 2 3 4 5 2 chunks +5 lines, -1 line 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java View 1 2 3 4 5 6 7 4 chunks +57 lines, -1 line 0 comments Download
M content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java View 1 2 3 4 5 2 chunks +18 lines, -1 line 0 comments Download
M content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java View 1 2 3 4 2 chunks +9 lines, -0 lines 0 comments Download
M content/renderer/render_view_impl.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/editing/Editor.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/FrameSelection.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/editing/InputMethodController.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/InputMethodController.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/core/page/ChromeClient.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/ChromeClientImpl.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/ChromeClientImpl.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebViewTest.cpp View 1 2 3 4 5 6 7 2 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/public/web/WebViewClient.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 85 (58 generated)
Changwan Ryu
Alex, although this is still a work in progress, the idea here is to avoid ...
4 years, 2 months ago (2016-09-26 03:23:38 UTC) #4
aelias_OOO_until_Jul13
This seems like a good direction. I haven't made up my mind about the nesting ...
4 years, 2 months ago (2016-09-26 21:25:59 UTC) #15
Changwan Ryu
On 2016/09/26 21:25:59, aelias wrote: > This seems like a good direction. > > I ...
4 years, 2 months ago (2016-09-27 02:56:48 UTC) #16
Changwan Ryu
https://codereview.chromium.org/2370663002/diff/1/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/2370663002/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2984 content/browser/renderer_host/render_widget_host_view_aura.cc:2984: if (has_composition_text_ && state->composition_start == -1) { On 2016/09/26 ...
4 years, 2 months ago (2016-09-27 03:00:12 UTC) #18
aelias_OOO_until_Jul13
https://codereview.chromium.org/2370663002/diff/1/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/2370663002/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2984 content/browser/renderer_host/render_widget_host_view_aura.cc:2984: if (has_composition_text_ && state->composition_start == -1) { On 2016/09/27 ...
4 years, 2 months ago (2016-09-27 04:39:47 UTC) #22
Changwan Ryu
On 2016/09/27 04:39:47, aelias wrote: > https://codereview.chromium.org/2370663002/diff/1/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/2370663002/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2984 > ...
4 years, 2 months ago (2016-09-27 05:52:26 UTC) #26
aelias_OOO_until_Jul13
lgtm, adding yosin@ for core/editing OWNERS. (FYI, I recently gained OWNERS over the Java directories ...
4 years, 2 months ago (2016-09-27 18:24:32 UTC) #30
yosin_UTC9
I could not find changes for (2) "InputMethodController to have up-to-date hasComposition() when there is ...
4 years, 2 months ago (2016-09-28 03:38:50 UTC) #31
Changwan Ryu
The current implementation of hasComposition() is to check m_composition, but it may not be up-to-date ...
4 years, 2 months ago (2016-09-28 06:20:12 UTC) #32
yosin_UTC9
Talked offline with IME expoers. We should confirm this changes with - CHromeOS, MacOS, Windows ...
4 years, 2 months ago (2016-10-04 06:06:12 UTC) #37
Changwan Ryu
On 2016/10/04 06:06:12, Yosi_UTC9 wrote: > Talked offline with IME expoers. > We should confirm ...
3 years, 11 months ago (2017-01-16 08:45:58 UTC) #45
yosin_UTC9
On 2017/01/16 at 08:45:58, changwan wrote: > On 2016/10/04 06:06:12, Yosi_UTC9 wrote: > > Talked ...
3 years, 11 months ago (2017-01-16 09:50:58 UTC) #46
Changwan Ryu
bokan@chromium.org: Please review changes in ChromeClient.h
3 years, 11 months ago (2017-01-16 11:28:58 UTC) #50
bokan
rs lgtm
3 years, 11 months ago (2017-01-16 19:37:07 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2370663002/80001
3 years, 11 months ago (2017-01-18 00:14:59 UTC) #54
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/101810)
3 years, 11 months ago (2017-01-18 01:38:39 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2370663002/80001
3 years, 11 months ago (2017-01-18 02:49:00 UTC) #59
Changwan Ryu
On 2017/01/18 02:49:00, commit-bot: I haz the power wrote: > CQ is trying da patch. ...
3 years, 11 months ago (2017-01-19 07:46:47 UTC) #61
Changwan Ryu
https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode254 third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; yosin@, could you take another look at ...
3 years, 11 months ago (2017-01-19 09:13:39 UTC) #64
yosin_UTC9
Please rebase just in case. We should make FrameSelection::setSelection() to handle handle visibility by default ...
3 years, 11 months ago (2017-01-19 09:29:51 UTC) #67
Changwan Ryu
https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode254 third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; On 2017/01/19 09:29:51, Yosi_UTC9 wrote: > On ...
3 years, 11 months ago (2017-01-19 11:42:59 UTC) #68
yosin_UTC9
On 2017/01/19 at 11:42:59, changwan wrote: > https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode254 ...
3 years, 11 months ago (2017-01-20 05:26:27 UTC) #73
Changwan Ryu
On 2017/01/20 05:26:27, Yosi_UTC9 wrote: > On 2017/01/19 at 11:42:59, changwan wrote: > > > ...
3 years, 11 months ago (2017-01-20 07:34:56 UTC) #74
rlanday
This patch also fixes a bug with composition underlines being jumpy that I've been running ...
3 years, 11 months ago (2017-01-24 22:23:59 UTC) #75
Changwan Ryu
FYI, https://codereview.chromium.org/2646963002 fixed the selection handle issue mentioned in patch #6. Patch #8 is basically ...
3 years, 10 months ago (2017-02-08 05:43:56 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2370663002/140001
3 years, 10 months ago (2017-02-08 05:44:41 UTC) #82
commit-bot: I haz the power
3 years, 10 months ago (2017-02-08 06:54:46 UTC) #85
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/50a098f28262dca3de4fa9ff3b26...

Powered by Google App Engine
This is Rietveld 408576698