|
|
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. |
DescriptionRemove 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 #Messages
Total messages: 85 (58 generated)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
changwan@chromium.org changed reviewers: + aelias@chromium.org
Alex, although this is still a work in progress, the idea here is to avoid input restarting behavior. Although I haven't thoroughly checked, there is a room for unresolvable conflicts, so I think IME event nesting should be addressed as well. What do you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This seems like a good direction. I haven't made up my mind about the nesting issue. First of all, isn't the nesting problem preexisting/orthogonal? I can't see what part of your patch would break the Enter-absorbed scenario you mention. https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2984: if (has_composition_text_ && state->composition_start == -1) { Code in RenderWidgetHostViewAura only executes on Windows+Linux+ChromeOS, but http://crbug.com/164427 is about Clank. In general, I think code like this will be needed, before we add it, we should have a test app that this fixes in order to sanity check that we're doing the right thing (I assume you found google.com does not repro http://crbug.com/164427 anymore nowadays even with the other deletions?) If we cannot find a real-world example right now, we could try making an artificial one, and/or land the Blink side of the change on dev channel and see what breakage is reported to us.
On 2016/09/26 21:25:59, aelias wrote: > This seems like a good direction. > > I haven't made up my mind about the nesting issue. First of all, isn't the > nesting problem preexisting/orthogonal? I can't see what part of your patch > would break the Enter-absorbed scenario you mention. Hmm... Actually nesting issue is mostly orthogonal to this issue. I didn't have to mention it here. In an extreme corner case, something like the following may happen, but I don't think we need to worry. 1) IME composition causes selection to move forward by one character. 2) JS listens to composition event and moves the cursor backward by one character. 1) and 2) are nested so we make no selection change. If we type continuously then neither selection and composition changes. 3) IME app waits for composition change forever or resend composition change. An example with more generic issue with nesting would be something like the following https://jsfiddle.net/galmacky/627f9p2k/3/ but it is out of scope of this CL. (FYI Google keyboard handles this correctly.) Let me update the CL description.
Description was changed from ========== [WIP] Let the keyboard app handle selection changes Although it is not explicitly stated in the spec, Android input methods apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. However, there may be a case where such a conflict cannot be resolved. For example, if typing an enter key caused JS event listener to revert the change, then there will be no selection update at all, and the keyboard app will be confused. Maybe we should remove IME event nesting or just expect keyboard app to restart after timeout in this case. (If we are to remove IME event nesting this should be sumitted after ImeThread is fully launched.) 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427 that the previous logic tried to fix.) BUG=630137 ========== to ========== Let the keyboard app handle selection changes Although it is not explicitly stated in the spec, Android input methods apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) BUG=630137 ==========
https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2984: if (has_composition_text_ && state->composition_start == -1) { On 2016/09/26 21:25:59, aelias wrote: > Code in RenderWidgetHostViewAura only executes on Windows+Linux+ChromeOS, but > http://crbug.com/164427 is about Clank. http://crbug.com/164427#c16 was the reason why it was originally implemented through cancelComposition(). I can still reproduce the issue if I remove this logic. > > In general, I think code like this will be needed, before we add it, we should > have a test app that this fixes in order to sanity check that we're doing the > right thing (I assume you found http://google.com does not repro > http://crbug.com/164427 anymore nowadays even with the other deletions?) You're right. It doesn't repro any more. As for the other issue, I think a unit test should be enough. > > If we cannot find a real-world example right now, we could try making an > artificial one, and/or land the Blink side of the change on dev channel and see > what breakage is reported to us.
Description was changed from ========== Let the keyboard app handle selection changes Although it is not explicitly stated in the spec, Android input methods apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) BUG=630137 ========== to ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input methods apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) BUG=630137 ==========
Description was changed from ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input methods apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) BUG=630137 ========== to ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input method apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. Android's SoftKeyboard sample app has such logic as well: http://androidxref.com/6.0.1_r10/xref/development/samples /SoftKeyboard/src/com/example/android/softkeyboard/ SoftKeyboard.java But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) BUG=630137 ==========
Description was changed from ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input method apps should finish composition when selection changes unexpectedly. Otherwise, even on EditText, composition will still be there when a user moves the caret to another paragraph, which is a bad user experience. Basically, all of it is up to keyboard app's discretion. Android's SoftKeyboard sample app has such logic as well: http://androidxref.com/6.0.1_r10/xref/development/samples /SoftKeyboard/src/com/example/android/softkeyboard/ SoftKeyboard.java But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we expect IME apps detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) BUG=630137 ========== to ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input method apps should finish composition when selection changes unexpectedly. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we can expect IME apps to detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) 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 ==========
https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2984: if (has_composition_text_ && state->composition_start == -1) { On 2016/09/27 at 03:00:12, Changwan Ryu wrote: > http://crbug.com/164427#c16 > was the reason why it was originally implemented through cancelComposition(). I can still reproduce the issue if I remove this logic. OK, understood, but the history (including the linked WebKit bug) shows this is a totally artificial test case, and there's no evidence that any real-world desktop JS forms rely on this. In fact, the evidence cuts the other way because the issue was preexisting before Aurimas noticed it while investigating an Android issue. Therefore I would prefer us not to go out of our way to add extra lines of code to address this possible non-bug. If there's a real issue here, we'll hear about it in dev/beta channel and can always add these lines then, with new confidence that they're useful.
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input method apps should finish composition when selection changes unexpectedly. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we can expect IME apps to detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. 4) Fix RenderWidgetHostViewAura to handle JS-triggered composition loss (This handles crbug.com/164427#c16 that the previous logic was fixing.) 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 ========== to ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input method apps should finish composition when selection changes unexpectedly. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we can expect IME apps to detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. In doing so, we decided not to propagate composition cancellation to Aura input methods (crbug.com/164427#c16) as the test case seems too artificial. 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 ==========
On 2016/09/27 04:39:47, aelias wrote: > https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/2370663002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_aura.cc:2984: if > (has_composition_text_ && state->composition_start == -1) { > On 2016/09/27 at 03:00:12, Changwan Ryu wrote: > > http://crbug.com/164427#c16 > > was the reason why it was originally implemented through cancelComposition(). > I can still reproduce the issue if I remove this logic. > > OK, understood, but the history (including the linked WebKit bug) shows this is > a totally artificial test case, and there's no evidence that any real-world > desktop JS forms rely on this. In fact, the evidence cuts the other way because > the issue was preexisting before Aurimas noticed it while investigating an > Android issue. Therefore I would prefer us not to go out of our way to add > extra lines of code to address this possible non-bug. If there's a real issue > here, we'll hear about it in dev/beta channel and can always add these lines > then, with new confidence that they're useful. Sounds good. Removed now. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aelias@chromium.org changed reviewers: + yosin@chromium.org
lgtm, adding yosin@ for core/editing OWNERS. (FYI, I recently gained OWNERS over the Java directories in http://crrev.com/2365533002, so no further OWNERS are needed.)
I could not find changes for (2) "InputMethodController to have up-to-date hasComposition() when there is a change to the node" in both PS#1 and PS#2. Did you drop this change? I think this is the right way to do so since selection change notification doesn't cover all composition changes. I suggest to introduce ChromeClient::didChangeComposition() as replacement of ChromeClient::didCancelCompositionOnSelectionChange() to notify composition changes, including both offset changes and content changes, e.g. "abc" => "ABC". https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (left): https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:286: m_frame->inputMethodController().cancelCompositionIfSelectionIsInvalid(); Note: This catches replacing Text node with same length of data, e.g. "abc" => "ABC", "abc" => "aBc". But, this can't catch "<b>abc</b>def" => <b>abc</b>XYZdef"; inserting Text node "XYZ" between "<b>abc</b>" and "def". In other words, current approach of detecting contents of selection change isn't enough. https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:334: m_frame->inputMethodController().cancelCompositionIfSelectionIsInvalid(); Ditto as L286. https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (left): https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:338: frame().chromeClient().didCancelCompositionOnSelectionChange(); Could you add TODO comment in "ChromeClient.h" to get rid of |didCancelCompositionOnSelectionChange()|?
The current implementation of hasComposition() is to check m_composition, but it may not be up-to-date when composition has been removed by JS change. Now with this CL we actually check with the Range that is registered to the owner document, it gets updated whenever the node changes. That's what I mean by up-to-date. (Otherwise, when sendKeyEvent(KEYCODE_ENTER) removes the composition and textInputInfo() reports composition range as (0, 0), not (-1, -1) as expected. This caused test failures.) https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (left): https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:286: m_frame->inputMethodController().cancelCompositionIfSelectionIsInvalid(); On 2016/09/28 03:38:50, Yosi_UTC9 wrote: > Note: This catches replacing Text node with same length of data, e.g. "abc" => > "ABC", "abc" => "aBc". > But, this can't catch "<b>abc</b>def" => <b>abc</b>XYZdef"; inserting Text node > "XYZ" between "<b>abc</b>" and "def". > In other words, current approach of detecting contents of selection change isn't > enough. Hmm.. My understanding is that there is no JS way to change the composition content. Isn't it the case? (Even if it's not, this CL is still orthogonal to the composition content handling.) If the content of the composition cannot change, then there are two possible ways the composition range can change by JS: 1) part of composition gets removed 2) composition range moves e.g., by inserting text before it. In most cases the above composition change should accompany selection change. I could not find a single case where it doesn't. When selection changes, it will trigger UpdateTextInputState() and m_compositionRange should have the updated anchor nodes and offsets because the composition Range is registered to the owner document. If the above is correct, we can rely on UpdateTextInputState() logic to notify the composition range update to the browser process. What do you think? https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:334: m_frame->inputMethodController().cancelCompositionIfSelectionIsInvalid(); On 2016/09/28 03:38:50, Yosi_UTC9 wrote: > Ditto as L286. Explained in L286. https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (left): https://codereview.chromium.org/2370663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:338: frame().chromeClient().didCancelCompositionOnSelectionChange(); On 2016/09/28 03:38:50, Yosi_UTC9 wrote: > Could you add TODO comment in "ChromeClient.h" to get rid of > |didCancelCompositionOnSelectionChange()|? My bad, I just went ahead and removed it.
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Talked offline with IME expoers. We should confirm this changes with - CHromeOS, MacOS, Windows - CN, JP, KR Especially for hiding candidate window (CN, JP) when text composition is canceled/restartd.
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Let the keyboard app handle unexpected selection changes Although it is not explicitly stated in the spec, Android input method apps should finish composition when selection changes unexpectedly. But Chromium has been finishing composition and restarting input *whenever* selection changes, as a half-baked way to cancel composition from editor side. Android platform does not provide a way to cancel composition nor provide a way to notify editor-initiated changes. But we can expect IME apps to detect editor-initiated changes and restart input as necessary. 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. In doing so, we decided not to propagate composition cancellation to Aura input methods (crbug.com/164427#c16) as the test case seems too artificial. 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 ========== to ========== 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) 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 ==========
On 2016/10/04 06:06:12, Yosi_UTC9 wrote: > Talked offline with IME expoers. > We should confirm this changes with > - CHromeOS, MacOS, Windows > - CN, JP, KR > Especially for hiding candidate window (CN, JP) when text composition is > canceled/restartd. Apologies for the delay in resuming this work - I have been too busy with other work (mainly redesign change) and I also had to set up Windows machine in the Seoul Office. I realized that the description is somewhat misleading, so updated it. Also, I've tested against Windows, Linux, ChromeOS, CN, JP, KR, but could not find any issue. Candidate window still gets hidden for desktop if you touch outside composition because RenderWidgetHostViewEventHandler::OnMouseEvent calls TextInputManager::ImeCancelComposition directly. JS doesn't hide candidate window now, but I found that the current solution is half-baked anyways. As for the composition and DOM propagation, I don't have a good solution yet. But as my updated description points out, they are not affected by this CL. (Sorry if previous description was suggesting it.) The current state update logic is initiated by user gestures, IME operations, and selection updates only. I think composition and DOM initiated state updates matter, but hope they are perpendicular. Could you take another look?
On 2017/01/16 at 08:45:58, changwan wrote: > On 2016/10/04 06:06:12, Yosi_UTC9 wrote: > > Talked offline with IME expoers. > > We should confirm this changes with > > - CHromeOS, MacOS, Windows > > - CN, JP, KR > > Especially for hiding candidate window (CN, JP) when text composition is > > canceled/restartd. > > Apologies for the delay in resuming this work - I have been too busy with other work (mainly redesign change) > and I also had to set up Windows machine in the Seoul Office. > > I realized that the description is somewhat misleading, so updated it. > > Also, I've tested against Windows, Linux, ChromeOS, CN, JP, KR, but could not find any issue. > Candidate window still gets hidden for desktop if you touch outside composition because > RenderWidgetHostViewEventHandler::OnMouseEvent calls TextInputManager::ImeCancelComposition directly. > JS doesn't hide candidate window now, but I found that the current solution is half-baked anyways. > > As for the composition and DOM propagation, I don't have a good solution yet. But as my updated description points out, > they are not affected by this CL. (Sorry if previous description was suggesting it.) The current state update logic is initiated by user gestures, IME operations, and selection updates only. I think composition and DOM initiated state updates matter, but hope they are perpendicular. > > Could you take another look? lgtm We introduced SynchronousMutationObserver. So, it is easy to observer DOM mutation. We may want to utilize SMO in InputMethodController to cancel composition at DOM mutation. [1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
changwan@chromium.org changed reviewers: + bokan@chromium.org
bokan@chromium.org: Please review changes in ChromeClient.h
rs lgtm
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2370663002/#ps80001 (title: "revived after 3 months...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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) InputMethodController to have up-to-date hasComposition() when there is a change to the node, e.g., deletion of composition text by sending a key event. Previously we blindly finished composition and updated m_hasComposition. Now the keyboard app should get notified of the exact change before finishing composition, and this change helps update composition of -1, -1 when composition is lost as a result of text data modification. Range::did* callbacks update m_compositionRange, so m_compositionRange keeps track of such modifications. 3) 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 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by changwan@chromium.org
On 2017/01/18 02:49:00, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I have locally fixed the test, but I found that there is a regression. If you long press text while composing another text, the insertion handles get dismissed because of the following sequence: 1) change selection range and show insertion handles 2) keyboard app finishes composition 3) InputMethodController::finishComposingText() -> setSelectionOffsets() -> FrameSelection::setSelectionAlgorithm() This sets m_handleVisibility = false. 4) FrameView::updateCompositedSelectionIfNeeded() -> computeCompositedSelection() returns false and calls clearCompositedSelection() because handleVisible is false. But technically, selection hasn't changed a bit, so I think handles should remain showing. I'll try to find a fix.
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; yosin@, could you take another look at this part? Without this, insertion handles get dismissed when you compose and then long-press text.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Please rebase just in case. We should make FrameSelection::setSelection() to handle handle visibility by default rather than callers take care of it. It is error prone on today and future. https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; On 2017/01/19 at 09:13:39, Changwan Ryu wrote: > yosin@, could you take another look at this part? Without this, insertion handles get dismissed when you compose and then long-press text. This is yet another case |FrameSeleciton.isHandleVisible()| causes bug. We should change FrameSeleciton::setSelection() to have default to make handle visible. See https://crrev.com/2642913003 Select All should show handles if they were already present
https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; On 2017/01/19 09:29:51, Yosi_UTC9 wrote: > On 2017/01/19 at 09:13:39, Changwan Ryu wrote: > > yosin@, could you take another look at this part? Without this, insertion > handles get dismissed when you compose and then long-press text. > > This is yet another case |FrameSeleciton.isHandleVisible()| causes bug. > We should change FrameSeleciton::setSelection() to have default to make handle > visible. > > See https://crrev.com/2642913003 Select All should show handles if they were > already present Thanks for the pointer. I considered fixing FrameSelection::setSelectionAlgorithm() but I didn't because of the complexity of the alternative solution. In most cases we do want to dismiss handles when selection change occurs. This may be default value (options without HandleVisible) or more explicit option such as HandleVisibleIfSelectionDoesNotChange . Or maybe we should change HandleVisible to something like ShowHandles. But in the case of finishComposingText(), replaceComposition moves selection to one place, and setSelectionOffsets moves selection back to the original offsets. But we still don't want to dismiss handles. So we may need something like SelectionHandleScope such that handle visibility can be kept throughout the changes. In this sense, this may be something perpendicular to what Pedro is trying to solve. What do you think?
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/01/19 at 11:42:59, changwan wrote: > https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; > On 2017/01/19 09:29:51, Yosi_UTC9 wrote: > > On 2017/01/19 at 09:13:39, Changwan Ryu wrote: > > > yosin@, could you take another look at this part? Without this, insertion > > handles get dismissed when you compose and then long-press text. > > > > This is yet another case |FrameSeleciton.isHandleVisible()| causes bug. > > We should change FrameSeleciton::setSelection() to have default to make handle > > visible. > > > > See https://crrev.com/2642913003 Select All should show handles if they were > > already present > > Thanks for the pointer. > I considered fixing FrameSelection::setSelectionAlgorithm() but I didn't because of the complexity of the alternative solution. > > In most cases we do want to dismiss handles when selection change occurs. This may be default value (options without HandleVisible) or more explicit option such as HandleVisibleIfSelectionDoesNotChange . Or maybe we should change HandleVisible to something like ShowHandles. > > But in the case of finishComposingText(), replaceComposition moves selection to one place, and setSelectionOffsets moves selection back to the original offsets. But we still don't want to dismiss handles. > So we may need something like SelectionHandleScope such that handle visibility can be kept throughout the changes. In this sense, this may be something perpendicular to what Pedro is trying to solve. > > What do you think? Thanks for response. As discussed [1], once we make SelectionTemplate has handle visibility, specifying handle visibility becomes easier, simpler and unified. And, we can track handle visibility easier. SelectionHandleScope is good alternative but we need to have yet another state in FrameSeleciton. Since we want to reduce number of state, it makes code complex. [1] http://crbug.com/633281 Refactor Selection Handle Visibility
On 2017/01/20 05:26:27, Yosi_UTC9 wrote: > On 2017/01/19 at 11:42:59, changwan wrote: > > > https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): > > > > > https://codereview.chromium.org/2370663002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: : 0; > > On 2017/01/19 09:29:51, Yosi_UTC9 wrote: > > > On 2017/01/19 at 09:13:39, Changwan Ryu wrote: > > > > yosin@, could you take another look at this part? Without this, insertion > > > handles get dismissed when you compose and then long-press text. > > > > > > This is yet another case |FrameSeleciton.isHandleVisible()| causes bug. > > > We should change FrameSeleciton::setSelection() to have default to make > handle > > > visible. > > > > > > See https://crrev.com/2642913003 Select All should show handles if they were > > > already present > > > > Thanks for the pointer. > > I considered fixing FrameSelection::setSelectionAlgorithm() but I didn't > because of the complexity of the alternative solution. > > > > In most cases we do want to dismiss handles when selection change occurs. This > may be default value (options without HandleVisible) or more explicit option > such as HandleVisibleIfSelectionDoesNotChange . Or maybe we should change > HandleVisible to something like ShowHandles. > > > > But in the case of finishComposingText(), replaceComposition moves selection > to one place, and setSelectionOffsets moves selection back to the original > offsets. But we still don't want to dismiss handles. > > So we may need something like SelectionHandleScope such that handle visibility > can be kept throughout the changes. In this sense, this may be something > perpendicular to what Pedro is trying to solve. > > > > What do you think? > > Thanks for response. > > As discussed [1], once we make SelectionTemplate has handle visibility, > specifying handle visibility becomes easier, simpler and unified. And, we can > track handle visibility easier. > > SelectionHandleScope is good alternative but we need to have yet another state > in FrameSeleciton. Since we want to reduce number of state, it makes code > complex. > > [1] http://crbug.com/633281 Refactor Selection Handle Visibility Uploaded a separate CL (WIP). yosin@, is this aligned with what you suggested? https://codereview.chromium.org/2646963002/
This patch also fixes a bug with composition underlines being jumpy that I've been running into with my work to support SuggestionSpans: http://crbug.com/683387
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
FYI, https://codereview.chromium.org/2646963002 fixed the selection handle issue mentioned in patch #6. Patch #8 is basically a rebase of patch #5, which I got necessary lgtms on. I'll go ahead and merge this change.
The CQ bit was unchecked by changwan@chromium.org
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, aelias@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2370663002/#ps140001 (title: "fixed handle issue in another CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486532659665330, "parent_rev": "2906e0c1298431ea3cdb9fb3a8da8046d7c9e6bb", "commit_rev": "50a098f28262dca3de4fa9ff3b26f7a14f5d582a"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/50a098f28262dca3de4fa9ff3b26... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/50a098f28262dca3de4fa9ff3b26... |