The key event should be stopped propagation when it's been processed as an accelerator.

The key event should be stopped propagation when it's been processed as an accelerator.

BUG=587345
Committed: https://crrev.com/f46e41598c3205fac6c4f2f994839014cefeb7ba
Cr-Commit-Position: refs/heads/master@{#376933}

[Shu Chen, 2016-02-17 09:05:47]

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

[Shu Chen, 2016-02-17 09:05:47]

Scott, can you please take a look? Thanks

[Shu Chen, 2016-02-17 09:06:35]

The CQ bit was checked by shuchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-17 09:07:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1703683003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1703683003/1

[commit-bot: I haz the power, 2016-02-17 09:22:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2016-02-17 09:23:11]

Description was changed from

==========
The key event should be stopped propagation when it's been processed as an
accelerator.

FocusManager::OnKeyEvent() should call Event::StopPropagation() as
necessary when ProcessAccelerator() returns true.

BUG=587345
==========

to

==========
The key event should be stopped propagation when it's been processed as an
accelerator.

BUG=587345
==========

[sky, 2016-02-17 18:15:10]

sky@chromium.org changed reviewers:
+ sadrul@chromium.org

[sky, 2016-02-17 18:15:11]

+sadrul
Please add test coverage.
Stopping propagation entirely may have unexpected consequences. What handler is
triggering after the event is handled that you don't want triggering?

[sadrul, 2016-02-17 21:17:55]

https://codereview.chromium.org/1703683003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1703683003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:881: }
Should this code just move into Widget::OnKeyEvent()? That way, we don't need to
duplicate this code in NWA and DNWA (and also in NativeWidgetMus)

[Shu Chen, 2016-02-18 00:19:15]

On 2016/02/17 18:15:11, sky wrote:
> +sadrul
> Please add test coverage.
> Stopping propagation entirely may have unexpected consequences. What handler
is
> triggering after the event is handled that you don't want triggering?

The InputMethod checks the stopped_propagation() state of the key event to
determine
whether it needs to insert characters into the input field.

The AcceleratorFilter is doing it correctly:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...

For the accelerator cases, I cannot think of potential side effects.

[Shu Chen, 2016-02-18 00:29:54]

https://codereview.chromium.org/1703683003/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/1703683003/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:881: }
On 2016/02/17 21:17:55, sadrul wrote:
> Should this code just move into Widget::OnKeyEvent()? That way, we don't need
to
> duplicate this code in NWA and DNWA (and also in NativeWidgetMus)

Done. Good idea.

[Shu Chen, 2016-02-18 04:44:56]

sky@, I've added a test. Can you please take another look? Thanks

[sky, 2016-02-18 17:49:54]

On Wed, Feb 17, 2016 at 4:19 PM,  <shuchen@chromium.org> wrote:
> On 2016/02/17 18:15:11, sky wrote:
>> +sadrul
>> Please add test coverage.
>> Stopping propagation entirely may have unexpected consequences. What
>> handler
> is
>> triggering after the event is handled that you don't want triggering?
>
> The InputMethod checks the stopped_propagation() state of the key event to
> determine
> whether it needs to insert characters into the input field.

Why does it check stopped_propagation rather than handled?

>
> The AcceleratorFilter is doing it correctly:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...
>
> For the accelerator cases, I cannot think of potential side effects.
>
>
> https://codereview.chromium.org/1703683003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Shu Chen, 2016-02-19 00:29:50]

On 2016/02/18 17:49:54, sky wrote:
> On Wed, Feb 17, 2016 at 4:19 PM,  <mailto:shuchen@chromium.org> wrote:
> > On 2016/02/17 18:15:11, sky wrote:
> >> +sadrul
> >> Please add test coverage.
> >> Stopping propagation entirely may have unexpected consequences. What
> >> handler
> > is
> >> triggering after the event is handled that you don't want triggering?
> >
> > The InputMethod checks the stopped_propagation() state of the key event to
> > determine
> > whether it needs to insert characters into the input field.
> 
> Why does it check stopped_propagation rather than handled?
It is because of
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....
If checking stopped_propagation, the keyboard typing would NOT work in web
pages.

> 
> >
> > The AcceleratorFilter is doing it correctly:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...
> >
> > For the accelerator cases, I cannot think of potential side effects.
> >
> >
> > https://codereview.chromium.org/1703683003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Shu Chen, 2016-02-19 00:30:24]

On 2016/02/19 00:29:50, Shu Chen wrote:
> On 2016/02/18 17:49:54, sky wrote:
> > On Wed, Feb 17, 2016 at 4:19 PM,  <mailto:shuchen@chromium.org> wrote:
> > > On 2016/02/17 18:15:11, sky wrote:
> > >> +sadrul
> > >> Please add test coverage.
> > >> Stopping propagation entirely may have unexpected consequences. What
> > >> handler
> > > is
> > >> triggering after the event is handled that you don't want triggering?
> > >
> > > The InputMethod checks the stopped_propagation() state of the key event to
> > > determine
> > > whether it needs to insert characters into the input field.
> > 
> > Why does it check stopped_propagation rather than handled?
> It is because of
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....
> If checking stopped_propagation, the keyboard typing would NOT work in web
> pages.
Sorry I meant that if checking handled, the keyboard typing would NOT work in
web pages.
> 
> > 
> > >
> > > The AcceleratorFilter is doing it correctly:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...
> > >
> > > For the accelerator cases, I cannot think of potential side effects.
> > >
> > >
> > > https://codereview.chromium.org/1703683003/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >

[sky, 2016-02-19 00:56:16]

On 2016/02/19 00:30:24, Shu Chen wrote:
> On 2016/02/19 00:29:50, Shu Chen wrote:
> > On 2016/02/18 17:49:54, sky wrote:
> > > On Wed, Feb 17, 2016 at 4:19 PM,  <mailto:shuchen@chromium.org> wrote:
> > > > On 2016/02/17 18:15:11, sky wrote:
> > > >> +sadrul
> > > >> Please add test coverage.
> > > >> Stopping propagation entirely may have unexpected consequences. What
> > > >> handler
> > > > is
> > > >> triggering after the event is handled that you don't want triggering?
> > > >
> > > > The InputMethod checks the stopped_propagation() state of the key event
to
> > > > determine
> > > > whether it needs to insert characters into the input field.
> > > 
> > > Why does it check stopped_propagation rather than handled?
> > It is because of
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....
> > If checking stopped_propagation, the keyboard typing would NOT work in web
> > pages.
> Sorry I meant that if checking handled, the keyboard typing would NOT work in
> web pages.
> > 
> > > 
> > > >
> > > > The AcceleratorFilter is doing it correctly:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...
> > > >
> > > > For the accelerator cases, I cannot think of potential side effects.
> > > >
> > > >
> > > > https://codereview.chromium.org/1703683003/
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >

Sadrul, do you know why RWHVA always calls SetHandled unconditionally? Having
widget and others know that they have to stop propagation in certain cases seems
bizarre to me. Seems like we should only call SetHandled when the event is
really handled, and IME should look at that. Maybe I'm missing something?
I have to admit I'm a bit confused by why we sometimes stop propagation and why
we call SetHandled.

[sadrul, 2016-02-19 21:30:55]

On 2016/02/19 00:56:16, sky wrote:
> On 2016/02/19 00:30:24, Shu Chen wrote:
> > On 2016/02/19 00:29:50, Shu Chen wrote:
> > > On 2016/02/18 17:49:54, sky wrote:
> > > > On Wed, Feb 17, 2016 at 4:19 PM,  <mailto:shuchen@chromium.org> wrote:
> > > > > On 2016/02/17 18:15:11, sky wrote:
> > > > >> +sadrul
> > > > >> Please add test coverage.
> > > > >> Stopping propagation entirely may have unexpected consequences. What
> > > > >> handler
> > > > > is
> > > > >> triggering after the event is handled that you don't want triggering?
> > > > >
> > > > > The InputMethod checks the stopped_propagation() state of the key
event
> to
> > > > > determine
> > > > > whether it needs to insert characters into the input field.
> > > > 
> > > > Why does it check stopped_propagation rather than handled?
> > > It is because of
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....
> > > If checking stopped_propagation, the keyboard typing would NOT work in web
> > > pages.
> > Sorry I meant that if checking handled, the keyboard typing would NOT work
in
> > web pages.
> > > 
> > > > 
> > > > >
> > > > > The AcceleratorFilter is doing it correctly:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...
> > > > >
> > > > > For the accelerator cases, I cannot think of potential side effects.
> > > > >
> > > > >
> > > > > https://codereview.chromium.org/1703683003/
> > > > 
> > > > -- 
> > > > You received this message because you are subscribed to the Google
Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > >
> 
> Sadrul, do you know why RWHVA always calls SetHandled unconditionally?

I thought you meant NativeWidgetAura, but looks like both NativeWidgetAura and
RenderWidgetHostViewAura always mark the events as handled.
 . In case of RWHVA, it makes sense, since it has sent the event across to the
renderer, and that essentially means as far as aura is concerned, the event has
been handled.
 . In case of NativeWidgetAura, I am not really sure. I guess we expect the
Widget to always handle the event? I would be curious to know if something
breaks if we stop doing that.

> Having
> widget and others know that they have to stop propagation in certain cases
seems
> bizarre to me. Seems like we should only call SetHandled when the event is
> really handled, and IME should look at that. Maybe I'm missing something?
> I have to admit I'm a bit confused by why we sometimes stop propagation and
why
> we call SetHandled.

Unfortunately, a lot of event-handlers do not look at whether an event has
already been handled before handling it themselves. Maybe IME is doing the same
(i.e. not ignoring already-handled events)?

[sky, 2016-02-19 23:19:27]

I'm going to LGTM this. I find the current state of affairs rather confusing,
but I'm not sure what the right answer is to make things better.

[Shu Chen, 2016-02-20 06:39:17]

On 2016/02/19 21:30:55, sadrul wrote:
> On 2016/02/19 00:56:16, sky wrote:
> > On 2016/02/19 00:30:24, Shu Chen wrote:
> > > On 2016/02/19 00:29:50, Shu Chen wrote:
> > > > On 2016/02/18 17:49:54, sky wrote:
> > > > > On Wed, Feb 17, 2016 at 4:19 PM,  <mailto:shuchen@chromium.org> wrote:
> > > > > > On 2016/02/17 18:15:11, sky wrote:
> > > > > >> +sadrul
> > > > > >> Please add test coverage.
> > > > > >> Stopping propagation entirely may have unexpected consequences.
What
> > > > > >> handler
> > > > > > is
> > > > > >> triggering after the event is handled that you don't want
triggering?
> > > > > >
> > > > > > The InputMethod checks the stopped_propagation() state of the key
> event
> > to
> > > > > > determine
> > > > > > whether it needs to insert characters into the input field.
> > > > > 
> > > > > Why does it check stopped_propagation rather than handled?
> > > > It is because of
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....
> > > > If checking stopped_propagation, the keyboard typing would NOT work in
web
> > > > pages.
> > > Sorry I meant that if checking handled, the keyboard typing would NOT work
> in
> > > web pages.
> > > > 
> > > > > 
> > > > > >
> > > > > > The AcceleratorFilter is doing it correctly:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/acceler...
> > > > > >
> > > > > > For the accelerator cases, I cannot think of potential side effects.
> > > > > >
> > > > > >
> > > > > > https://codereview.chromium.org/1703683003/
> > > > > 
> > > > > -- 
> > > > > You received this message because you are subscribed to the Google
> Groups
> > > > > "Chromium-reviews" group.
> > > > > To unsubscribe from this group and stop receiving emails from it, send
> an
> > > > email
> > > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > >
> > 
> > Sadrul, do you know why RWHVA always calls SetHandled unconditionally?
> 
> I thought you meant NativeWidgetAura, but looks like both NativeWidgetAura and
> RenderWidgetHostViewAura always mark the events as handled.
>  . In case of RWHVA, it makes sense, since it has sent the event across to the
> renderer, and that essentially means as far as aura is concerned, the event
has
> been handled.
>  . In case of NativeWidgetAura, I am not really sure. I guess we expect the
> Widget to always handle the event? I would be curious to know if something
> breaks if we stop doing that.
> 
> > Having
> > widget and others know that they have to stop propagation in certain cases
> seems
> > bizarre to me. Seems like we should only call SetHandled when the event is
> > really handled, and IME should look at that. Maybe I'm missing something?
> > I have to admit I'm a bit confused by why we sometimes stop propagation and
> why
> > we call SetHandled.
> 
> Unfortunately, a lot of event-handlers do not look at whether an event has
> already been handled before handling it themselves. Maybe IME is doing the
same
> (i.e. not ignoring already-handled events)?

Currently IME does ignore already-handled events, but it checks
stopped_propagation() instead of handled().
Because if it checks handled(), the text insertion would not work on web pages
due to the "always SetHandled()" issue in RWHVA.

Sadrul, do you have more comments on this cl? Thanks.

[Shu Chen, 2016-02-23 03:00:23]

landing this to catch the M49 stable cut on 2/25.

[Shu Chen, 2016-02-23 03:00:27]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2016-02-23 03:03:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1703683003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1703683003/60001

[commit-bot: I haz the power, 2016-02-23 03:53:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 03:53:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shu Chen, 2016-02-23 05:32:27]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2016-02-23 05:32:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1703683003/#ps80001
(title: "CQ green.")

[commit-bot: I haz the power, 2016-02-23 05:33:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1703683003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1703683003/80001

[commit-bot: I haz the power, 2016-02-23 06:18:39]

Description was changed from

==========
The key event should be stopped propagation when it's been processed as an
accelerator.

BUG=587345
==========

to

==========
The key event should be stopped propagation when it's been processed as an
accelerator.

BUG=587345
==========

[commit-bot: I haz the power, 2016-02-23 06:18:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-23 06:19:56]

Description was changed from

==========
The key event should be stopped propagation when it's been processed as an
accelerator.

BUG=587345
==========

to

==========
The key event should be stopped propagation when it's been processed as an
accelerator.

BUG=587345

Committed: https://crrev.com/f46e41598c3205fac6c4f2f994839014cefeb7ba
Cr-Commit-Position: refs/heads/master@{#376933}
==========

[commit-bot: I haz the power, 2016-02-23 06:19:57]

Patchset 5 (id:??) landed as
https://crrev.com/f46e41598c3205fac6c4f2f994839014cefeb7ba
Cr-Commit-Position: refs/heads/master@{#376933}