Why can't you use the keydown message and not the keypress?

Looking at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp?l=1207

is difficult because only that code executes on the Windows platform. Other platforms follow other code paths.

I debugged a little bit on Windows, and I found reasons why it doesn't work:

At first some context: we are interested that method AutofillAgent::TextFieldDidChange is called on any user keystroke in input elements and that in this method we could be able to verify that this was user gesture https://cs.chromium.org/chromium/src/components/autofill/content/renderer/autofill_agent.cc?type=cs&q=if+%5C(is_user_gesture_required_+%26%26+!IsUserGesture%5C(&sq=package:chromium&l=333

That code that Maxim pointed out in the bug description was the first suspect, but it turned out, even with commenting it, we didn't receive info that this was user gesture. 

And the reason is that when backspace is pressed, |WebKeyboardEvent.is_browser_shortcut| is set to true and for shortcuts UserGestureIndicator is not created (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp?q=KeyboardEventManager.cpp&sq=package:chromium&dr&l=202)

I've tried to fix this with creating UserGestureIndicator for backspace in CL https://chromium-review.googlesource.com/c/568299/ and it works (I've tested on Linux and Windows). It seems that it's logical to created UserGestureIndicator for backspace. WDYT whether such fix will work?

I wonder whether backspace is still a browser shortcut or whether this is a reminiscent of times when it triggered a back navigation.

This issue may cause incorrect password saves, especially, saves of strong passwords generated by Chrome. I increase the priority.

third_party/WebKit/Source/core/OWNERS says that dtapuska reviews input-related changes. Assign bug to him. Friendly ping.

Sending to avi@ as he was last in this user gesture code specifically around this area.

I altered the behavior of the user gesture, removing it from browser shortcuts, but backspace isn't a browser shortcut any more.

Dave, are you familiar with the "browser shortcut" code? Do you know why backspace is still counted as one?

Browser shortcut is being reported as not a browser shortcut for backspace for me... So comment #5 seems odd. 

[1:1:0714/151949.812748:ERROR:KeyboardEventManager.cpp(201)] Is browser shortcut 0

Can the reporters be explicit about what happened here? ie; was it a regression? Is there a per-revision bisect that you can report against?

It seems this bug has two many things going on.
1) keypress events are skipped for good reasons in the web platform.
2) It is unclear if password manager was changed that caused some issues
3) Or is this a long standing bug that you are trying to fix.

s/two/too :-|

dvadym@ can you respond against comment #11.

Sorry for confusing.

To #11: (3) It is a long standing bug. Its effects were mitigated in old design, but now we are implementing manual fallbacks where updates of field value are crucial. We need to receive backspace events as well. How can we receive them on the renderer side?

how does password manager possibly work with devices that don't generate keyboard events? 
Or dead keys?
I guess you don't use composition in password fields right?

Ideally you shouldn't listen to the raw keyboard events but actually the composition event stream. 

Is this manager supposed to work on Android?

Backspace used to be a shortcut for navigating back.  It might be a residue of that behavior (which was deleted recently).

Autofill change detection largely works OK on Android with IME input.  At least the yellow/white color change works fine.  So I think these calls are mostly hooked up properly through whatever mechanism.

1.KeyboardEventManager::KeyEvent is called several times for kRawKeyDown, kKeyUp, kChar, on Linux logs of calls looks (we're intereted in that user_gesture=1 in call of TextFieldDidChange):
 1) for not shortcut
[1:1:0717/135726.361745:ERROR:KeyboardEventManager.cpp(200)] is_browser_shortcut=0 kRawKeyDown
[1:1:0717/135726.362577:ERROR:KeyboardEventManager.cpp(200)] is_browser_shortcut=0 kChar
[1:1:0717/135726.365723:ERROR:autofill_agent.cc(330)] TextFieldDidChange user_gesture=1
[1:1:0717/135726.528957:ERROR:KeyboardEventManager.cpp(200)] is_browser_shortcut=0 kKeyUp
  
 2) for shortcut (incl backspace):
[1:1:0717/135735.516666:ERROR:KeyboardEventManager.cpp(200)] is_browser_shortcut=1 kRawKeyDown
[1:1:0717/135735.520571:ERROR:autofill_agent.cc(330)] TextFieldDidChange user_gesture=0  <- this is the problem
[1:1:0717/135735.600921:ERROR:KeyboardEventManager.cpp(200)] is_browser_shortcut=0 kKeyUp

dtapuska@, you probably saw calls of kKeyUp.

The problem is that UserGestureIndicator is not created on shortcut as in https://chromium-review.googlesource.com/c/568299/1

