Issue metadata
Sign in to add a comment
|
Keydown on an input field after autocomplete doesn't work the first time
Reported by
p...@paultondeur.com,
Jan 2 2017
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36 Steps to reproduce the problem: I created a JSFiddle to demonstrate the issue: http://jsfiddle.net/rzngeLhj/ 1. Trigger autofill suggestions on an input field 2. Select an autofill option using your keyboard. 3. Hit enter 4. Type 2 characters What is the expected behavior? Both characters to show up after the autocompleted text in the input field. What went wrong? Just one character appears to be appended to the input. Did this work before? N/A Chrome version: 57.0.2969.0 Channel: canary OS Version: OS X 10.12.2 Flash Version: Shockwave Flash 24.0 r0 I tested with Canary 56 and 57 which did not work as expected. I also tested with Chrome 55 (non canary), which seemed to work as expected. Not sure if this is a canary only bug, or something that was introduced in Canary 56.
,
Jan 17 2017
Gentle ping to get an update on this.
,
Jan 17 2017
I haven't yet had time to investigate this issue, but it's on my list of things to do. Simply reverting https://codereview.chromium.org/2482853002 is unfortunately not a good option.
,
Jan 17 2017
The patch https://codereview.chromium.org/2482853002 aims to address the issue 654140 , right? If the patch can be modified to work only when the browser window is fullscreen, then at least my problems described in Issue 668969 will be solved not perfectly but mostly. If a perfect fix is diffcult, even such a temporary fix will be appreciated.
,
Jan 17 2017
Yes, it was to address fullscreen trapping as in issue 654140 . Issue 668969 had the M-57 label but actually this affects M-56, so a fix is somewhat urgent. I'll do my best to have one this week.
,
Jan 17 2017
,
Jan 17 2017
Thanks for the updates. We are close to Stable Release, please make sure to land the fix and get it merged into the release branch ASAP. Thank you!
,
Jan 18 2017
Yeah, sorry about that, because issue 668969 had the M-57 label I mistakenly thought that the relevant change landed after the M57 branch point and that I had more time to handle this. Will look into it today.
,
Jan 18 2017
I've added some logging http://jsfiddle.net/rzngeLhj/2/ to get clues. Following the instructions and pressing keys a, down, enter, b, c, these are the events delivered before my change: keydown: KeyA keypress: KeyA keyup: KeyA keyup: ArrowDown keyup: Enter keydown: KeyB keypress: KeyB keyup: KeyB keydown: KeyC keypress: KeyC keyup: KeyC And after: keydown: KeyA keypress: KeyA keyup: KeyA keydown: KeyB keyup: KeyB keydown: KeyC keypress: KeyC keyup: KeyC In other words, previously bare keyup events for ArrowDown and Enter were sent, and all three events for b and c. Now, no events are sent for ArrowDown and Enter, which arguably makes more sense. But the keypress event for KeyB has also gone missing, which does not make sense however one looks at it.
,
Jan 18 2017
It looks as though the problem here is an additional layer of keypress event suppression in WebViewImpl. With the sequence of events now reaching WebViewImpl now slightly different, the RawKeyDown event for the first event after enter in WebViewImpl::handleKeyEvent() gets a WebInputEventResult::HandledSystem result, which causes m_suppressNextKeypressEvent to be set to true. That's all for today, will have to step in debugger to see what different state is causing this. (Before the change, that event would not be considered handled and thus the following event is not suppressed.)
,
Jan 19 2017
Looping in all reviewers of https://codereview.chromium.org/2482853002 The cause of this is now clear. In the repro steps, one presses the ArrowDown key to select one of the datalist options. For they key, RenderWidgetHostViewAura::ForwardKeyboardEvent created and sent a MoveDown command right before the call to RenderWidgetHostImpl::ForwardKeyboardEvent. There, if the key sequence is suppressed entirely, that command will be taken to apply to whatever keypress follows. In the repro steps, that was KeyB, and the reason the keypress event goes missing is that HTMLInputElement::defaultEventHandler() ends up in RenderFrameImpl::handleCurrentKeyboardEvent() where the MoveDown command is executed and the key event is considered handled, which suppresses the keypress event.
,
Jan 19 2017
CL is going through a CQ dry run: https://codereview.chromium.org/2644843004
,
Jan 23 2017
,
Jan 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b7b0799fa40dd39cc93e826e6bacfea41231d989 commit b7b0799fa40dd39cc93e826e6bacfea41231d989 Author: foolip <foolip@chromium.org> Date: Tue Jan 24 09:11:16 2017 Send keyboard-derived commands only if the key events are sent There was a regression in https://codereview.chromium.org/2482853002 Previously, when a keydown was handled on the browser side, the keypress was suppressed but the keyup was not. Now, all three events are suppressed. However, the fix did not take into consideration the InputMsg_SetEditCommandsForNextKeyEvent messages sent right before the call to RenderWidgetHostImpl::ForwardKeyboardEvent at two call sites. Because the keyup event was also suppressed, those commands would instead be associated with a following keydown. See https://bugs.chromium.org/p/chromium/issues/detail?id=677827#c11 for how that could end up dropping the keypress event of what followed. The fix is to introduce a ForwardKeyboardEventWithCommands which sends the commands only if they key itself is sent. BUG= 677827 Review-Url: https://codereview.chromium.org/2644843004 Cr-Commit-Position: refs/heads/master@{#445687} [modify] https://crrev.com/b7b0799fa40dd39cc93e826e6bacfea41231d989/content/browser/renderer_host/render_widget_host_impl.cc [modify] https://crrev.com/b7b0799fa40dd39cc93e826e6bacfea41231d989/content/browser/renderer_host/render_widget_host_impl.h [modify] https://crrev.com/b7b0799fa40dd39cc93e826e6bacfea41231d989/content/browser/renderer_host/render_widget_host_unittest.cc [modify] https://crrev.com/b7b0799fa40dd39cc93e826e6bacfea41231d989/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/b7b0799fa40dd39cc93e826e6bacfea41231d989/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Jan 24 2017
It is too late to merge this to M56 now, which is my fault as described in #8. Instead targeting 57, tweaking labels to reflect this.
,
Jan 25 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69e39c484ec2f1f3b7320f07f43fd0724b6cb382 commit 69e39c484ec2f1f3b7320f07f43fd0724b6cb382 Author: Philip Jägenstedt <foolip@chromium.org> Date: Wed Jan 25 09:53:03 2017 Send keyboard-derived commands only if the key events are sent There was a regression in https://codereview.chromium.org/2482853002 Previously, when a keydown was handled on the browser side, the keypress was suppressed but the keyup was not. Now, all three events are suppressed. However, the fix did not take into consideration the InputMsg_SetEditCommandsForNextKeyEvent messages sent right before the call to RenderWidgetHostImpl::ForwardKeyboardEvent at two call sites. Because the keyup event was also suppressed, those commands would instead be associated with a following keydown. See https://bugs.chromium.org/p/chromium/issues/detail?id=677827#c11 for how that could end up dropping the keypress event of what followed. The fix is to introduce a ForwardKeyboardEventWithCommands which sends the commands only if they key itself is sent. BUG= 677827 Review-Url: https://codereview.chromium.org/2644843004 Cr-Commit-Position: refs/heads/master@{#445687} (cherry picked from commit b7b0799fa40dd39cc93e826e6bacfea41231d989) Review-Url: https://codereview.chromium.org/2650913006 . Cr-Commit-Position: refs/branch-heads/2987@{#84} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/69e39c484ec2f1f3b7320f07f43fd0724b6cb382/content/browser/renderer_host/render_widget_host_impl.cc [modify] https://crrev.com/69e39c484ec2f1f3b7320f07f43fd0724b6cb382/content/browser/renderer_host/render_widget_host_impl.h [modify] https://crrev.com/69e39c484ec2f1f3b7320f07f43fd0724b6cb382/content/browser/renderer_host/render_widget_host_unittest.cc [modify] https://crrev.com/69e39c484ec2f1f3b7320f07f43fd0724b6cb382/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/69e39c484ec2f1f3b7320f07f43fd0724b6cb382/content/browser/renderer_host/render_widget_host_view_mac.mm
,
Jan 25 2017
,
Jan 25 2017
Just to update regarding the added 'Needs-triage-Mobile' label on the reported version: 57.0.2969.0. Issue is not applicable on Samsung Galaxy S5 Handset, Android Version: 5.0.0; SM-G900H Build/LRX21T and Galaxy Tab S2, Android Version: 6.0.1; SM-T815Y Build/MMB29K. Steps followed: ================ 1. Paired a Bluetooth keyboard to the Handset and Tablet. 2. Opened the jsfiddle: http://jsfiddle.net/rzngeLhj/ Unable to select jsfiddle option using arrow down key from the paired Bluetooth keyboard on the above jsfiddle.
,
Feb 16 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by kkaluri@chromium.org
, Jan 3 2017Labels: -Type-Bug hasbisect-per-revision M-57 OS-Linux Type-Bug-Regression
Owner: foolip@chromium.org
Status: Assigned (was: Unconfirmed)