[NOT FOR COMMIT] Ignore inserting characters of Keys with modifiers on CrOS

Ignore inserting characters of Keys with modifiers on CrOS

User-defined extension commands (chrome://extensions/configureCommands)
that are set to Search+Key, when pressed while a textfield is in focus,
the Key is inserted in the textfield before the command is executed.

On CrOs, we want to treat Search/Ctrl/Alt as modifiers that prevent the
characters from being inserted in the textfield.

R=oshima@chromium.org
BUG=471488
TEST=views_unittests --gtest_filter=TextfieldTest.KeysWithModifiersTest

[afakhry, 2015-08-14 20:18:55]

oshima@ Please review when you're back from vacation. :) Thanks!

https://codereview.chromium.org/1221083008/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1221083008/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1591: #if
defined(OS_CHROMEOS)
I'm not sure if we want to ignore keys with modifiers on other platforms too?
For now I'm just doing it for CrOs.

[Shu Chen, 2015-08-17 03:27:39]

shuchen@chromium.org changed reviewers:
+ shuchen@chromium.org

[Shu Chen, 2015-08-17 03:27:40]

I think this cl is going to the wrong way.

By reading crbug.com/471488, I think the root cause is that "Search+M" is
interpret as a command but the key event isn't marked as stopped_propagation()
after processing.
Please refer to
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_....

And please also refer to the bug thread: crbug.com/508668.

[afakhry, 2015-08-18 18:35:53]

On 2015/08/17 03:27:40, Shu Chen wrote:
> I think this cl is going to the wrong way.
> 
> By reading crbug.com/471488, I think the root cause is that "Search+M" is
> interpret as a command but the key event isn't marked as stopped_propagation()
> after processing.
> Please refer to
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_....
> 
> And please also refer to the bug thread: crbug.com/508668.

This CL is trying to enforce treating the Search key as a modifier like Alt and
Ctrl regardless of whether Search+Key is tied to a command or not.

[Shu Chen, 2015-08-19 00:50:28]

On 2015/08/18 18:35:53, afakhry wrote:
> On 2015/08/17 03:27:40, Shu Chen wrote:
> > I think this cl is going to the wrong way.
> > 
> > By reading crbug.com/471488, I think the root cause is that "Search+M" is
> > interpret as a command but the key event isn't marked as
stopped_propagation()
> > after processing.
> > Please refer to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_....
> > 
> > And please also refer to the bug thread: crbug.com/508668.
> 
> This CL is trying to enforce treating the Search key as a modifier like Alt
and
> Ctrl regardless of whether Search+Key is tied to a command or not.

I think it maybe better to make event.GetCharacter() return 0, so that
InertChar() will not be called at all.
But that would be more risky than this cl.

I am removing myself from the reviewers and let other reviewers decide.

[Shu Chen, 2015-08-19 00:50:37]

shuchen@chromium.org changed reviewers:
- shuchen@chromium.org

[oshima, 2015-09-02 18:06:56]

On 2015/08/19 00:50:28, Shu Chen wrote:
> On 2015/08/18 18:35:53, afakhry wrote:
> > On 2015/08/17 03:27:40, Shu Chen wrote:
> > > I think this cl is going to the wrong way.
> > > 
> > > By reading crbug.com/471488, I think the root cause is that "Search+M" is
> > > interpret as a command but the key event isn't marked as
> stopped_propagation()
> > > after processing.
> > > Please refer to
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_....
> > > 
> > > And please also refer to the bug thread: crbug.com/508668.
> > 
> > This CL is trying to enforce treating the Search key as a modifier like Alt
> and
> > Ctrl regardless of whether Search+Key is tied to a command or not.
> 
> I think it maybe better to make event.GetCharacter() return 0, so that
> InertChar() will not be called at all.
> But that would be more risky than this cl.

I believe we discussed this a bit in the past. How risky is this? Is it worth
doing this now to see if this causes any issue on 47?

> 
> I am removing myself from the reviewers and let other reviewers decide.

[afakhry, 2015-09-02 18:21:20]

afakhry@chromium.org changed reviewers:
+ shuchen@chromium.org

[afakhry, 2015-09-02 18:22:26]

On 2015/09/02 18:06:56, oshima wrote:
> On 2015/08/19 00:50:28, Shu Chen wrote:
> > On 2015/08/18 18:35:53, afakhry wrote:
> > > On 2015/08/17 03:27:40, Shu Chen wrote:
> > > > I think this cl is going to the wrong way.
> > > > 
> > > > By reading crbug.com/471488, I think the root cause is that "Search+M"
is
> > > > interpret as a command but the key event isn't marked as
> > stopped_propagation()
> > > > after processing.
> > > > Please refer to
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_....
> > > > 
> > > > And please also refer to the bug thread: crbug.com/508668.
> > > 
> > > This CL is trying to enforce treating the Search key as a modifier like
Alt
> > and
> > > Ctrl regardless of whether Search+Key is tied to a command or not.
> > 
> > I think it maybe better to make event.GetCharacter() return 0, so that
> > InertChar() will not be called at all.
> > But that would be more risky than this cl.
> 
> I believe we discussed this a bit in the past. How risky is this? Is it worth
> doing this now to see if this causes any issue on 47?

I think Shu Chen had already removed himself from this CL, so maybe he can't see
your
reply. I added him back.

> > 
> > I am removing myself from the reviewers and let other reviewers decide.

[chromium-reviews, 2015-09-03 14:35:12]

On Thu, Sep 3, 2015 at 2:06 AM, <oshima@chromium.org> wrote:

> On 2015/08/19 00:50:28, Shu Chen wrote:
>
>> On 2015/08/18 18:35:53, afakhry wrote:
>> > On 2015/08/17 03:27:40, Shu Chen wrote:
>> > > I think this cl is going to the wrong way.
>> > >
>> > > By reading crbug.com/471488, I think the root cause is that
>> "Search+M" is
>> > > interpret as a command but the key event isn't marked as
>> stopped_propagation()
>> > > after processing.
>> > > Please refer to
>> > >
>> >
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_...
> .
>
>> > >
>> > > And please also refer to the bug thread: crbug.com/508668.
>> >
>> > This CL is trying to enforce treating the Search key as a modifier like
>> Alt
>> and
>> > Ctrl regardless of whether Search+Key is tied to a command or not.
>>
>
> I think it maybe better to make event.GetCharacter() return 0, so that
>> InertChar() will not be called at all.
>> But that would be more risky than this cl.
>>
>
> I believe we discussed this a bit in the past. How risky is this? Is it
> worth
> doing this now to see if this causes any issue on 47?
>
 +kpschoedel@ who may be the best guy to evaluate the risk of modifying
event.cc.

>
>
> I am removing myself from the reviewers and let other reviewers decide.
>>
>
>
>
> https://codereview.chromium.org/1221083008/
>



-- 

Shu Chen | Software Engineer | shuchen@google.com | +86 (10) 6250-3615

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kpschoedel, 2015-09-03 15:18:09]

