Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2434433006:
MacViews: Implement InputMethodMac::CancelComposition. (Closed)
|
Created:
4 years, 2 months ago by karandeepb Modified:
4 years, 1 month ago CC:
chromium-reviews, nona+watch_chromium.org, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Implement InputMethodMac::CancelComposition.
Currently, clicking inside a Textfield with an active composition, commits the
composition text but does not close the IME candidate window. Hence, while
Views::Textfield has committed the composition, the Cocoa text system does not
know about it, leading to an incorrect state where the composition text may be
committed once more.
This CL implements InputMethodMac::CancelComposition which signals the Cocoa
text system to close the IME candidate window and discard the composition text.
BUG=652957
Committed: https://crrev.com/de65bcf491e13a0a7e0724e8732977561588072e
Cr-Commit-Position: refs/heads/master@{#427557}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address comments. #
Messages
Total messages: 20 (12 generated)
Description was changed from ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and commit the composition text. Ideally, BridgedContentView's mouseEvent: method should have passed the event to [NSTextInputContext handleEvent:], but we can't do it currently since it uses characterIndexForPoint: which the BridgedContentView does not currently implement. BUG=652957 ========== to ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. Ideally, BridgedContentView's mouseEvent: method should have passed the event to [NSTextInputContext handleEvent:], but we can't do it currently since it uses characterIndexForPoint: which the BridgedContentView does not currently implement. BUG=652957 ==========
The CQ bit was checked by karandeepb@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...
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. This still has two minor drawbacks- 1) The candidate window would still not be closed on a mouse click, when there is no marked text (Eg. - Long press A). 2) On a left mouse click with active composition text, the cursor moves to clicked location as opposed to at the end of the composed text (which is the Cocoa behavior.) These can be solved by directly using unmarkText and discardMarkedText within BCV's mouseEvent method. However, then we'll lose hit testing. (Clicking outside a textfield with an active composition text will also close the IME window which shouldn't happen). Also, we should do this anyways to ensure the Cocoa and Views text system are in sync.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/20 06:14:14, karandeepb wrote: > PTAL Trent. This still has two minor drawbacks- > > 1) The candidate window would still not be closed on a mouse click, when there > is no marked text (Eg. - Long press A). > 2) On a left mouse click with active composition text, the cursor moves to > clicked location as opposed to at the end of the composed text (which is the > Cocoa behavior.) > > These can be solved by directly using unmarkText and discardMarkedText within > BCV's mouseEvent method. However, then we'll lose hit testing. (Clicking outside > a textfield with an active composition text will also close the IME window which > shouldn't happen). > > Also, we should do this anyways to ensure the Cocoa and Views text system are in > sync. ping Trent.
sorry for the delay. lgtm, but an IME person should take a look too. https://codereview.chromium.org/2434433006/diff/1/ui/base/ime/input_method_ma... File ui/base/ime/input_method_mac.mm (right): https://codereview.chromium.org/2434433006/diff/1/ui/base/ime/input_method_ma... ui/base/ime/input_method_mac.mm:7: #include <Cocoa/Cocoa.h> nit: import https://codereview.chromium.org/2434433006/diff/1/ui/base/ime/input_method_ma... ui/base/ime/input_method_mac.mm:32: [[NSTextInputContext currentInputContext] discardMarkedText]; Do we need if (!IsTextInputClientFocused(client)) return; the way Linux and Windows do? They also do some GetEngine() stuff -- an IME reviewer might know if we need something there too (but I suspect not).
Description was changed from ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. Ideally, BridgedContentView's mouseEvent: method should have passed the event to [NSTextInputContext handleEvent:], but we can't do it currently since it uses characterIndexForPoint: which the BridgedContentView does not currently implement. BUG=652957 ========== to ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. BUG=652957 ==========
karandeepb@chromium.org changed reviewers: + shuchen@chromium.org
PTAL shuchen@. https://codereview.chromium.org/2434433006/diff/1/ui/base/ime/input_method_ma... File ui/base/ime/input_method_mac.mm (right): https://codereview.chromium.org/2434433006/diff/1/ui/base/ime/input_method_ma... ui/base/ime/input_method_mac.mm:32: [[NSTextInputContext currentInputContext] discardMarkedText]; On 2016/10/25 02:57:57, tapted wrote: > Do we need > > if (!IsTextInputClientFocused(client)) > return; > > the way Linux and Windows do? Added the check.
lgtm
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2434433006/#ps20001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. BUG=652957 ========== to ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. BUG=652957 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. BUG=652957 ========== to ========== MacViews: Implement InputMethodMac::CancelComposition. Currently, clicking inside a Textfield with an active composition, commits the composition text but does not close the IME candidate window. Hence, while Views::Textfield has committed the composition, the Cocoa text system does not know about it, leading to an incorrect state where the composition text may be committed once more. This CL implements InputMethodMac::CancelComposition which signals the Cocoa text system to close the IME candidate window and discard the composition text. BUG=652957 Committed: https://crrev.com/de65bcf491e13a0a7e0724e8732977561588072e Cr-Commit-Position: refs/heads/master@{#427557} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/de65bcf491e13a0a7e0724e8732977561588072e Cr-Commit-Position: refs/heads/master@{#427557} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
