Pin keyboard use enter to submit when focus is not on a button.

Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9c8327e6e81fc97b5b2c3480c2671b6225e85d6c
Cr-Commit-Position: refs/heads/master@{#404904}

[sammiequon, 2016-06-30 21:58:27]

Description was changed from

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
==========

to

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sammiequon, 2016-06-30 21:59:03]

Description was changed from

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[sammiequon, 2016-06-30 21:59:04]

sammiequon@chromium.org changed reviewers:
+ jdufault@chromium.org

[sammiequon, 2016-06-30 21:59:29]

jdufault@ - Please take a look.

[jdufault, 2016-06-30 22:15:10]

On 2016/06/30 21:59:29, sammiequon wrote:
> jdufault@ - Please take a look.

If you revert this change are you still able to reproduce the paper-button
animation glitches when you tab/spacebar really fast?

[jdufault, 2016-06-30 22:18:44]

https://codereview.chromium.org/2119713003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right):

https://codereview.chromium.org/2119713003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:111:
on-focus="onButtonFocus_"
Can we use one of the paper-button properties instead of intercepting focus
events?

active[1], focused, and receivedFocusFromKeyboard all look very interesting.

1: https://elements.polymer-project.org/elements/paper-button#property-active

[sammiequon, 2016-07-01 21:13:58]

https://codereview.chromium.org/2119713003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right):

https://codereview.chromium.org/2119713003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:111:
on-focus="onButtonFocus_"
On 2016/06/30 22:18:44, jdufault wrote:
> Can we use one of the paper-button properties instead of intercepting focus
> events?
> 
> active[1], focused, and receivedFocusFromKeyboard all look very interesting.
> 
> 1: https://elements.polymer-project.org/elements/paper-button#property-active

Done.

[jdufault, 2016-07-01 21:20:52]

https://codereview.chromium.org/2119713003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):

https://codereview.chromium.org/2119713003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:62: if
(this.focusedElement_ != event.target)
Can we traverse the DOM to find the focused element instead of caching it? Does
document.activeElement work?

[sammiequon, 2016-07-01 22:19:14]

https://codereview.chromium.org/2119713003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):

https://codereview.chromium.org/2119713003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:62: if
(this.focusedElement_ != event.target)
On 2016/07/01 21:20:52, jdufault wrote:
> Can we traverse the DOM to find the focused element instead of caching it?
Does
> document.activeElement work?

document.activeElement always returns the button if you tap it. But I want to
send focus back to the input if the previous focused element, so I cached it. I
think we can replace event.target with document.activeElement thought.

[jdufault, 2016-07-06 19:07:05]

https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):

https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:63: this.focus();
Can this just be

  // Only focus buttons if the user tabbed to them.
  if (!event.target.receivedFocusFromKeyboard)
    this.focus()

If that works, then all the other changes can be reverted.

[sammiequon, 2016-07-06 19:23:05]

On 2016/07/06 19:07:05, jdufault wrote:
>
https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):
> 
>
https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:63:
this.focus();
> Can this just be
> 
>   // Only focus buttons if the user tabbed to them.
>   if (!event.target.receivedFocusFromKeyboard)
>     this.focus()
> 
> If that works, then all the other changes can be reverted.

That works, except if we tab to a number, then click it with mouse, the focus
goes to the input. Is that what we want?

[jdufault, 2016-07-06 19:48:35]

On 2016/07/06 19:23:05, sammiequon wrote:
> On 2016/07/06 19:07:05, jdufault wrote:
> >
>
https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
> > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):
> > 
> >
>
https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:63:
> this.focus();
> > Can this just be
> > 
> >   // Only focus buttons if the user tabbed to them.
> >   if (!event.target.receivedFocusFromKeyboard)
> >     this.focus()
> > 
> > If that works, then all the other changes can be reverted.
> 
> That works, except if we tab to a number, then click it with mouse, the focus
> goes to the input. Is that what we want?

If we want to maintain that behavior, then I think instead of tracking the
focused element we should just track if the user used tabbing (and make sure to
reset the tab behavior if the user selected the input element or the element was
focused), so something along the lines of:

  onNumberTap_: function() {
    // ...
    if (event.target == this.$$('#pin-input'))
      this.didUseTabNavigation_ = false;
    else if (event.target.receivedFocusFromKeyboard)
      this.didUseTabNavigation_ = true;

    if (!this.didUseTabNavigation_)
      this.focus();
    // ...
  },

  focus: function() {
    // ...
    this.didUseTabNavigation_ = false;
    // ...
  }

Doing something like this removes the second event handler, which is nice
because it keeps all of the logic in one place.

But I think having click reset focus is fine, so the approach discussed in the
original comment is good enough (so no need to keep the didUseTabNavigation_
variable).

[sammiequon, 2016-07-06 20:04:56]

Patchset #4 (id:50001) has been deleted

[sammiequon, 2016-07-06 20:05:40]