2.This bug is related only to keyboard events.

3.We don't listen any raw events, we listen text change in text fields, bug UserGestureIndicator is created somewhere deep in processing of raw input event in Blink (namely in place what's changed in https://chromium-review.googlesource.com/c/568299/1)

4.Backspace is still in accelerators table. I've uploaded another patch to CL https://chromium-review.googlesource.com/c/568299/3, with removing it from accelerator table. This patch fixes our problem. Are you ok with it? 

(1) How come we don't set the user gesture bit on browser shortcuts?  On its face that doesn't sound correct, but I assume there's some reason for this.  (Sounds like Avi knows?)

(2) What will be the effect on extensions like https://chrome.google.com/webstore/detail/go-back-with-backspace/eekailopagacbcdloonjhbiecobagjci?hl=en if we remove this from the accelerator table?  Will they stop intercepting backspace on certain sites?

Re (1) this was a deliberate change to solve a problem.

We only allow a page to show a beforeunload dialog if the user has interacted with the page. This helps prevents abusive use of beforeunload by requiring at least the possibility of there being unsaved data before the page can show a dialog.

However, we don't want a control-R keystroke to count as a user gesture for the purpose of allowing a page to show a dialog.

That's why I made the change to make browser keystrokes not count as user gestures, as they are not indicative of an intent by a user to interact with the web page.

My questions here are:

1. Why do we still mark the backspace as a browser command?
2. Why does there need to exist a user gesture for the password manager to detect a backspace?

1. Avi did make this change. He can comment on the reasoning.
2. I don't think it should have an effect since extensions are run part of the page. The page is actually going to start seeing that backspace is cancelable now (which it really should have been). But Vadym can you try your patch against that extension and make sure it is still working?

In response to avi@ comments

1. Because it appears there is a dialog that reminds the user the shortcut has changed for the first 5 invocations of the dialog. Vadym change this as per my suggestion: https://chromium-review.googlesource.com/c/568299/

2. Not sure why.

In response of pkasting@ comments

2. I've checked extension https://chrome.google.com/webstore/detail/go-back-with-backspace/eekailopagacbcdloonjhbiecobagjci?hl=en with my patch. It works.
 

In response to avi@ comments

2.Password Manager needs to know which input value changes are from the user in order to prevent cases when JavaScript mangles username/password and incorrect values are saved.


I'll add tests for my CL, after returning from traveling in 3 days.

Assigning to dvadym@ since a patch is in the works just needs a few cleanups.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b750849271adebe4dc164d1101ccaaa3790f922

commit 8b750849271adebe4dc164d1101ccaaa3790f922
Author: Avi Drissman <avi@chromium.org>
Date: Thu Aug 10 21:18:26 2017

Revert "Don't count browser shortcut events as being user gestures."

This reverts commit 56f9a641544d2cafa37142545bb39254f7bd98b3.

The lack of a user gesture for browser shortcut events causes web
compatibility issues. This needs a better-engineered approach.

BUG=709765,  749005 ,  753612 ,  741612 
TEST=as in bugs

Change-Id: I2faee9623597561297e01756b4bdf6f56c049eeb
Reviewed-on: https://chromium-review.googlesource.com/610829
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493549}
[modify] https://crrev.com/8b750849271adebe4dc164d1101ccaaa3790f922/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp

FYI, I had to revert the CL that removed user gestures from browser commands. I don't know how that affects this bug in particular but I thought you should know.

Thanks avi@. Password Manager works correctly now (as far as I've tested so far).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0234225c87bd459b2039d642921e9a7ad502eb89

commit 0234225c87bd459b2039d642921e9a7ad502eb89
Author: Avi Drissman <avi@chromium.org>
Date: Fri Aug 11 17:35:51 2017

Revert "Don't count browser shortcut events as being user gestures."

This reverts commit 56f9a641544d2cafa37142545bb39254f7bd98b3.

The lack of a user gesture for browser shortcut events causes web
compatibility issues. This needs a better-engineered approach.

BUG=709765,  749005 ,  753612 ,  741612 
TEST=as in bugs
TBR=avi@chromium.org

Change-Id: I2faee9623597561297e01756b4bdf6f56c049eeb
Reviewed-on: https://chromium-review.googlesource.com/610829
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#493549}(cherry picked from commit 8b750849271adebe4dc164d1101ccaaa3790f922)
Reviewed-on: https://chromium-review.googlesource.com/611668
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#490}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/0234225c87bd459b2039d642921e9a7ad502eb89/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp

I think this is fixed now.