Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
929053004:
[KeyboardEvent] Add embedder APIs to translate between Dom |key| enum and strings (Closed)
|
Created:
5 years, 10 months ago by Habib Virji Modified:
5 years, 1 month ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, tdresser+watch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-shell_chromium.org, jdduke+watch_chromium.org, jochen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[KeyboardEvent] Add embedder APIs to translate between Dom |key| enum and strings
Embedder API for accessing DOM3 |key| value based on the enum value.
Sets the dom |key| enum value for each WebKeyboardEvent. Blink then uses the embedded API to translate the dom |key| enum value to a value string.
It is dependent on CL 1103263004, which sets dom |key| value in X11.
BUG=227231
R=piman@chromium.org, Wez
Patch Set 1 #
Total comments: 2
Patch Set 2 : "API has been updated on the blink side" #
Total comments: 12
Patch Set 3 : Code review comments #
Total comments: 12
Patch Set 4 : #Patch Set 5 : Added a new parameter to pass DOM |key| character #
Total comments: 20
Patch Set 6 : Passing Dom KEY printable and non-printable to pass value under domKey #
Total comments: 9
Patch Set 7 : Updated MakeWebKeyboardEvent function with the changes. #
Total comments: 10
Patch Set 8 : event_sender changes to use Dom |code| instead of KeyboardCode #Patch Set 9 : Fixed failing LayoutTests. #Patch Set 10 : Fix keyboard.location layout test failure #Patch Set 11 : UnitTest updated as MakeKeyboardEvent now adds a value to differentiate #
Total comments: 24
Patch Set 12 : Using DomKey::CHARACTER to differentiate DomKey value and unicode value #Patch Set 13 : Cleanup of the dom_key.h #
Total comments: 7
Messages
Total messages: 77 (14 generated)
habib.virji@samsung.com changed reviewers: + kpschoedel@chromium.org
DOM3 key support via embedder API. Includes a small change in event.cc, to set key value in X11.
lgtm
In the description and title: Suggest "Add embedder APIs to translate between domKey enum and DOM |key| strings." as the issue title. "Set the domKey value to the..." -> "Sets the domKey value on each WebKeyboardEvent. Blink then uses the embedded API to translate the domKey to a DOM |key| value string." Typo: keyboard_converer_data.h -> keyboard_converter_data.h, although I'd suggest just removing that comment, since it doesn't really add anything. Re eventsender comment: eventsender->EventSender. I'd also suggest clarifying that it sets the domKey value based on assuming US English layout.
https://codereview.chromium.org/929053004/diff/1/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/929053004/diff/1/ui/events/event.cc#newcode785 ui/events/event.cc:785: GetMeaningFromKeyCode(key_code_, flags(), &key_, &character_); GetMeaningFromKeyCode currently assumes a US English layout, hence the TODO that Kevin put for the usage in the #else clause; suggest copying that TODO to this call as well. Since this code only works for US English layouts currently, we need to get it working for general layouts before we make the |key| attribute available to JS, really (or perhaps only make it available if we detect a US layout is in effect?)
https://codereview.chromium.org/929053004/diff/1/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/929053004/diff/1/ui/events/event.cc#newcode785 ui/events/event.cc:785: GetMeaningFromKeyCode(key_code_, flags(), &key_, &character_); I have started looking into enabling |key| attribute to other layout. Kevin has forwarded me his current work and I am looking into it. Will prefer holding it enabled for all other layout before landing this.
@Wez: I have uploaded CL already that addresses above issue. Based on your response for that CL, we can proceed with this CL.
https://codereview.chromium.org/929053004/diff/20001/content/child/blink_plat... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/20001/content/child/blink_plat... content/child/blink_platform_impl.cc:1280: static_cast<ui::DomKey>(dom_key))); nit: Strictly speaking the behaviour of static_cast<enum> is undefined if you pass in an int value that is not a valid enum value, IIRC; we seem to be doing the same thing above for domCodeStringFromEnum, though, and these should always be values that we provided in the first place, so shouldn't be a problem. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1282: std::string domString; Looks like this should be domCode, to match naming of domKey. Similarly, "code" above should become "keyCode", unfortunately. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1411: domKey = domString; This looks wrong; DOM |code| and |key| are different things; you can't just assign a |code| value as the |key|. https://codereview.chromium.org/929053004/diff/20001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/929053004/diff/20001/ui/events/event.cc#newco... ui/events/event.cc:791: GetMeaningFromKeyCode(key_code_, flags(), &key_, &character_); Not sure what you're trying to do here. GetMeaningFromKeyCode translates from a Windows VKEY code + flags to a DOM |key| value, and optionally a corresponding character code. Under X11 the |key_code_| field is already derived from the X KeySym value, so we've already lost information - we instead want to map the KeySym -> DOM |key| directly.
Also note that Kevin has crrev.com/841263005 in progress, so some of the ui::Event code is going to change.
(Incidentally, my apologies once again for the delay in reviewing this; please feel free to send a "ping" message on these CLs if they sit un-reviewed for more than a day or two!)
Thanks wez for review. Apologise I was late this time as I am catching up with the blink work. I have modified event_sender as you suggested. This change still relies on the other CL and will push in changes to it. https://codereview.chromium.org/929053004/diff/20001/content/child/blink_plat... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/20001/content/child/blink_plat... content/child/blink_platform_impl.cc:1280: static_cast<ui::DomKey>(dom_key))); Right wez,this issue was discussed in domCodeString case too and the conclusion was as you stated above, since the value here are sent by the embedder API. It should be okay. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1282: std::string domString; On 2015/04/21 02:25:58, Wez wrote: > Looks like this should be domCode, to match naming of domKey. Similarly, "code" > above should become "keyCode", unfortunately. Done. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1411: domKey = domString; Agreed but for some keys such as Enter, ArrowRight/Down/Left/Up, Insert, PageUp/Down, Delete. Home, End, PrintScreen they share same value. In case domKey is empty, then domCode is assigned, the aim is to cover for above shared keys. https://codereview.chromium.org/929053004/diff/20001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/929053004/diff/20001/ui/events/event.cc#newco... ui/events/event.cc:791: GetMeaningFromKeyCode(key_code_, flags(), &key_, &character_); There was a following CL: 954943003 that was trying to get the mapping from KeySym to DomKey https://codereview.chromium.org/954943003/patch/40001/20016 SInce Kevin CL has landed. I will update 954943003 accordingly. This will remain same.
@Wez: I have updated other CL and it does not impact on this. Should we land this as I am hopefully pushing Blink changes in.
@wez blink side changes has landed. Can we land this change, X11 changes are depending on Kevin CL 1103263004. Which I have updated in the description. I reckon if we land this, event.cc will be updated when Kevin lands his patch as he is updating that part of the code which is changed in this CL. For other platforms, it is working and since it is in experimental flag it will help us fix some issue prior to enabling on May 15.
On 2015/05/01 15:40:22, Habib Virji wrote: > @wez blink side changes has landed. Can we land this change, X11 changes are > depending on Kevin CL 1103263004. Which I have updated in the description. I > reckon if we land this, event.cc will be updated when Kevin lands his patch as > he is updating that part of the code which is changed in this CL. > > For other platforms, it is working and since it is in experimental flag it will > help us fix some issue prior to enabling on May 15. @wez: Please can you review this change. This requires your attention.
https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1411: domKey = domString; On 2015/04/29 16:05:18, Habib Virji wrote: > Agreed but for some keys such as Enter, ArrowRight/Down/Left/Up, Insert, > PageUp/Down, Delete. Home, End, PrintScreen they share same value. In case > domKey is empty, then domCode is assigned, the aim is to cover for above shared > keys. It doesn't make sense in general, though, since the two differ, e.g. if you inject an event for Digit0 then |key| should be '0'. The EventSender seems to be designed to support only basic Windows VKEY events, and to implicitly assume a US English layout - I'd suggest we take one of two approaches here: 0. Convert the supplied string into the VKEY we want to generate, using the existing code in this function. 1. Use the DomCode<->VKEY mapping table we have for US English to go from that to the DOM |code|. 2. Use the DomCode<->DomKey+character mapping table we have for US English to populate the DOM |key| field. Step #0 could go away in future, if we had the EventSender just accept a DOM |code| string directly, but we can do that as a separate stage, if we want, since it'll require updating a load of tests. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1413: ui::KeycodeConverter::KeyStringToDomKey(domKey.c_str())); This handles the DomKey enum values, but those only cover non-printable DOM |key| values - what about printable/character DOM |key| values? e.g. If the caller chooses to inject code=Digit0 then the DomKey value will be DomKey::CHARACTER and the event needs to store the character code for the DOM |key| string separately - see: https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... Not sure if you can get away with (ab)using |m_text| in the WebKeyboardEvent, or will need to add a new member to store the character code, unfortunately. https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); You also need to propagate the ui::KeyEvent::character_ value into the WebKeyboardEvent, to cope with printable DOM |key| values, since the DomKey enum only deals with non-printable keys. https://codereview.chromium.org/929053004/diff/40001/content/child/blink_plat... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/929053004/diff/40001/content/child/blink_plat... content/child/blink_platform_impl.h:176: virtual blink::WebString domKeyStringFromEnum(int dom_code); nit: dom_code -> dom_key https://codereview.chromium.org/929053004/diff/40001/content/child/blink_plat... content/child/blink_platform_impl.h:177: virtual int domKeyEnumFromString(const blink::WebString& codeString); nit: codeString -> keyString
Apologise wez for delay in replying. Was travelling and wanted to be back on desk to see how to implement printable/character DOM |key| values. Apparently there is already a support present via unmodifiedText and will not require any new changes. event_sender needs a new mechanism to support domKey and domCode and I have started looking into it. It will ease if we can land this CL and I continue event_sender in a separate CL. The effected test file, using event_sender is already marked as a failing test in TestExpectations. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1411: domKey = domString; I agree with the point you are making and it appears to be bit an effort. Have started looking into using DomCode, Vkey mapping and then using DomCode to DomKey. It will require more few days fixing all the tests. It will ease if I can land this CL and carry on with fixing event_sender as a separate CL. Will that be okay? Can also remove the event_sender and implement in other CL. https://codereview.chromium.org/929053004/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:1413: ui::KeycodeConverter::KeyStringToDomKey(domKey.c_str())); This is already handled in unmodifiedText: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... So the changes will also set this value corectly. https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); On 2015/05/07 00:23:22, Wez wrote: > You also need to propagate the ui::KeyEvent::character_ value into the > WebKeyboardEvent, to cope with printable DOM |key| values, since the DomKey enum > only deals with non-printable keys. It is already covered in the below unmodifiedText as GetUnmodifiedText in turns call GetCharacter(). https://codereview.chromium.org/929053004/diff/40001/content/child/blink_plat... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/929053004/diff/40001/content/child/blink_plat... content/child/blink_platform_impl.h:176: virtual blink::WebString domKeyStringFromEnum(int dom_code); On 2015/05/07 00:23:23, Wez wrote: > nit: dom_code -> dom_key Acknowledged. https://codereview.chromium.org/929053004/diff/40001/content/child/blink_plat... content/child/blink_platform_impl.h:177: virtual int domKeyEnumFromString(const blink::WebString& codeString); On 2015/05/07 00:23:23, Wez wrote: > nit: codeString -> keyString Acknowledged.
https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); You'll also need to set .domKey in the OS_WIN case of MakeWebKeyboardEvent() below. (Look for the .domCode after you rebase.) Also add to WebInputEventAuraTest.TestMakeWebKeyboardEvent in content/browser/renderer_host/web_input_event_aura_unittest.cc (again, look for .domCode there).
Thanks Kevin, incorporated your suggested changes.
@wez: event_sender.cc issues are mostly addressed with DomCode and DomKey are
derived using UsLayoutKeyboardCodeToDomCode and GetMeaningFromKeyCode
respectively. The only issue currently is with printable character for the
DomKey for the layout test, it require changes on the Blink side.
In browser it is working correctly by reusing unmodifiedText value for the
DomKey. I have followup CL in the blink (not submitted yet, working on setting
printable character working for the layout test) where in
Source/web/WebInputEventConversion.cpp:: PlatformKeyboardEventBuilder =>
if (m_key.isNull() && !m_unmodifiedText.isNull())
m_key = m_unmodifiedText;
This sets the value for the domKey and is printing similar to firefox values.
https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer...
File content/browser/renderer_host/web_input_event_aura.cc (right):
https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer...
content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey =
static_cast<int>(event.GetDomKey());
On 2015/05/19 20:00:26, kpschoedel wrote:
> You'll also need to set .domKey in the OS_WIN case of MakeWebKeyboardEvent()
> below. (Look for the .domCode after you rebase.) Also add to
> WebInputEventAuraTest.TestMakeWebKeyboardEvent in
> content/browser/renderer_host/web_input_event_aura_unittest.cc (again, look
for
> .domCode there).
Done.
On 2015/05/21 16:36:10, Habib Virji wrote: > Thanks Kevin, incorporated your suggested changes. > > @wez: event_sender.cc issues are mostly addressed with DomCode and DomKey are > derived using UsLayoutKeyboardCodeToDomCode and GetMeaningFromKeyCode > respectively. The only issue currently is with printable character for the > DomKey for the layout test, it require changes on the Blink side. > > In browser it is working correctly by reusing unmodifiedText value for the > DomKey. I have followup CL in the blink (not submitted yet, working on setting > printable character working for the layout test) where in > Source/web/WebInputEventConversion.cpp:: PlatformKeyboardEventBuilder => > > if (m_key.isNull() && !m_unmodifiedText.isNull()) > m_key = m_unmodifiedText; > > This sets the value for the domKey and is printing similar to firefox values. > > https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... > content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = > static_cast<int>(event.GetDomKey()); > On 2015/05/19 20:00:26, kpschoedel wrote: > > You'll also need to set .domKey in the OS_WIN case of MakeWebKeyboardEvent() > > below. (Look for the .domCode after you rebase.) Also add to > > WebInputEventAuraTest.TestMakeWebKeyboardEvent in > > content/browser/renderer_host/web_input_event_aura_unittest.cc (again, look > for > > .domCode there). > > Done. @wez can u please have a look
lgtm
On 2015/05/26 18:24:56, kpschoedel wrote: > lgtm @wez ping
https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); On 2015/05/19 16:16:28, Habib Virji wrote: > On 2015/05/07 00:23:22, Wez wrote: > > You also need to propagate the ui::KeyEvent::character_ value into the > > WebKeyboardEvent, to cope with printable DOM |key| values, since the DomKey > enum > > only deals with non-printable keys. > > It is already covered in the below unmodifiedText as GetUnmodifiedText in turns > call GetCharacter(). unmodifiedText is an old field in WebKit keyboard events - it has nothing to do with the new |key| specification, and is only valid on |keypress| (i.e. character) events, in general.
Patchset #5 (id:80001) has been deleted
Thanks wez. Updated code now to pass a printable DOM |key| value separately. Please have a look. https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); In GetUnmodifiedText it does not check the |keypress| event. https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc...
https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); On 2015/06/05 20:05:11, Habib Virji wrote: > In GetUnmodifiedText it does not check the |keypress| event. > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc... No, but the GetCharacter() function that it delegates to returns |character_|, which is not set for non-keypress events under Windows. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1258: int keyCode = 0; keyCode -> key_code Chromium style-guide requires unix_style names for variables, rather than camelCaseStyle. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1320: WebString::fromUTF8(code_str.data(), code_str.size()); Why do we even convert this to a WebString? We never seem to use it as a WebString anywhere here? https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1327: text = keyCode = web_code_str.at(0); DOM |code| values for e.g. keys A-Z are of the form "KeyA", for example, not "A", so this won't do what you intend - you'll always generate VKEY_K for those |code| values, surely? nit: This implementation also seems to be assuming a US English layout - is that valid for EventSender? https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1340: ui::DomKey domkey = ui::DomKey::NONE; nit: domkey -> dom_key, domcode -> dom_code, domkey_char -> dom_key_char nit: Declare domKey immediately before domKeyChar, below. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1345: ui::EF_NONE, &domkey, &domkey_char); We're trying to get rid of GetMeaningFromKeyCode(), since it's a hack. To migrate EventSender::KeyDown() to the new scheme it should: 1. Process the supplied |code_str| (which should be renamed |key_code_str| to match |key_code| and |keyCode| naming within this function) to get the VKEY value. 2. Call UsLayoutKeyboardCodeToDomCode to get the DomCode for the VKEY under a US English layout. 3. Call DomCodeToUsLayoutMeaning to get the DomKey+character corresponding to the meaning of that key under a US English layout. Ideally we'd migrate EventSender to use DOM |code| as the parameter in place of the old KeyboardCode parameter, at which point we would be able to dispense with step #1, and instead set the KeyboardCode based on the result of #3. https://codereview.chromium.org/929053004/diff/100001/content/child/blink_pla... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/929053004/diff/100001/content/child/blink_pla... content/child/blink_platform_impl.h:180: virtual int domEnumFromCodeString(const blink::WebString& codeString); code_string (see below) https://codereview.chromium.org/929053004/diff/100001/content/child/blink_pla... content/child/blink_platform_impl.h:183: virtual int domKeyEnumFromString(const blink::WebString& keyString); Sorry, in Chromium-style this should be key_string - my mistake.
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:88: webkit_event.domKey = static_cast<int>(event.GetDomKey()); These methods do not trap the KEY_PRESSED events that are generated via NativeWebKeyboardEvent (see native_web_keyboard_event_aura.cc line 60). They only trap the native events from the env; but the key_pressed event is generated is it not? (I'm currently looking into http://crbug.com/428018 where the old "keyIdentifier" is generated incorrectly because of the conflated windows key. I was wondering if I could change it's generation based on the dom code; but it seems that you aren't populating that at all either.
On 2015/06/18 14:28:30, Dave Tapuska wrote: > https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... > content/browser/renderer_host/web_input_event_aura.cc:88: webkit_event.domKey = > static_cast<int>(event.GetDomKey()); > These methods do not trap the KEY_PRESSED events that are generated via > NativeWebKeyboardEvent (see native_web_keyboard_event_aura.cc line 60). > > They only trap the native events from the env; but the key_pressed event is > generated is it not? Sorry, not sure what the question you're asking here is - are you talking about how keypress events are generated? > (I'm currently looking into http://crbug.com/428018 where the old > "keyIdentifier" is generated incorrectly because of the conflated windows key. I > was wondering if I could change it's generation based on the dom code; but it > seems that you aren't populating that at all either. Commented on the bug; basically the keypress/keyIdentifier behaviour is not something I think we should be spending time fixing.
On 2015/06/22 13:45:09, Wez wrote: > On 2015/06/18 14:28:30, Dave Tapuska wrote: > > > https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... > > File content/browser/renderer_host/web_input_event_aura.cc (right): > > > > > https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... > > content/browser/renderer_host/web_input_event_aura.cc:88: webkit_event.domKey > = > > static_cast<int>(event.GetDomKey()); > > These methods do not trap the KEY_PRESSED events that are generated via > > NativeWebKeyboardEvent (see native_web_keyboard_event_aura.cc line 60). > > > > They only trap the native events from the env; but the key_pressed event is > > generated is it not? > > Sorry, not sure what the question you're asking here is - are you talking about > how keypress events are generated? > > > (I'm currently looking into http://crbug.com/428018 where the old > > "keyIdentifier" is generated incorrectly because of the conflated windows key. > I > > was wondering if I could change it's generation based on the dom code; but it > > seems that you aren't populating that at all either. > > Commented on the bug; basically the keypress/keyIdentifier behaviour is not > something I think we should be spending time fixing. No I was specifically concerned that the domKey and domKeyChar won't be populated correctly because there is another constructor that is used as I identified for the key_press events that doesn't call into this code path. (I could be wrong; as I'm just currently trying to understand all this code).
My understanding of the spec is that |key| must be set for keypress events (to contain the printable character resulting from the keyboard input), but not |code|. On 22 June 2015 at 14:55, <dtapuska@chromium.org> wrote: > On 2015/06/22 13:45:09, Wez wrote: > >> On 2015/06/18 14:28:30, Dave Tapuska wrote: >> > >> > > > https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... > >> > File content/browser/renderer_host/web_input_event_aura.cc (right): >> > >> > >> > > > https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... > >> > content/browser/renderer_host/web_input_event_aura.cc:88: >> > webkit_event.domKey > >> = >> > static_cast<int>(event.GetDomKey()); >> > These methods do not trap the KEY_PRESSED events that are generated via >> > NativeWebKeyboardEvent (see native_web_keyboard_event_aura.cc line 60). >> > >> > They only trap the native events from the env; but the key_pressed >> event is >> > generated is it not? >> > > Sorry, not sure what the question you're asking here is - are you talking >> > about > >> how keypress events are generated? >> > > > (I'm currently looking into http://crbug.com/428018 where the old >> > "keyIdentifier" is generated incorrectly because of the conflated >> windows >> > key. > >> I >> > was wondering if I could change it's generation based on the dom code; >> but >> > it > >> > seems that you aren't populating that at all either. >> > > Commented on the bug; basically the keypress/keyIdentifier behaviour is >> not >> something I think we should be spending time fixing. >> > > No I was specifically concerned that the domKey and domKeyChar won't be > populated > correctly because there is another constructor that is used as I > identified for > the key_press events that doesn't call into this code path. (I could be > wrong; > as I'm just currently trying to understand all this code). > > > > https://codereview.chromium.org/929053004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks wez for your review comments. I have removed an additional variable introduced in last patch. It is in line of your suggestion of differntiating printable in non-printable in embedder API. https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/40001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:75: webkit_event.domKey = static_cast<int>(event.GetDomKey()); On 2015/06/11 00:09:05, Wez wrote: > On 2015/06/05 20:05:11, Habib Virji wrote: > > In GetUnmodifiedText it does not check the |keypress| event. > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc... > > No, but the GetCharacter() function that it delegates to returns |character_|, > which is not set for non-keypress events under Windows. Acknowledged. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1258: int keyCode = 0; On 2015/06/11 00:09:05, Wez wrote: > keyCode -> key_code > > Chromium style-guide requires unix_style names for variables, rather than > camelCaseStyle. Done. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1320: WebString::fromUTF8(code_str.data(), code_str.size()); I did try just using code_str, but apparently it is for line below web_code_str(0 i.e. to get the ascii value for the string. BTW it was not my addition, but it was added prior. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1327: text = keyCode = web_code_str.at(0); Sorry did not followed about the DOM |code| comment. Not sure about how test will behave in non-US english layout. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1340: ui::DomKey domkey = ui::DomKey::NONE; On 2015/06/11 00:09:05, Wez wrote: > nit: domkey -> dom_key, domcode -> dom_code, domkey_char -> dom_key_char > nit: Declare domKey immediately before domKeyChar, below. Done. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1345: ui::EF_NONE, &domkey, &domkey_char); Implemented as suggested. It also include now the way to differentiate DOM |Key| printable and non-printable characters, since this is the code that's called when running layout tests. https://codereview.chromium.org/929053004/diff/100001/content/child/blink_pla... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/929053004/diff/100001/content/child/blink_pla... content/child/blink_platform_impl.h:180: virtual int domEnumFromCodeString(const blink::WebString& codeString); On 2015/06/11 00:09:05, Wez wrote: > code_string (see below) Done. https://codereview.chromium.org/929053004/diff/100001/content/child/blink_pla... content/child/blink_platform_impl.h:183: virtual int domKeyEnumFromString(const blink::WebString& keyString); On 2015/06/11 00:09:05, Wez wrote: > Sorry, in Chromium-style this should be key_string - my mistake. Done. https://codereview.chromium.org/929053004/diff/120001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/120001/components/test_runner/... components/test_runner/event_sender.cc:1328: text = key_code = web_code_str.at(0); WebString is used to facilitate getting int value for the string. https://codereview.chromium.org/929053004/diff/120001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/120001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:94: webkit_event.domKey = DOM_KEY_PRINT_NON_DIFF + static_cast<int>(dom_key); Here a distinction is created if it is non-printable character. https://codereview.chromium.org/929053004/diff/120001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:289: webkit_event.domKey = static_cast<int>(event.GetCharacter()); Missed updating code here will push in new patch to update this. https://codereview.chromium.org/929053004/diff/120001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/120001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { This is to check if it is printable character. The limit is quite high to cover printable character. https://codereview.chromium.org/929053004/diff/120001/content/child/blink_pla... content/child/blink_platform_impl.cc:1398: std::string dom_key_char = base::StringPrintf("%c", dom_key); This was needed as we needed to convert int to char value. Using std::to_string will give back integer in string format. https://codereview.chromium.org/929053004/diff/120001/content/child/blink_pla... content/child/blink_platform_impl.cc:1402: static_cast<ui::DomKey>(dom_key - DOM_KEY_PRINT_NON_DIFF))); Here value is decremented to get correct mapping to enum value. https://codereview.chromium.org/929053004/diff/120001/content/child/blink_pla... content/child/blink_platform_impl.cc:1409: if (ui::DomKey::NONE == dom_key) { KeyStringToDomKey is returning DomKey::NONE if it does not find value matching in the DOM key table. https://codereview.chromium.org/929053004/diff/120001/content/child/blink_pla... content/child/blink_platform_impl.cc:1410: return key.at(0); This is to convert back printable character to int value. https://codereview.chromium.org/929053004/diff/120001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key.h (right): https://codereview.chromium.org/929053004/diff/120001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key.h:15: #define DOM_KEY_PRINT_NON_DIFF 0x400 Probably not an ideal name but wanted to cover it id printable and non-printable character differentiator.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, kpschoedel@chromium.org Link to the patchset: https://codereview.chromium.org/929053004/#ps140001 (title: "Updated MakeWebKeyboardEvent function with the changes.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929053004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Message was sent while issue was closed.
Apologise it got closed by mistake ...
Message was sent while issue was closed.
Habib, Dave, I've been meaning to write up a summary of how this is all intended to hang together, to post on chromium.org - I'll try to finish that this week to help clear up some of the issues here. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1320: WebString::fromUTF8(code_str.data(), code_str.size()); On 2015/06/24 14:32:12, Habib Virji wrote: > I did try just using code_str, but apparently it is for line below > web_code_str(0 i.e. to get the ascii value for the string. BTW it was not my > addition, but it was added prior. All that the conversion is actually doing is taking |key_code_str| in UTF-8, converting it to UTF-16 and looking at the first code-point (i.e. character) that it contains. You can do the same thing using the conversion UTF8toUTF16 function from https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/utf_s... and then pulling out the first character (if any), without needing to involve WebString. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1327: text = keyCode = web_code_str.at(0); On 2015/06/24 14:32:12, Habib Virji wrote: > Sorry did not followed about the DOM |code| comment. Sorry - I mis-read that |web_code_str| contained the DOM |code|, which would contain e.g. "KeyA", but actually it contains the |key_code_str|, which would just be 'a' or 'A'. > Not sure about how test will behave in non-US english layout. Essentially this function completely bypasses the layout, injecting events with specific |keyCode| values - now that we have |key| and |code|, though, the distinction becomes important. Basically we should re-implement this function to work out what to generate based on the DOM |code| of the key, using the US English layout routines since this is just for unit-tests. We can support the existing interface, based on |keyCode| by converting the |keyCode| down to DOM |code| at the start of the method, again using the US English layout routines, i.e: ... KeyDown(key_code_str) { if (key_code_str == "rightArrow") { event.dom_code = ... } else if (...) { ... } else { event.dom_code = UsEnglishKeyboardCodeToDomCode(...) } event.dom_key = UsEnglishDomCodeToDomKeyAndCharacter(..., dom_key_character) if (dom_key == CHARACTER) event.text = event.unmodifiedText = dom_key_character ... } WDYT? https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/100001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:88: webkit_event.domKey = static_cast<int>(event.GetDomKey()); On 2015/06/18 14:28:30, Dave Tapuska wrote: > These methods do not trap the KEY_PRESSED events that are generated via > NativeWebKeyboardEvent (see native_web_keyboard_event_aura.cc line 60). > > They only trap the native events from the env; but the key_pressed event is > generated is it not? > > (I'm currently looking into http://crbug.com/428018 where the old > "keyIdentifier" is generated incorrectly because of the conflated windows key. I > was wondering if I could change it's generation based on the dom code; but it > seems that you aren't populating that at all either. As discussed, keyIdentifier is deprecated and should be removed, and even when it was part of the DOM Level 3 spec it didn't apply to keypress events. Similarly the DOM |code| field does not apply to keypress events - I believe that |key| is currently specified as being present on keypress events, though its presence is questionable since keypress itself is a legacy-compatibility event. https://codereview.chromium.org/929053004/diff/140001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/140001/components/test_runner/... components/test_runner/event_sender.cc:1341: ui::KeyboardCode codex = ui::VKEY_UNKNOWN; nit: Why "codex"? https://codereview.chromium.org/929053004/diff/140001/components/test_runner/... components/test_runner/event_sender.cc:1347: &codex)) { In principle the output KeyboardCode should be the same as the one we're trying to generate, otherwise something is wrong with the layout conversion routines, so consider just passing &key_code here? The only difference it would make is if the caller passes in a key_code_str that can't be converted to DOM |code| and back. https://codereview.chromium.org/929053004/diff/140001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/140001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:290: webkit_event.domKey = static_cast<int>(event.GetCharacter()); As discussed elsewhere, this won't work since GetCharacter fetches |character_|, which is set only for keypress events, not for keydown or keyup events - we'll need to change that if we want to use DomKey+Character to generate the DOM |key| that we pass to WebKit. https://codereview.chromium.org/929053004/diff/140001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/140001/content/child/blink_pla... content/child/blink_platform_impl.cc:1409: if (ui::DomKey::NONE == dom_key) { This should be testing for DomKey::CHARACTER, not NONE https://codereview.chromium.org/929053004/diff/140001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key.h (right): https://codereview.chromium.org/929053004/diff/140001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key.h:15: #define DOM_KEY_PRINT_NON_DIFF 0x400 Where does this value come from? DOM |key| could contain any Unicode code-point, so surely this needs to be at least 0x110000 (one above the maximum Unicode code point)?
Message was sent while issue was closed.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Message was sent while issue was closed.
Thanks Wez I have fixed issues in event_sender.cc. In regards to GetCharacter lacking for keyDown and keyUp in Windows can this be done in a separate CL? As this CL is stopping domKey support in the browser.. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1320: WebString::fromUTF8(code_str.data(), code_str.size()); Thanks this is useful information. https://codereview.chromium.org/929053004/diff/100001/components/test_runner/... components/test_runner/event_sender.cc:1327: text = keyCode = web_code_str.at(0); I agree with your suggestion. Since unittest has very small values to check it is feasible and does not have dead keys as kevin has pointed. I have updated code based on the above suggestion. https://codereview.chromium.org/929053004/diff/140001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/140001/components/test_runner/... components/test_runner/event_sender.cc:1341: ui::KeyboardCode codex = ui::VKEY_UNKNOWN; Removed. https://codereview.chromium.org/929053004/diff/140001/components/test_runner/... components/test_runner/event_sender.cc:1347: &codex)) { The reason was key_code was not KeyboardCode, to pass to DomCodeToUsLayoutMeaning() which expects the pointer this required a temp variable. https://codereview.chromium.org/929053004/diff/140001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/140001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:290: webkit_event.domKey = static_cast<int>(event.GetCharacter()); As I understand based on your previous comments this is specific to windows, (not getting character_ for keyDown and keyUp events). Since I am working on Linux this issue is not that apparent. Can we have a follow up CL for Windows to support keyDown and keyUp events. https://codereview.chromium.org/929053004/diff/140001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/140001/content/child/blink_pla... content/child/blink_platform_impl.cc:1409: if (ui::DomKey::NONE == dom_key) { KeyStringToDomKey returns DomKey::NONE if no matching key is not found. https://codereview.chromium.org/929053004/diff/140001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key.h (right): https://codereview.chromium.org/929053004/diff/140001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key.h:15: #define DOM_KEY_PRINT_NON_DIFF 0x400 Updated to 0x110000
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:180001) has been created in https://codereview.chromium.org/1214823002/ by habib.virji@samsung.com. The reason for reverting is: This pach is still under review and was accidentally closed..
Message was sent while issue was closed.
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
On 2015/06/26 19:23:46, Habib Virji wrote: > A revert of this CL (patchset #9 id:180001) has been created in > https://codereview.chromium.org/1214823002/ by mailto:habib.virji@samsung.com. > > The reason for reverting is: This pach is still under review and was > accidentally closed.. Re-opened CL, since it wasn't supposed to land yet.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929053004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, kpschoedel@chromium.org Link to the patchset: https://codereview.chromium.org/929053004/#ps220001 (title: "UnitTest updated as MakeKeyboardEvent now adds a value to differentiate")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929053004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@wez: Thanks for reopening. There use to be option of re-opening if closed by mistake. That option has for some reason not available to me. Have updated CL. Can you PTAL.
@wez: ping
Sorry Habib, will get to this ASAP. On 22 July 2015 at 15:21, <habib.virji@samsung.com> wrote: > @wez: ping > > https://codereview.chromium.org/929053004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks again for your patience, Habib. This is looking pretty close - have suggested a tweak to the DomKey+character->int packing scheme and code layout that I think will help simplify calling code, especially where it needs to check for DomKey values. https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:94: webkit_event.domKey = DOM_KEY_PRINT_NON_DIFF + static_cast<int>(dom_key); Can you have this code re-use the same logic as the Blink string<->DomKey APIs? e.g. if you create a helper function in dom_key.h to pack a DomKey & character value into a single integer? https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:292: webkit_event.domKey = DOM_KEY_PRINT_NON_DIFF + static_cast<int>(dom_key); See above re adding a reusable helper for this. https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura_unittest.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura_unittest.cc:38: webkit_event.domKey - DOM_KEY_PRINT_NON_DIFF); As noted as per Kevin's request - if the DomKey values come first in the integer Blink DOM key value space then this test no longer needs the ugly DOM_KEY_PRINT_NON_DIFF subtraction. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { Discussing this with kpschoedel@, he prefers that the DomKey enum values come first, so that the "enum" values used here are either equal to a DomKey enum, or are out of range, in which case they represent a character. My suggestion would be to: - Make sure that DomKey::CHARACTER is the last/max DomKey enum value. < DomKey::CHARACTER -> DomKey enum >= DomKey::CHARACTER -> Unicode This way DomKey enums can be compared to these Blink DOM key enums directly, and DomKey::CHARACTER acts as a natural sentinel value. e.g. to encode a DomKey + character: int domKeyAndCharToBlinkDomKeyEnum(DomKey dom_key, int code_point) { dom_key_int = dom_key; if (dom_key == DomKey::CHARACTER) dom_key_int += code_point; return dom_key_int; } e.g. to decode a Blink DOM key to DomKey + character: DomKey blinkDomKeyEnumToDomKeyAndChar(int dom_key_int, int* code_point) { if (dom_key_int >= DomKey::CHARACTER) { *code_point = dom_key_int - DomKey::CHARACTER; return DomKey::CHARACTER; } else { return static_cast<int>(dom_key_int); } } https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1409: if (ui::DomKey::NONE == dom_key) { As noted previously, NONE means something different from CHARACTER. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1410: return key.at(0); My understanding is that WebString's are expressed in UTF16, so that at(0) returns the first 16-bit code-point - if that's true then this will break if |key| represents a character outside the Unicode Basic Multilingual Plane, such as a Chinese GB18030 character. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1412: return static_cast<int>(dom_key); Note that the way this function is written currently, you have the character values and DomKey values using the same space, from zero upwards, so you'll find that in this impl, your DomKeys won't decode properly - they'll be treated as characters. https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key.h (right): https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key.h:15: #define DOM_KEY_PRINT_NON_DIFF 0x110000 See comments on Blink API impl - I don't think you need this, if you move CHARACTER to be the maximum DomKey enum value. https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom_... File ui/events/keycodes/dom_us_layout_data.h (right): https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom_... ui/events/keycodes/dom_us_layout_data.h:8: #include "ui/events/keycodes/keyboard_codes.h" Do you need this? https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_code_conversion.cc (right): https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_code_conversion.cc:21: ui::KeyboardCode key_code; ?
https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { Apologies for being away from this for a while. Given Gary's clarification of the UI Events spec it's probably easier that I had previously thought to migrate |ui::DomKey| to a combined Unicode/non-printable code. I'm going to look at that over the next few days and see whether there are any obstacles elsewhere in Chrome.
https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { On 2015/07/29 14:43:14, kpschoedel wrote: > Apologies for being away from this for a while. > > Given Gary's clarification of the UI Events spec it's probably easier that I had > previously thought to migrate |ui::DomKey| to a combined Unicode/non-printable > code. I'm going to look at that over the next few days and see whether there are > any obstacles elsewhere in Chrome. > > SGTM - thanks Kevin!
https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { On 2015/07/25 23:12:26, Wez wrote: > Discussing this with kpschoedel@, he prefers that the DomKey enum values come > first, so that the "enum" values used here are either equal to a DomKey enum, or > are out of range, in which case they represent a character. I'm now thinking the opposite for a unified ui::DomKey implementation — I'd like to preserve the existing usage of ui::DomKey as the type and ui::DomKey::NAME as enumerated value, to match C++ 'enum class' types including ui::DomCode, but to allow non-enumerated values (i.e. Unicode characters) means giving up the type safety of 'enum class', and so I think it's safer to have the non-Unicode key constants outside the Unicode range.
https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { On 2015/07/29 19:16:11, kpschoedel wrote: > On 2015/07/25 23:12:26, Wez wrote: > > Discussing this with kpschoedel@, he prefers that the DomKey enum values come > > first, so that the "enum" values used here are either equal to a DomKey enum, > or > > are out of range, in which case they represent a character. > > I'm now thinking the opposite for a unified ui::DomKey implementation — I'd like > to preserve the existing usage of ui::DomKey as the type and ui::DomKey::NAME as > enumerated value, to match C++ 'enum class' types including ui::DomCode, but to > allow non-enumerated values (i.e. Unicode characters) means giving up the type > safety of 'enum class', and so I think it's safer to have the non-Unicode key > constants outside the Unicode range. I'm not sure that type-safety is really an issue here, since the current DomKey enum doesn't cover the Unicode code-points at all. It may be better to swap DomKey out for a plain old int, and provide inline static functions to generate the opaque integer values corresponding to a Unicode code point, or a non-printable DOM key identifier, i.e. promote Habib's Blink DOM key enum APIs to be a core part of ui/, and perhaps just provide constants for the non-printable DOM key for convenience, and a domkeyFromUnicode() helper in case of printable-character DOM key values. WDYT?
On 2015/07/29 19:59:48, Wez wrote: > I'm not sure that type-safety is really an issue here, since the current DomKey > enum doesn't cover the Unicode code-points at all. I may be overly cautious here due to working with old code that treated VKEY and character values as interchangeable, when they aren't; in the DomKey case it looks practical to make key effectively a superset of Unicode. > It may be better to swap DomKey out for a plain old int, and provide inline > static functions to generate the opaque integer values corresponding to a > Unicode code point, or a non-printable DOM key identifier, i.e. promote Habib's > Blink DOM key enum APIs to be a core part of ui/, and perhaps just provide > constants for the non-printable DOM key for convenience, and a > domkeyFromUnicode() helper in case of printable-character DOM key values. I'm currently working on trying a DomKey interchangeable with int (actually int32_t for standard/style conformance — not that we'll ever port Chromium to MS-DOS). The //ui code does have some use cases that Blink doesn't care about directly, like distinguishing dead keys, in order to get the benefit of replacing the current (key, character) tuple; DomKeyToKeyString() et al would hide that at the web level.
On 30 July 2015 at 07:16, <kpschoedel@chromium.org> wrote: > On 2015/07/29 19:59:48, Wez wrote: > >> I'm not sure that type-safety is really an issue here, since the current >> > DomKey > >> enum doesn't cover the Unicode code-points at all. >> > > I may be overly cautious here due to working with old code that treated > VKEY and > character values as interchangeable, when they aren't; in the DomKey case > it > looks practical to make key effectively a superset of Unicode. Ah, that sounds like the crazy legacy-DOM-compatibility hack of having keyCode mean different things between keydown/keyup and keypress events... > > > It may be better to swap DomKey out for a plain old int, and provide inline >> static functions to generate the opaque integer values corresponding to a >> Unicode code point, or a non-printable DOM key identifier, i.e. promote >> > Habib's > >> Blink DOM key enum APIs to be a core part of ui/, and perhaps just provide >> constants for the non-printable DOM key for convenience, and a >> domkeyFromUnicode() helper in case of printable-character DOM key values. >> > > I'm currently working on trying a DomKey interchangeable with int (actually > int32_t for standard/style conformance — not that we'll ever port Chromium > to > MS-DOS). The //ui code does have some use cases that Blink doesn't care > about > directly, like distinguishing dead keys, in order to get the benefit of > replacing the current (key, character) tuple; DomKeyToKeyString() et al > would > hide that at the web level. > Cool. Look forward to seeing how that works out. > > https://codereview.chromium.org/929053004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks Wez and Kevin, please find update based on using DomKey::CHARACTER as way of differentiating DomKey unicode and enum values. https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:94: webkit_event.domKey = DOM_KEY_PRINT_NON_DIFF + static_cast<int>(dom_key); On 2015/07/25 23:12:26, Wez wrote: > Can you have this code re-use the same logic as the Blink string<->DomKey APIs? > e.g. if you create a helper function in dom_key.h to pack a DomKey & character > value into a single integer? Done. Have introduced GetDomKeyOrCharacter() to get the corresponding value. https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:292: webkit_event.domKey = DOM_KEY_PRINT_NON_DIFF + static_cast<int>(dom_key); On 2015/07/25 23:12:26, Wez wrote: > See above re adding a reusable helper for this. Done. https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura_unittest.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura_unittest.cc:38: webkit_event.domKey - DOM_KEY_PRINT_NON_DIFF); On 2015/07/25 23:12:26, Wez wrote: > As noted as per Kevin's request - if the DomKey values come first in the integer > Blink DOM key value space then this test no longer needs the ugly > DOM_KEY_PRINT_NON_DIFF subtraction. Done. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1397: if (dom_key < DOM_KEY_PRINT_NON_DIFF) { It is a good suggestion. In the code snippet though we receive only one value. So codepoint cannot be used. Have updated code based on one value. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1409: if (ui::DomKey::NONE == dom_key) { On 2015/07/25 23:12:26, Wez wrote: > As noted previously, NONE means something different from CHARACTER. Done. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1410: return key.at(0); Have changed to using StringToInt as it appears to be better alternate to at.. https://codereview.chromium.org/929053004/diff/220001/content/child/blink_pla... content/child/blink_platform_impl.cc:1412: return static_cast<int>(dom_key); With the update checking DomKey::CHARACTER it should fix this issue. https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key.h (right): https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key.h:15: #define DOM_KEY_PRINT_NON_DIFF 0x110000 This is removed now. https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom_... File ui/events/keycodes/dom_us_layout_data.h (right): https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/dom_... ui/events/keycodes/dom_us_layout_data.h:8: #include "ui/events/keycodes/keyboard_codes.h" Yes, as VKEY is defined in keyboard_codes.h. https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_code_conversion.cc (right): https://codereview.chromium.org/929053004/diff/220001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_code_conversion.cc:21: ui::KeyboardCode key_code; On 2015/07/25 23:12:26, Wez wrote: > ? Removed
The CQ bit was checked by habib.virji@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929053004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/929053004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@wez/kevin: ping
On 2015/08/05 09:45:46, Habib Virji wrote: > @wez/kevin: ping Apologies, this week has a holiday and an event for me, so I'm behind; will resume on Friday.
https://codereview.chromium.org/929053004/diff/260001/components/test_runner/... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/929053004/diff/260001/components/test_runner/... components/test_runner/event_sender.cc:1265: std::string key_code_str; I'm not sure we should use a std::string here; what is the purpose? As this structure gets created each time this method is executed. Are we intended on that? I guess this is only the test harness; but this code might get copied; a const char* might be better to avoid the malloc/free's. https://codereview.chromium.org/929053004/diff/260001/components/test_runner/... components/test_runner/event_sender.cc:1298: for (int i = 1; i <= 24; ++i) { This would benefit of a prefix match on F... https://codereview.chromium.org/929053004/diff/260001/components/test_runner/... components/test_runner/event_sender.cc:1308: base::char16 str = static_cast<base::char16>(key_code_str.at(0)); This doesn't need to be in the loop; it is const for all iterations... also str seems like a bad variable name. Likewise .at(0) is going to return you the first variable of the utf8 encoded value won't it? Not a char16... I know the event sender looks like it doesn't support non-US; but if these are known issues; they at least need a TODO... https://codereview.chromium.org/929053004/diff/260001/content/child/blink_pla... File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/929053004/diff/260001/content/child/blink_pla... content/child/blink_platform_impl.cc:1377: static_cast<ui::DomKey>(dom_key))); This cast is undefined when dom_key is less than 0. Any idea why we didn't add dom_key as a uint? In understand this is a virtual specified in the blink API; I'm just commenting how I would have written it. https://codereview.chromium.org/929053004/diff/260001/content/child/blink_pla... content/child/blink_platform_impl.cc:1379: std::string dom_key_char = base::StringPrintf("%c", Does StringPrintf work for unicode characters? ie; I don't believe it does an int -> utf8 conversion. I could be wrong; but I tried it in content_shell and it prints out the incorrect value for non-ascii values. https://codereview.chromium.org/929053004/diff/260001/content/child/blink_pla... content/child/blink_platform_impl.cc:1390: base::StringToInt(key.utf8().data(), &dom_char); Can we add an ASSERT that this doesn't fail? https://codereview.chromium.org/929053004/diff/260001/ui/events/keycodes/dom/... File ui/events/keycodes/dom/dom_key_data.inc (right): https://codereview.chromium.org/929053004/diff/260001/ui/events/keycodes/dom/... ui/events/keycodes/dom/dom_key_data.inc:381: DOM_KEY_MAP(nullptr, CHARACTER), Should a comment be added here that CHARACTER is assumed to be the last item in the enumeration? Or should there be a special define for the end of the enumeration that matches the last most item instead of referring to CHARACTER everywhere?
I have no objections in principle to this and will make the necessary revisions if I land a unified ui::DomKey later (e.g. https://codereview.chromium.org/1284433002/ or otherwise)
Looks like crrev.com/1284433002 looks pretty close to being complete. Habib, I think Kevin's patch to unify the character & non-character codes should greatly simplify this CL!
On 2015/08/26 23:42:41, Wez wrote: > Looks like crrev.com/1284433002 looks pretty close to being complete. > > Habib, I think Kevin's patch to unify the character & non-character codes should > greatly simplify this CL! @Habib; please let us know if you have time to revise this based on Kevin's new change. I'd like to get these moving in M47.
On 2015/08/27 13:23:06, dtapuska wrote: > On 2015/08/26 23:42:41, Wez wrote: > > Looks like crrev.com/1284433002 looks pretty close to being complete. > > > > Habib, I think Kevin's patch to unify the character & non-character codes > should > > greatly simplify this CL! > > @Habib; please let us know if you have time to revise this based on Kevin's new > change. I'd like to get these moving in M47. I will start looking into kevin changes. Will address your comments too in the push. When is M47 scheduled?
On 2015/08/27 16:29:12, Habib Virji wrote: > On 2015/08/27 13:23:06, dtapuska wrote: > > On 2015/08/26 23:42:41, Wez wrote: > > > Looks like crrev.com/1284433002 looks pretty close to being complete. > > > > > > Habib, I think Kevin's patch to unify the character & non-character codes > > should > > > greatly simplify this CL! > > > > @Habib; please let us know if you have time to revise this based on Kevin's > new > > change. I'd like to get these moving in M47. > > I will start looking into kevin changes. Will address your comments too in the > push. When is M47 scheduled? M47 is our current sprint; Oct 2nd branch date. I hope that your change should turn into a few lines with Kevin's changes (haven't landed yet).
On 2015/08/27 16:41:46, dtapuska wrote: > On 2015/08/27 16:29:12, Habib Virji wrote: > > On 2015/08/27 13:23:06, dtapuska wrote: > > > On 2015/08/26 23:42:41, Wez wrote: > > > > Looks like crrev.com/1284433002 looks pretty close to being complete. > > > > > > > > Habib, I think Kevin's patch to unify the character & non-character codes > > > should > > > > greatly simplify this CL! > > > > > > @Habib; please let us know if you have time to revise this based on Kevin's > > new > > > change. I'd like to get these moving in M47. > > > > I will start looking into kevin changes. Will address your comments too in the > > push. When is M47 scheduled? > > M47 is our current sprint; Oct 2nd branch date. > I hope that your change should turn into a few lines with Kevin's changes > (haven't landed yet). I put a subsequent change up for review: https://codereview.chromium.org/1337503002/ It is so that I can continue getting Android and Mac supported for domCode and domKey.. I'll leave aura for last in case you want to do it.
On 2015/09/10 15:51:12, dtapuska wrote: > On 2015/08/27 16:41:46, dtapuska wrote: > > On 2015/08/27 16:29:12, Habib Virji wrote: > > > On 2015/08/27 13:23:06, dtapuska wrote: > > > > On 2015/08/26 23:42:41, Wez wrote: > > > > > Looks like crrev.com/1284433002 looks pretty close to being complete. > > > > > > > > > > Habib, I think Kevin's patch to unify the character & non-character > codes > > > > should > > > > > greatly simplify this CL! > > > > > > > > @Habib; please let us know if you have time to revise this based on > Kevin's > > > new > > > > change. I'd like to get these moving in M47. > > > > > > I will start looking into kevin changes. Will address your comments too in > the > > > push. When is M47 scheduled? > > > > M47 is our current sprint; Oct 2nd branch date. > > I hope that your change should turn into a few lines with Kevin's changes > > (haven't landed yet). > > I put a subsequent change up for review: > https://codereview.chromium.org/1337503002/ > > It is so that I can continue getting Android and Mac supported for domCode and > domKey.. I'll leave aura for last in case you want to do it. Does that mean this CL can be closed?
Message was sent while issue was closed.
On 2015/09/14 22:05:29, Wez wrote: > On 2015/09/10 15:51:12, dtapuska wrote: > > On 2015/08/27 16:41:46, dtapuska wrote: > > > On 2015/08/27 16:29:12, Habib Virji wrote: > > > > On 2015/08/27 13:23:06, dtapuska wrote: > > > > > On 2015/08/26 23:42:41, Wez wrote: > > > > > > Looks like crrev.com/1284433002 looks pretty close to being complete. > > > > > > > > > > > > Habib, I think Kevin's patch to unify the character & non-character > > codes > > > > > should > > > > > > greatly simplify this CL! > > > > > > > > > > @Habib; please let us know if you have time to revise this based on > > Kevin's > > > > new > > > > > change. I'd like to get these moving in M47. > > > > > > > > I will start looking into kevin changes. Will address your comments too in > > the > > > > push. When is M47 scheduled? > > > > > > M47 is our current sprint; Oct 2nd branch date. > > > I hope that your change should turn into a few lines with Kevin's changes > > > (haven't landed yet). > > > > I put a subsequent change up for review: > > https://codereview.chromium.org/1337503002/ > > > > It is so that I can continue getting Android and Mac supported for domCode and > > domKey.. I'll leave aura for last in case you want to do it. > > Does that mean this CL can be closed? I've closed the issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