On 2016/07/06 19:48:35, jdufault wrote:
> On 2016/07/06 19:23:05, sammiequon wrote:
> > On 2016/07/06 19:07:05, jdufault wrote:
> > >
> >
>
https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
> > > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2119713003/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:63:
> > this.focus();
> > > Can this just be
> > > 
> > >   // Only focus buttons if the user tabbed to them.
> > >   if (!event.target.receivedFocusFromKeyboard)
> > >     this.focus()
> > > 
> > > If that works, then all the other changes can be reverted.
> > 
> > That works, except if we tab to a number, then click it with mouse, the
focus
> > goes to the input. Is that what we want?
> 
> If we want to maintain that behavior, then I think instead of tracking the
> focused element we should just track if the user used tabbing (and make sure
to
> reset the tab behavior if the user selected the input element or the element
was
> focused), so something along the lines of:
> 
>   onNumberTap_: function() {
>     // ...
>     if (event.target == this.$$('#pin-input'))
>       this.didUseTabNavigation_ = false;
>     else if (event.target.receivedFocusFromKeyboard)
>       this.didUseTabNavigation_ = true;
> 
>     if (!this.didUseTabNavigation_)
>       this.focus();
>     // ...
>   },
> 
>   focus: function() {
>     // ...
>     this.didUseTabNavigation_ = false;
>     // ...
>   }
> 
> Doing something like this removes the second event handler, which is nice
> because it keeps all of the logic in one place.
> 
> But I think having click reset focus is fine, so the approach discussed in the
> original comment is good enough (so no need to keep the didUseTabNavigation_
> variable).

Done.

[jdufault, 2016-07-06 21:08:51]

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right):

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:143:
<inner-text>2</inner-text>
Revert all changes in this file.

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:41:
pinInput.focus();
Revert these changes.

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:49: // If the
number received focus from the mouse click, we trasnfer the focus
nit: transfer

Can you update the comment to explain why? Right now it just describes the code.

[sammiequon, 2016-07-06 23:36:37]

Patchset #5 (id:90001) has been deleted

[sammiequon, 2016-07-06 23:38:02]

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right):

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:143:
<inner-text>2</inner-text>
On 2016/07/06 21:08:51, jdufault wrote:
> Revert all changes in this file.

Done.

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:41:
pinInput.focus();
On 2016/07/06 21:08:51, jdufault wrote:
> Revert these changes.

Done.

https://codereview.chromium.org/2119713003/diff/70001/chrome/browser/resource...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:49: // If the
number received focus from the mouse click, we trasnfer the focus
On 2016/07/06 21:08:51, jdufault wrote:
> nit: transfer
> 
> Can you update the comment to explain why? Right now it just describes the
code.

Done.

[jdufault, 2016-07-07 00:07:27]

lgtm

[sammiequon, 2016-07-07 19:54:27]

sammiequon@chromium.org changed reviewers:
+ xiyuan@chromium.org

[sammiequon, 2016-07-07 19:54:42]

xiyuan@ - Please take a look.

[xiyuan, 2016-07-07 19:56:40]

lgtm

[sammiequon, 2016-07-07 20:47:12]

The CQ bit was checked by sammiequon@chromium.org

[commit-bot: I haz the power, 2016-07-07 20:48:04]

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-07-07 21:53:46]

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

[commit-bot: I haz the power, 2016-07-07 21:53:47]

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

[sammiequon, 2016-07-08 00:22:41]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2016-07-08 00:22:41]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
jdufault@chromium.org
Link to the patchset: https://codereview.chromium.org/2119713003/#ps130001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-07-08 00:23:24]

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-07-08 00:39:42]

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

[commit-bot: I haz the power, 2016-07-08 00:39:44]

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

[sammiequon, 2016-07-11 23:14:40]

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

[commit-bot: I haz the power, 2016-07-11 23:15:59]

Dry run: 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-07-11 23:30:41]

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

[commit-bot: I haz the power, 2016-07-11 23:30:42]

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

[sammiequon, 2016-07-12 19:36:36]

jdufault@ -Updated to pass closure compiliation. Please take a look.

[jdufault, 2016-07-12 19:59:44]

https://codereview.chromium.org/2119713003/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right):

https://codereview.chromium.org/2119713003/diff/170001/chrome/browser/resourc...
chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:65: if
(!event.target.classList.contains('keyboard-focus'))
What about just casting event.target to the right type?

Checking the active class state seems like a pretty round-about way to do this.

[jdufault, 2016-07-12 23:39:26]

lgtm

[sammiequon, 2016-07-12 23:47:29]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2016-07-12 23:47:30]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2119713003/#ps190001
(title: "Compatiable with closure compiler.")

[commit-bot: I haz the power, 2016-07-12 23:49:09]

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-07-13 02:20:59]

Description was changed from

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-13 02:21:00]

Committed patchset #9 (id:190001)

[commit-bot: I haz the power, 2016-07-13 02:23:30]

Description was changed from

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Pin keyboard use enter to submit when focus is not on a button.

BUG=624191
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9c8327e6e81fc97b5b2c3480c2671b6225e85d6c
Cr-Commit-Position: refs/heads/master@{#404904}
==========

[commit-bot: I haz the power, 2016-07-13 02:23:32]

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