On 2015/09/03 14:35:12, chromium-reviews wrote:
> > I think it maybe better to make event.GetCharacter() return 0, so that
> >> InertChar() will not be called at all.
> >> But that would be more risky than this cl.

Does the character actually come from GetCharacter() or from one of GetText() /
GetUnmodifiedText() which already have specific hacks? (Though GetText() looks
suspicious to me, as it ignores the keyboard layout if Control is set.)

I'm actually right now trying to figure out what KeyEvent::GetCharacter() should
do, along with what the canonical KeyEvent::key_ should be - see issue 527111.
When |key_| gets fixed to match the spec, GetCharacter() will certainly need to
change in some way. I don't see any immediate problem with having it return 0 if
one of the ‘action’ flags (Control/Alt/Search) is down.

CC +dtapuska@ +garykac@ for web event knowledge.

[dtapuska, 2015-09-03 15:27:11]

dtapuska@chromium.org changed reviewers:
+ dtapuska@chromium.org

[dtapuska, 2015-09-03 15:27:12]

https://codereview.chromium.org/1221083008/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1221083008/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1600: is_char,
This doesn't seem correct to me.

This is going to generate two keydown events in javascript and not a keydown,
keypress, keyup sequence as intended.

You should try this page http://nonan.jp/keyevent/input.html

and see the sequence of events for each. I suspect you'll start seeing a
duplicated keydown event which is incorrect.

[afakhry, 2015-09-03 17:32:47]

On 2015/09/03 15:18:09, kpschoedel wrote:
> On 2015/09/03 14:35:12, chromium-reviews wrote:
> > > I think it maybe better to make event.GetCharacter() return 0, so that
> > >> InertChar() will not be called at all.
> > >> But that would be more risky than this cl.
> 
> Does the character actually come from GetCharacter() or from one of GetText()
/
> GetUnmodifiedText() which already have specific hacks? (Though GetText() looks
> suspicious to me, as it ignores the keyboard layout if Control is set.)

The character comes from KeyEvent::GetCharacter(). See 
InputMethodChromeOS::ProcessUnfilteredKeyPressEvent() here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/input_...

I think KeyEvent::GetCharacter() should return 0 if DomKey::IsCharacter()
returns false here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc...

> I'm actually right now trying to figure out what KeyEvent::GetCharacter()
should
> do, along with what the canonical KeyEvent::key_ should be - see issue 527111.
> When |key_| gets fixed to match the spec, GetCharacter() will certainly need
to
> change in some way. I don't see any immediate problem with having it return 0
if
> one of the ‘action’ flags (Control/Alt/Search) is down.
> 
> CC +dtapuska@ +garykac@ for web event knowledge.

[afakhry, 2015-09-03 17:34:03]

https://codereview.chromium.org/1221083008/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1221083008/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1600: is_char,
On 2015/09/03 15:27:12, dtapuska wrote:
> This doesn't seem correct to me.
> 
> This is going to generate two keydown events in javascript and not a keydown,
> keypress, keyup sequence as intended.
> 
> You should try this page http://nonan.jp/keyevent/input.html
> 
> and see the sequence of events for each. I suspect you'll start seeing a
> duplicated keydown event which is incorrect.

You are right, I used to get two keydowns. I changed it to avoid sending the
event altogether if |is_char| is false. I validated that behavior of pressing
Ctrl+Q matches that of Search+Q using the test page you provided.