Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2580483002:
[Linux] Ctrl should be considered as system modifier and not producing characters (Closed)
Description[Linux] Ctrl should be considered as system modifier and not producing characters
|KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not
necessarily the character to be produced by the event.
e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t".
|Textfield| (and other components) relies on the system modifier check to detect
whether they should put a character, and this CL adds Ctrl key as an additional filter.
This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a
printable character.
For Windows:
1. We can't do it because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard
2. It's currently not breaking because Windows native 'keypress' has different |key| value < 0x20
For Mac:
1. Mac doesn't have this issue and it seems to be using a different approach in
|RenderWidgetHostViewMac::insertText|
Related issue https://crbug.com/633838
BUG=673302
Review-Url: https://codereview.chromium.org/2580483002
Cr-Commit-Position: refs/heads/master@{#450387}
Committed: https://chromium.googlesource.com/chromium/src/+/4127317ca378c1cd8be4dd09428f0dd232888f68
Patch Set 1 #Patch Set 2 : dtapuska's review: Add IsControlKeyModifier() instead of changing system modifier #
Total comments: 18
Patch Set 3 : msw's comments #
Total comments: 8
Patch Set 4 : msw's comments 2 #
Total comments: 2
Patch Set 5 : msw's comment 3: Add comments for ctrl modifier on Linux #
Messages
Total messages: 60 (42 generated)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [X11] |KeyEvent::GetCharacter()| should return 0 when Ctrl is down |KeyEvent::GetCharacter()| is getting character from DomKey, however according to spec DomKey for Ctrl-t should be "t", while the actual produced character should be 0. This CL added a filter in |KeyEvent::GetCharacter()| so we return 0 when Ctrl is down. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [X11] Ctrl should be considered as a system modifier and shouldn't produce a character |KeyEvent::GetCharacter()| is getting character from DomKey, however according to spec DomKey for Ctrl-t should be "t", while the actual produced character should be 0. This CL added a filter in |KeyEvent::GetCharacter()| so we return 0 when Ctrl is down. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [X11] Ctrl should be considered as a system modifier and shouldn't produce a character |KeyEvent::GetCharacter()| is getting character from DomKey, however according to spec DomKey for Ctrl-t should be "t", while the actual produced character should be 0. This CL added a filter in |KeyEvent::GetCharacter()| so we return 0 when Ctrl is down. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not the necessary the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key to X11's system modifier mask. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ==========
Description was changed from ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not the necessary the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key to X11's system modifier mask. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key to X11's system modifier mask. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ==========
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ PTAL, thanks!
On 2016/12/19 20:46:56, chongz wrote: > dtapuska@ PTAL, thanks! I'm not sure if this is right... Typically I think the system modifier on linux is just alt. Perhaps the function needs a rename if you are changing it's semantics.
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chongz@chromium.org changed reviewers: + sadrul@chromium.org
> I'm not sure if this is right... Typically I think the system modifier on linux > is just alt. Perhaps the function needs a rename if you are changing it's > semantics. I've added a new function |IsControlKeyModifier()| for Ctrl check. sadrul@ Can you take a look at this CL please? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@chromium.org changed reviewers: + kpschoedel@chromium.org
+kpschoedel@
On 2016/12/20 17:43:41, sadrul wrote: > +kpschoedel@ kpschoedel@ Please take a look, thanks!
LGTM, but I'm not especially familiar with this code.
chongz@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@ PTAL at "ui/views/controls/textfield/textfield.cc", thanks!
It's not obvious to me why this change is X11-specific. The CL description justifies this with an argument that seems like it would apply across platforms. I suggest msw rather than me as OWNER for this, he has more familiarity in this area.
sadrul@chromium.org changed reviewers: + msw@chromium.org - sadrul@chromium.org
--> msw@ for owner
I have more questions than helpful advice; sorry. +CC afakhry, oshima, and avi (per related https://codereview.chromium.org/1325333002 and for increased visibility, hoping someone understands nuances here better than I do) https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:219: #if defined(USE_X11) I share Peter's confusion about the X11-specific nature of this fix. Can you elaborate on why the CTRL key is only treated as a modifier there? It naively seems like CTRL should have been treated as a modifier by views::Textfield all along (on Windows as well); why hasn't this come up before? Why doesn't CTRL+M currently produce 'm' in a textfield (or similar for unbound CTRL+CHAR combos)? https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1286: // Filter out all control characters, including tab and new line characters, nit: it'd be nice to update this comment if it's out of date. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:766: #if defined(USE_X11) Does this pass on other platforms? Enable it as broadly as possible. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:770: SendKeyEvent(0x0448, 0); nit: comment that this is the code for 'CYRILLIC SMALL LETTER SHA' https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:771: SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN); Can we also test CTRL+M or some unbound CTRL+[EN_US-CHAR]? https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:771: SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN); Instead of passing ui::EF_CONTROL_DOWN, should this use the SendKeyEvent overload that takes a |control_or_command| flag to support mac testing? https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:777: #if defined(OS_WIN) || defined(OS_MACOSX) Can we run this test on Linux and Chrome OS? It seems related.
Description was changed from ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key to X11's system modifier mask. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character, we can't & don't need to do this for Windows because: 1. Ctrl+Shift+2 on Windows should insert ZWNJ with Persian Keyboard 2. We won't enter here on Windows if there is no WM_CHAR message Related issue https://crbug.com/633838 BUG=673302 ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character, we can't & don't need to do this for Windows because: 1. Ctrl+Shift+2 on Windows should insert ZWNJ with Persian Keyboard 2. We won't enter here on Windows if there is no WM_CHAR message Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. For Windows: 1. We can't do it because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard 2. It's currently not breaking because Windows native 'keypress' has different |key| value < 0x20 For Mac: 1. Mac doesn't have this issue and it seems to be using a different approach in |RenderWidgetHostViewMac::insertText| Related issue https://crbug.com/633838 BUG=673302 ==========
Description was changed from ========== [X11] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. For Windows: 1. We can't do it because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard 2. It's currently not breaking because Windows native 'keypress' has different |key| value < 0x20 For Mac: 1. Mac doesn't have this issue and it seems to be using a different approach in |RenderWidgetHostViewMac::insertText| Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [Linux] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. For Windows: 1. We can't do it because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard 2. It's currently not breaking because Windows native 'keypress' has different |key| value < 0x20 For Mac: 1. Mac doesn't have this issue and it seems to be using a different approach in |RenderWidgetHostViewMac::insertText| Related issue https://crbug.com/633838 BUG=673302 ==========
msw@ Thanks for the review and sorry for the delay. I think we can extend the filter to cover Chrome OS but not other platforms, PTAL, thanks! https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:219: #if defined(USE_X11) On 2017/01/11 19:36:37, msw wrote: > I share Peter's confusion about the X11-specific nature of this fix. Can you > elaborate on why the CTRL key is only treated as a modifier there? It naively > seems like CTRL should have been treated as a modifier by views::Textfield all > along (on Windows as well); why hasn't this come up before? Why doesn't CTRL+M > currently produce 'm' in a textfield (or similar for unbound CTRL+CHAR combos)? Sorry for the confusion, I've updated CL description and changed the macro to cover Linux. Linux: This bug was introduced by me in https://crrev.com/2474083002 @ M56 Before: 1. When ctrl is down we return a |key| based on US layout, which is wrong (e.g. Always return 'a'-'z'). 2. However 'a'-'z' will be turned into control characters < 0x20 in |KeyEvent::GetCharacter():1272|, so this bug didn't happen before After: 1. We return the same |key| with/without ctrl, including unicode characters ChromeOS: I don't have the latest version but M53 has this issue as well. e.g. Ctrl-m on Russian keyboard prints an unicode character. Windows: 1. Windows doesn't have this issue, I think it's because the native 'keypress' events has |key| < 0x20, which is different from 'keydown' events. (On Linux 'keypress' and 'keydown' has the same |key|) 2. We don't have a spec for 'keypress' so I'm not sure what |key| we should be using. 3. We can't add this filter because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard Mac: Mac doesn't have this issue, I'd assume it has a different approach in |RenderWidgetHostViewMac::insertText| https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1286: // Filter out all control characters, including tab and new line characters, On 2017/01/11 19:36:37, msw wrote: > nit: it'd be nice to update this comment if it's out of date. Done. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:766: #if defined(USE_X11) On 2017/01/11 19:36:37, msw wrote: > Does this pass on other platforms? Enable it as broadly as possible. Expanded to OS_Linux. This test should pass on both Linux and ChromeOS. However it won't pass on Mac as |SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN)| will always clear the textfield and I have no idea why. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:770: SendKeyEvent(0x0448, 0); On 2017/01/11 19:36:37, msw wrote: > nit: comment that this is the code for 'CYRILLIC SMALL LETTER SHA' Done. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:771: SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN); On 2017/01/11 19:36:37, msw wrote: > Can we also test CTRL+M or some unbound CTRL+[EN_US-CHAR]? Done. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:771: SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN); On 2017/01/11 19:36:37, msw wrote: > Instead of passing ui::EF_CONTROL_DOWN, should this use the SendKeyEvent > overload that takes a |control_or_command| flag to support mac testing? I failed to make this test pass on Mac. https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:777: #if defined(OS_WIN) || defined(OS_MACOSX) On 2017/01/11 19:36:37, msw wrote: > Can we run this test on Linux and Chrome OS? It seems related. |ScopedKeyboardLayout| doesn't have implementation on Linux and Chrome OS yet, see: https://cs.chromium.org/chromium/src/ui/events/test/keyboard_layout.h?sq=pack...
https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:219: #if defined(USE_X11) On 2017/01/13 16:08:38, chongz wrote: > On 2017/01/11 19:36:37, msw wrote: > > I share Peter's confusion about the X11-specific nature of this fix. Can you > > elaborate on why the CTRL key is only treated as a modifier there? It naively > > seems like CTRL should have been treated as a modifier by views::Textfield all > > along (on Windows as well); why hasn't this come up before? Why doesn't CTRL+M > > currently produce 'm' in a textfield (or similar for unbound CTRL+CHAR > combos)? > > Sorry for the confusion, I've updated CL description and changed the macro to > cover Linux. > > Linux: > This bug was introduced by me in https://crrev.com/2474083002 @ M56 > Before: > 1. When ctrl is down we return a |key| based on US layout, which is wrong > (e.g. Always return 'a'-'z'). > 2. However 'a'-'z' will be turned into control characters < 0x20 in > |KeyEvent::GetCharacter():1272|, so this bug didn't happen before > After: > 1. We return the same |key| with/without ctrl, including unicode characters > > ChromeOS: > I don't have the latest version but M53 has this issue as well. e.g. Ctrl-m on > Russian keyboard prints an unicode character. > > Windows: > 1. Windows doesn't have this issue, I think it's because the native 'keypress' > events has |key| < 0x20, which is different from 'keydown' events. (On Linux > 'keypress' and 'keydown' has the same |key|) > 2. We don't have a spec for 'keypress' so I'm not sure what |key| we should be > using. > 3. We can't add this filter because Ctrl+Shift+2 should insert ZWNJ with Persian > Keyboard > > Mac: > Mac doesn't have this issue, I'd assume it has a different approach in > |RenderWidgetHostViewMac::insertText| Are Linux users with other keyboard layouts still able to easily use accelerators after your prior CL? (eg. if they previously hit CTRL+'à' to select-all (converted to a control character in step 2 before), does that still work? Line 112 in this file specifically looks for ui::VKEY_A, so do users with certain layouts need to go out of their way to type CTRL+'a' to use that accelerators now?) https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:766: #if defined(USE_X11) On 2017/01/13 16:08:38, chongz wrote: > On 2017/01/11 19:36:37, msw wrote: > > Does this pass on other platforms? Enable it as broadly as possible. > > Expanded to OS_Linux. This test should pass on both Linux and ChromeOS. > > However it won't pass on Mac as |SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN)| will > always clear the textfield and I have no idea why. Can you use another character besides 0x0448? It'd be nice to run everywhere. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1292: // and all characters with Alt modifier (and Search on ChromeOS, Ctrl on Is it safe to consider the control key a modifier on all platforms here? It seems like a quirk that Windows (and Mac?) already filters out control characters from this function; so perhaps we shouldn't have a platform-specific check here, but skip any event on any platform here if |event.flags() & ui::EF_CONTROL_DOWN != 0|. The less platform-specific code we have in this class, the better. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:771: #if defined(OS_LINUX) Can we also run this on Windows? The less platform-specific, the better. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:781: SendKeyEvent(ui::VKEY_I, false, false); nit: please use your new SendKeyEvent overload throughout this test. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:786: EXPECT_EQ(WideToUTF16(L"\x0448\x044C\x0069\x006D"), textfield_->text()); nit: for readability, please use the following literals: WideToUTF16(L"\x0448\x044C" L"im")
https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:219: #if defined(USE_X11) To make a long story short… Personal background note: when I was on Chrome, I implemented a good part of the keyboard UI Events spec <https://www.w3.org/TR/uievents/> at the ui::Event level as part of the Ozone project; I still know that pretty well but not so much views:: or higher levels. Web background note: 'legacy' web keyboard events were pretty much exactly Windows events, and Chrome uses these heavily internally, in keeping with its Windows origin. Other platforms have to emulate Windows events, which is complicated (see below), necessarily approximate (ditto), and error prone. (My opinion, after all that, is that Chrome should internally transition away from Windows-style events to UIEvents-style events, except when actually exposing legacy events to web code, or the whack-a-mole bugs like this one will never end.) > I share Peter's confusion about the X11-specific nature of this fix. Can you > elaborate on why the CTRL key is only treated as a modifier there? It naively > seems like CTRL should have been treated as a modifier by views::Textfield all > along (on Windows as well); why hasn't this come up before? Why doesn't CTRL+M > currently produce 'm' in a textfield (or similar for unbound CTRL+CHAR combos)? On X11 (or XKB generally, i.e. including Wayland and ChromeOS/Ozone), and also on OS X, Control is ‘just another modifier’ that is not fundamentally different from Shift or AltGr — and Control+a is (typically, but layout-dependently) ASCII SOH in the same sense that Shift+a is (typically, but layout-dependently) capital 'A'. On Windows, Control is a system shortcut key, and Control+a is just 'a' with a flag. There is inherent conflict between the respect-the-user's-selected-layout view — which on *nix means that Control+M and Enter both generate CR just like the key above Q and the numpad key both generate '1' — and the WebWindows view — which means that it generates 'm'. Somebody's going to be unhappy. > Are Linux users with other keyboard layouts still able to easily use > accelerators after your prior CL? (eg. if they previously hit CTRL+'à' to > select-all (converted to a control character in step 2 before), does that still > work? Line 112 in this file specifically looks for ui::VKEY_A, so do users with > certain layouts need to go out of their way to type CTRL+'a' to use that > accelerators now?) It depends on the current keyboard layout. Recall that VKEY is a synthetic Windows-emulating property. If the XKB layout is sufficiently similar to a Windows keyboard layout that keyboard_code_conversion_x.cc recognizes a correspondence, then the VKEY will be the same as on the ‘equivalent’ Windows layout. Otherwise, the VKEY will be that which the same *physical* key produces on Windows' US English layout. This attempts to emulate Windows' fallback style where e.g. 'Й' is VKEY_Q. None of that has (intentionally) changed in living memory. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1292: // and all characters with Alt modifier (and Search on ChromeOS, Ctrl on On 2017/01/13 18:15:50, msw wrote: > Is it safe to consider the control key a modifier on all platforms here? It > seems like a quirk that Windows (and Mac?) already filters out control > characters from this function; so perhaps we shouldn't have a platform-specific > check here, but skip any event on any platform here if |event.flags() & > ui::EF_CONTROL_DOWN != 0|. The less platform-specific code we have in this > class, the better. There's nothing inherently preventing an XKB layout from generating printable characters from a Control-modified key combination, although no layout in the upstream xkeyboard-config package that most distributions (and ChromeOS) use does so. Chrome would break with such a layout in other ways, anyway.
pkasting@chromium.org changed reviewers: - pkasting@chromium.org
msw@ just an update: Are you convinced by kpschoedel@'s comments? https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2580483002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:766: #if defined(USE_X11) On 2017/01/13 18:15:50, msw wrote: > On 2017/01/13 16:08:38, chongz wrote: > > On 2017/01/11 19:36:37, msw wrote: > > > Does this pass on other platforms? Enable it as broadly as possible. > > > > Expanded to OS_Linux. This test should pass on both Linux and ChromeOS. > > > > However it won't pass on Mac as |SendKeyEvent(0x0448, ui::EF_CONTROL_DOWN)| > will > > always clear the textfield and I have no idea why. > > Can you use another character besides 0x0448? It'd be nice to run everywhere. No, I've tried all Russian characters but they don't work on Mac. Besides I don't think we should extend it to other platforms as Control has different meanings and behaviors on each platform. Please see kpschoedel@'s comments for more details. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:771: #if defined(OS_LINUX) On 2017/01/13 18:15:51, msw wrote: > Can we also run this on Windows? The less platform-specific, the better. I don't think we should extend it to other platforms as Control has different meanings and behaviors on each platform. Please see kpschoedel@'s comments for more details. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:781: SendKeyEvent(ui::VKEY_I, false, false); On 2017/01/13 18:15:51, msw wrote: > nit: please use your new SendKeyEvent overload throughout this test. Done. https://codereview.chromium.org/2580483002/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:786: EXPECT_EQ(WideToUTF16(L"\x0448\x044C\x0069\x006D"), textfield_->text()); On 2017/01/13 18:15:51, msw wrote: > nit: for readability, please use the following literals: > WideToUTF16(L"\x0448\x044C" L"im") Done.
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with a nit; +CC sadrul for event code sanity checking https://codereview.chromium.org/2580483002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:219: bool IsControlKeyModifier(int flags) { nit: Add a comment. It won't be obvious to future readers why this function only recognizes the control flag on Linux.
https://codereview.chromium.org/2580483002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2580483002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:219: bool IsControlKeyModifier(int flags) { On 2017/02/13 18:05:42, msw wrote: > nit: Add a comment. It won't be obvious to future readers why this function only > recognizes the control flag on Linux. Done.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kpschoedel@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2580483002/#ps120001 (title: "msw's comment 3: Add comments for ctrl modifier on Linux")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487088041318030,
"parent_rev": "58084dd8f1d79ce115ee976daa4f1d6b8d51481b", "commit_rev":
"4127317ca378c1cd8be4dd09428f0dd232888f68"}
Message was sent while issue was closed.
Description was changed from ========== [Linux] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. For Windows: 1. We can't do it because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard 2. It's currently not breaking because Windows native 'keypress' has different |key| value < 0x20 For Mac: 1. Mac doesn't have this issue and it seems to be using a different approach in |RenderWidgetHostViewMac::insertText| Related issue https://crbug.com/633838 BUG=673302 ========== to ========== [Linux] Ctrl should be considered as system modifier and not producing characters |KeyEvent::GetCharacter()| returns the character value of the DomKey, which is not necessarily the character to be produced by the event. e.g. "Ctrl+t" doesn't produce a printable character, but the DomKey is "t". |Textfield| (and other components) relies on the system modifier check to detect whether they should put a character, and this CL adds Ctrl key as an additional filter. This is based on the fact that XKB doesn't have a combination with Ctrl that maps to a printable character. For Windows: 1. We can't do it because Ctrl+Shift+2 should insert ZWNJ with Persian Keyboard 2. It's currently not breaking because Windows native 'keypress' has different |key| value < 0x20 For Mac: 1. Mac doesn't have this issue and it seems to be using a different approach in |RenderWidgetHostViewMac::insertText| Related issue https://crbug.com/633838 BUG=673302 Review-Url: https://codereview.chromium.org/2580483002 Cr-Commit-Position: refs/heads/master@{#450387} Committed: https://chromium.googlesource.com/chromium/src/+/4127317ca378c1cd8be4dd09428f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4127317ca378c1cd8be4dd09428f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
