Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973463002:
MacViews: Don't InsertChar() for consumed events. (Closed)
|
Created:
3 years, 5 months ago by tapted Modified:
3 years, 5 months ago Reviewers:
msw CC:
chromium-reviews, yusukes+watch_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, mac-reviews_chromium.org, James Su, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Don't InsertChar() for consumed events.
Currently, BridgedContentView ignores KeyEvent::handled() when
processing a key event both as a KeyDown and as text input. Handling
should only proceed to text input when the event isn't handled after
first being sent to OnKeyEvent handlers.
Otherwise, it means a <space> on a control that causes a textfield to be
focused will also send that space as input to the textfield.
BUG=732229
Review-Url: https://codereview.chromium.org/2973463002
Cr-Commit-Position: refs/heads/master@{#484475}
Committed: https://chromium.googlesource.com/chromium/src/+/a42d24fc97610d00ddf15722e14aa0f413e56561
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Fix non-mac #
Total comments: 6
Patch Set 4 : Respond to commments #Patch Set 5 : Rebase (patch failure due to r484422) #
Messages
Total messages: 27 (22 generated)
The CQ bit was checked by tapted@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 tapted@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.
Description was changed from ========== MacViews: Don't InsertChar() for consumed events. BUG=732229 ========== to ========== MacViews: Don't InsertChar() for consumed events. Currently, BridgedContentView ignores KeyEvent::handled() when processing a key event both as a KeyDown and as text input. Handling should only proceed to text input when the event isn't handled after first being sent to OnKeyEvent handlers. Otherwise, it means a <space> on a control that causes a textfield to be focused will also send that space as input to the textfield. BUG=732229 ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
Hi Mike, please take a look https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:160: return DispatchKeyEventPostIME(key); Note Mac bails out of this function up here -- it didn't need the change on line 185. But it's a bug for MockInputMethod not do have the stuff on line 185 for other platforms (coincidentally, almost the same bug that BridgedContentView had)
lgtm with a q and a nit. https://codereview.chromium.org/2973463002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2973463002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:533: if (charEvent.handled()) q: Should this actually be in the |isCharacterEvent && ![self hasMarkedText]| conditional, or should other previously-handled key events also early return? https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:160: return DispatchKeyEventPostIME(key); On 2017/07/04 08:13:15, tapted wrote: > Note Mac bails out of this function up here -- it didn't need the change on line > 185. But it's a bug for MockInputMethod not do have the stuff on line 185 for > other platforms (coincidentally, almost the same bug that BridgedContentView > had) Acknowledged. https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:359: void PretendNotConsumed() { consume_ = false; } nit: do a simple |void set_consume(bool consume) { consume_ = consume; }|
The CQ bit was checked by tapted@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tapted@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.
Thanks Mike! https://codereview.chromium.org/2973463002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2973463002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:533: if (charEvent.handled()) On 2017/07/05 19:21:21, msw wrote: > q: Should this actually be in the |isCharacterEvent && ![self hasMarkedText]| > conditional, or should other previously-handled key events also early return? This one is a special case. The |charEvent| that gets handled only exists within the scope of the (|isCharacterEvent && ![self hasMarkedText]|) conditional, so the check for early return must be here. It's special because on Mac we need to handle actions from non-accelerator events such as VKEY_SPACE inside a handler for [.. insertText:@" "] so that IME first gets a chance to handle it (see long comment above :). suppression of handled key events in other places happens via hasUnhandledKeyDownEvent_, but we can't do that here because the unhandled KeyDown we track doesn't have all the information on it (i.e. the full string), which we need later in this method. https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2973463002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:359: void PretendNotConsumed() { consume_ = false; } On 2017/07/05 19:21:21, msw wrote: > nit: do a simple |void set_consume(bool consume) { consume_ = consume; }| Done.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2973463002/#ps80001 (title: "Rebase (patch failure due to r484422)")
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": 80001, "attempt_start_ts": 1499320700417630,
"parent_rev": "13592b1a361d8faad62b9789fed6725afa91f008", "commit_rev":
"a42d24fc97610d00ddf15722e14aa0f413e56561"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Don't InsertChar() for consumed events. Currently, BridgedContentView ignores KeyEvent::handled() when processing a key event both as a KeyDown and as text input. Handling should only proceed to text input when the event isn't handled after first being sent to OnKeyEvent handlers. Otherwise, it means a <space> on a control that causes a textfield to be focused will also send that space as input to the textfield. BUG=732229 ========== to ========== MacViews: Don't InsertChar() for consumed events. Currently, BridgedContentView ignores KeyEvent::handled() when processing a key event both as a KeyDown and as text input. Handling should only proceed to text input when the event isn't handled after first being sent to OnKeyEvent handlers. Otherwise, it means a <space> on a control that causes a textfield to be focused will also send that space as input to the textfield. BUG=732229 Review-Url: https://codereview.chromium.org/2973463002 Cr-Commit-Position: refs/heads/master@{#484475} Committed: https://chromium.googlesource.com/chromium/src/+/a42d24fc97610d00ddf15722e14a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a42d24fc97610d00ddf15722e14a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
