Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
649343002:
event_sender: Keep keyIdentifier in keypress event for Enter (Closed)
|
Created:
6 years, 2 months ago by robwu Modified:
6 years, 1 month ago CC:
Avi (use Gerrit), chromium-reviews, darin-cc_chromium.org, jam, jbroman, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionevent_sender: Keep keyIdentifier in keypress event
Blink's actual implementation preserves keyIdentifier='Enter'
when Enter is pressed. To make sure that the test framework
behaves like the actual implementation, this CL makes sure that
keyIdentifier is set to 'Enter' when the Enter key is simulated.
BUG=423270
Committed: https://crrev.com/994f9e8c89cfcf1861ffb88b57523e373e62176f
Cr-Commit-Position: refs/heads/master@{#302403}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Only show a keyIdentifier for Enter #Patch Set 3 : UI::VKEY_ESCAPE -> UI::VKEY_RETURN #
Total comments: 1
Patch Set 4 : remove braces #Patch Set 5 : UI -> ui #
Total comments: 2
Patch Set 6 : not not not but not #Messages
Total messages: 21 (7 generated)
rob@robwu.nl changed reviewers: + dpranke@chromium.org
This issue appeared in https://codereview.chromium.org/619613005/. The writer of the CL wanted to use keyIdentifier in keypress, but for some reason it was blank.
rob@robwu.nl changed reviewers: + abarth@chromium.org
I've updated and temporarily disabled some layout tests in https://codereview.chromium.org/641233004/ to avoid test failures on the bots after this patch lands.
dpranke@chromium.org changed reviewers: + jbroman@chromium.org, jochen@chromium.org
This change looks plausible, but I don't know this code at all. Maybe jochen or jbroman can take a look?
jbroman@chromium.org changed reviewers: - jbroman@chromium.org
I'm afraid I'm not the expert you're looking for; I was just moving code in this file awhile ago. https://codereview.chromium.org/649343002/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/event_sender.cc (left): https://codereview.chromium.org/649343002/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/event_sender.cc:1368: event_char.keyIdentifier[0] = '\0'; Are char events supposed to have key identifiers? The other source of them in this files leaves this field null (though other sites do populate it). https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... This seems suspicious to me; please ask someone who knows more about how char events are supposed to work.
https://codereview.chromium.org/649343002/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/event_sender.cc (left): https://codereview.chromium.org/649343002/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/event_sender.cc:1368: event_char.keyIdentifier[0] = '\0'; On 2014/10/14 17:53:56, jbroman wrote: > Are char events supposed to have key identifiers? The other source of them in > this files leaves this field null (though other sites do populate it). > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... > > This seems suspicious to me; please ask someone who knows more about how char > events are supposed to work. This seems to have been intentionally added in: https://codereview.chromium.org/27332/diff/2008/webkit/tools/test_shell/event... though there's not much context as to *why*
https://codereview.chromium.org/649343002/diff/1/content/shell/renderer/test_... File content/shell/renderer/test_runner/event_sender.cc (left): https://codereview.chromium.org/649343002/diff/1/content/shell/renderer/test_... content/shell/renderer/test_runner/event_sender.cc:1368: event_char.keyIdentifier[0] = '\0'; On 2014/10/14 17:53:56, jbroman wrote: > Are char events supposed to have key identifiers? I tested every key on my keyboard (US keyboard, Linux), and it seems that it is only set for the Enter key. I've updated the CL to match this finding. To reproduce, open the Developer tools (F12), run the following code, then press any key within the page. Only Enter is printed: onkeypress = function(e){console.log(e.keyIdentifier)}; (Side note: In Firefox, the "key" property (which is the successor to the no-longer-standard keyIdentifier property) is non-empty for every key. Getting the Keyboard events to match the spec is https://crbug.com/263724)
the comment says enter key, but the code says escape key?
On 2014/10/15 15:22:58, jochen wrote: > the comment says enter key, but the code says escape key? Thanks for catching. I've changed it to RETURN.
lgtm https://codereview.chromium.org/649343002/diff/70001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/649343002/diff/70001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1371: if (code != UI::VKEY_RETURN) { nit. no { } required
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/649343002/diff/110001/content/shell/renderer/... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/649343002/diff/110001/content/shell/renderer/... content/shell/renderer/test_runner/event_sender.cc:1369: // This behavior is not not standard (keyIdentifier itself is not even nit: -not
https://codereview.chromium.org/649343002/diff/110001/content/shell/renderer/... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/649343002/diff/110001/content/shell/renderer/... content/shell/renderer/test_runner/event_sender.cc:1369: // This behavior is not not standard (keyIdentifier itself is not even On 2014/10/24 22:37:20, mmenke wrote: > nit: -not not not not but not - thanks for spotting this :)
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649343002/130001
Message was sent while issue was closed.
Committed patchset #6 (id:130001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/994f9e8c89cfcf1861ffb88b57523e373e62176f Cr-Commit-Position: refs/heads/master@{#302403} |
