views: don't send OnKeypressUnhandled for all keys

views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007
Committed: https://crrev.com/48a12e434146dc568dabb364d84be911b6d369e4
Cr-Commit-Position: refs/heads/master@{#423908}

[Elly Fong-Jones, 2016-10-05 18:22:14]

Description was changed from

==========
views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007
==========

to

==========
views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007
==========

[Elly Fong-Jones, 2016-10-05 18:22:14]

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

[Elly Fong-Jones, 2016-10-05 18:22:23]

sky: ptal? :)

[sky, 2016-10-06 15:58:12]

https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (left):

https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:688: if (!handled)
Should this instead check handled || event.handled() ||
event.stopped_propagation()?

[Elly Fong-Jones, 2016-10-06 16:20:05]

https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (left):

https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:688: if (!handled)
On 2016/10/06 15:58:12, sky wrote:
> Should this instead check handled || event.handled() ||
> event.stopped_propagation()?

I don't think so - for example, when you tab away from the textfield, the event
is handled further up the view hierarchy (as I understand it), so by the time
OnKeyPressed runs, the event hasn't been handled yet, but will be in future.

[sky, 2016-10-06 21:17:11]

Did you verify that? Tab should be handled by the FocusManager and
before the View.

  -Scott

On Thu, Oct 6, 2016 at 9:20 AM,  <ellyjones@chromium.org> wrote:
>
>
https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
> File ui/views/controls/textfield/textfield.cc (left):
>
>
https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
> ui/views/controls/textfield/textfield.cc:688: if (!handled)
> On 2016/10/06 15:58:12, sky wrote:
>> Should this instead check handled || event.handled() ||
>> event.stopped_propagation()?
>
> I don't think so - for example, when you tab away from the textfield,
> the event is handled further up the view hierarchy (as I understand it),
> so by the time OnKeyPressed runs, the event hasn't been handled yet, but
> will be in future.
>
> https://codereview.chromium.org/2399603002/

-- 
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.

[Elly Fong-Jones, 2016-10-07 15:42:19]

It does work with Tab, but for example, if I open the bookmark bubble, focus the
text field, then hit Esc, inside Textfield::OnKeyPressed() handled == false,
event.handled() == false, and event.stopped_propagation() == false, but the Esc
key is still eventually handled by the bubble to close it. I don't want the
textfield to beep at this keypress, but I'm not sure how to detect that it will
be handled by the bubble.

On 2016/10/06 21:17:11, sky wrote:
> Did you verify that? Tab should be handled by the FocusManager and
> before the View.
> 
>   -Scott
> 
> On Thu, Oct 6, 2016 at 9:20 AM,  <mailto:ellyjones@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
> > File ui/views/controls/textfield/textfield.cc (left):
> >
> >
>
https://codereview.chromium.org/2399603002/diff/1/ui/views/controls/textfield...
> > ui/views/controls/textfield/textfield.cc:688: if (!handled)
> > On 2016/10/06 15:58:12, sky wrote:
> >> Should this instead check handled || event.handled() ||
> >> event.stopped_propagation()?
> >
> > I don't think so - for example, when you tab away from the textfield,
> > the event is handled further up the view hierarchy (as I understand it),
> > so by the time OnKeyPressed runs, the event hasn't been handled yet, but
> > will be in future.
> >
> > https://codereview.chromium.org/2399603002/
> 
> -- 
> 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-10-07 16:41:30]

I get it. LGTM I think the confusion is OnKeypressUnhandled is now a poor name,
given the function really means key pressed in uneditable textfield.

[Elly Fong-Jones, 2016-10-07 17:04:05]

On 2016/10/07 16:41:30, sky wrote:
> I get it. LGTM I think the confusion is OnKeypressUnhandled is now a poor
name,
> given the function really means key pressed in uneditable textfield.

Renamed it.

[Elly Fong-Jones, 2016-10-07 17:04:15]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-10-07 17:04:16]

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/2399603002/#ps20001
(title: "OnKeypressUnhandled -> OnEditFailed")

[commit-bot: I haz the power, 2016-10-07 17:04:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 18:00:27]

Description was changed from

==========
views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007
==========

to

==========
views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007
==========

[commit-bot: I haz the power, 2016-10-07 18:00:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-07 18:04:53]

Description was changed from

==========
views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007
==========

to

==========
views: don't send OnKeypressUnhandled for all keys

Textfield::OnKeyPressed() is not responsible for all keystroke handling,
but it is called for every pressed key, which means that 'handled' in
this function is not correct if the key is handled elsewhere, like IME
or focus hotkeys. As such, don't call OnKeypressUnhandled() in OnKeyPressed()
at all; only call it in InsertChar() for readonly textfields.

BUG=649559, 649007

Committed: https://crrev.com/48a12e434146dc568dabb364d84be911b6d369e4
Cr-Commit-Position: refs/heads/master@{#423908}
==========

[commit-bot: I haz the power, 2016-10-07 18:04:54]

Patchset 2 (id:??) landed as
https://crrev.com/48a12e434146dc568dabb364d84be911b6d369e4
Cr-Commit-Position: refs/heads/master@{#423908}

[sky, 2016-10-07 19:53:40]

Nice! I like the new name. Thanks!