Fix VK flashing after clicking action area on user pod

Fix VK flashing after clicking action area on user pod

BUG=396016
Committed: https://crrev.com/673add86a33e1cd0c753030f9ff5faa5b11a39bc
Cr-Commit-Position: refs/heads/master@{#339346}

[bshe, 2015-06-29 20:47:16]

bshe@chromium.org changed reviewers:
+ dzhioev@chromium.org

[bshe, 2015-06-29 20:47:17]

Hi Pavel. Do you mind to take a look at this CL?

The problem was when user click the action area button on an inactive user pod,
VK flashed before the action box menu shows up. It looks like during the process
of activating the action box menu, a focus event is dispatched on
actionBoxAreaElement. And when handling the focus event, we try to focus on the
inactive user
pod(https://code.google.com/p/chromium/codesearch/#chromium/src/ui/login/account_picker/user_pod_row.js&l=3002),
which results VK to show up briefly before the text box in user pod lost focus
again.

It looks like we shouldn't use "action-box-button" but should use
"action-box-area". This way we could prevent focus on an inactive user pod when
just active the menu. The code in question was introduced here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
It replaced "remove-user-button". But the counterpart for it should be
"action-box-area" which is the parent of "action-box-button" (see
user_pod_template.html in the same CL).

Let me know what do you think. Thanks!

[dzhioev (left Google), 2015-06-30 03:54:27]

On 2015/06/29 20:47:17, bshe wrote:
> Hi Pavel. Do you mind to take a look at this CL?
> 
> The problem was when user click the action area button on an inactive user
pod,
> VK flashed before the action box menu shows up. It looks like during the
process
> of activating the action box menu, a focus event is dispatched on
> actionBoxAreaElement. And when handling the focus event, we try to focus on
the
> inactive user
>
pod(https://code.google.com/p/chromium/codesearch/#chromium/src/ui/login/account_picker/user_pod_row.js&l=3002),
> which results VK to show up briefly before the text box in user pod lost focus
> again.
> 
> It looks like we shouldn't use "action-box-button" but should use
> "action-box-area". This way we could prevent focus on an inactive user pod
when
> just active the menu. The code in question was introduced here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> It replaced "remove-user-button". But the counterpart for it should be
> "action-box-area" which is the parent of "action-box-button" (see
> user_pod_template.html in the same CL).
> 
> Let me know what do you think. Thanks!

Wooh, I just spend an hour to understand how all this focusing code works, but I
still don't understand it completely.

But I agree with you, 'action-box-button' most likely shold be replaced with
'action-box-area' here.
At least because 'action-box-button' can not be a target of focus event, as it
is a <div> and it never
has "taborder" attribute set. So
|e.target.classList.contains('action-box-button')| is always false.

LGTM

[bshe, 2015-06-30 13:55:38]

On 2015/06/30 03:54:27, dzhioev wrote:
> On 2015/06/29 20:47:17, bshe wrote:
> > Hi Pavel. Do you mind to take a look at this CL?
> > 
> > The problem was when user click the action area button on an inactive user
> pod,
> > VK flashed before the action box menu shows up. It looks like during the
> process
> > of activating the action box menu, a focus event is dispatched on
> > actionBoxAreaElement. And when handling the focus event, we try to focus on
> the
> > inactive user
> >
>
pod(https://code.google.com/p/chromium/codesearch/#chromium/src/ui/login/account_picker/user_pod_row.js&l=3002),
> > which results VK to show up briefly before the text box in user pod lost
focus
> > again.
> > 
> > It looks like we shouldn't use "action-box-button" but should use
> > "action-box-area". This way we could prevent focus on an inactive user pod
> when
> > just active the menu. The code in question was introduced here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > It replaced "remove-user-button". But the counterpart for it should be
> > "action-box-area" which is the parent of "action-box-button" (see
> > user_pod_template.html in the same CL).
> > 
> > Let me know what do you think. Thanks!
> 
> Wooh, I just spend an hour to understand how all this focusing code works, but
I
> still don't understand it completely.
> 
> But I agree with you, 'action-box-button' most likely shold be replaced with
> 'action-box-area' here.
> At least because 'action-box-button' can not be a target of focus event, as it
> is a <div> and it never
> has "taborder" attribute set. So
> |e.target.classList.contains('action-box-button')| is always false.
> 
> LGTM

Cool. Thanks! The code is indeed complex...

[bshe, 2015-06-30 13:55:44]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2015-06-30 13:55:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219763002/1

[bshe, 2015-06-30 14:07:02]

The CQ bit was unchecked by bshe@chromium.org

[bshe, 2015-06-30 21:51:39]

On 2015/06/30 13:55:58, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1219763002/1

Sorry. I found a few issues with this patch. Will hold it until I figure out how
to solve the problem.
The focus code is indeed tricky...

[dzhioev (left Google), 2015-06-30 23:36:28]

BTW, you can remove all references to this.keyboardActivated_, as it is
unused.

On Tue, Jun 30, 2015 at 2:51 PM <bshe@chromium.org> wrote:

> On 2015/06/30 13:55:58, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >   https://chromium-cq-status.appspot.com/patch-status/1219763002/1
>
> Sorry. I found a few issues with this patch. Will hold it until I figure
> out how
> to solve the problem.
> The focus code is indeed tricky...
>
> https://codereview.chromium.org/1219763002/
>

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

[bshe, 2015-07-02 15:35:01]

Patchset #2 (id:20001) has been deleted

[bshe, 2015-07-02 15:39:43]

On 2015/06/30 23:36:28, dzhioev wrote:
> BTW, you can remove all references to this.keyboardActivated_, as it is
> unused.
> 
> On Tue, Jun 30, 2015 at 2:51 PM <mailto:bshe@chromium.org> wrote:
> 
> > On 2015/06/30 13:55:58, commit-bot: I haz the power wrote:
> > > CQ is trying da patch. Follow status at
> > >   https://chromium-cq-status.appspot.com/patch-status/1219763002/1
> >
> > Sorry. I found a few issues with this patch. Will hold it until I figure
> > out how
> > to solve the problem.
> > The focus code is indeed tricky...
> >
> > https://codereview.chromium.org/1219763002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hi Pavel. This is ready for another round of review. I added an option parameter
to suppress the temporary focus on password input box. This is a kind of hacky
but
safer approach. It looks like a few other things depends on the user pod has
focus
state to work appropriately. My previous patch doesn't change user pod to focus
state
which results problems like failed to remove user pod once click the remove user
menu button.

[bshe, 2015-07-08 16:55:33]

On 2015/07/02 15:39:43, bshe wrote:
> On 2015/06/30 23:36:28, dzhioev wrote:
> > BTW, you can remove all references to this.keyboardActivated_, as it is
> > unused.
> > 
> > On Tue, Jun 30, 2015 at 2:51 PM <mailto:bshe@chromium.org> wrote:
> > 
> > > On 2015/06/30 13:55:58, commit-bot: I haz the power wrote:
> > > > CQ is trying da patch. Follow status at
> > > >   https://chromium-cq-status.appspot.com/patch-status/1219763002/1
> > >
> > > Sorry. I found a few issues with this patch. Will hold it until I figure
> > > out how
> > > to solve the problem.
> > > The focus code is indeed tricky...
> > >
> > > https://codereview.chromium.org/1219763002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Hi Pavel. This is ready for another round of review. I added an option
parameter
> to suppress the temporary focus on password input box. This is a kind of hacky
> but
> safer approach. It looks like a few other things depends on the user pod has
> focus
> state to work appropriately. My previous patch doesn't change user pod to
focus
> state
> which results problems like failed to remove user pod once click the remove
user
> menu button.

friendly ping? I used a very different approach in the new patch, would you mind
to take another look at this CL?

[dzhioev (left Google), 2015-07-17 18:48:50]

LGTM with nits

https://codereview.chromium.org/1219763002/diff/40001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1219763002/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:2730: * @param {boolean=}
opt_skip_input_focus If true, dont focus on the input
nit: s/dont/don't/

https://codereview.chromium.org/1219763002/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:2733: focusPod: function(podToFocus,
opt_force, opt_skip_input_focus) {
nit: I'm not sure, but I think it should be called opt_skipInputFocus

[bshe, 2015-07-17 21:34:58]

thanks for review!

https://codereview.chromium.org/1219763002/diff/40001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1219763002/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:2730: * @param {boolean=}
opt_skip_input_focus If true, dont focus on the input
On 2015/07/17 18:48:50, dzhioev wrote:
> nit: s/dont/don't/

Done.

https://codereview.chromium.org/1219763002/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:2733: focusPod: function(podToFocus,
opt_force, opt_skip_input_focus) {
On 2015/07/17 18:48:50, dzhioev wrote:
> nit: I'm not sure, but I think it should be called opt_skipInputFocus

Done.

[bshe, 2015-07-17 21:35:02]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-07-17 21:35:03]

The patchset sent to the CQ was uploaded after l-g-t-m from dzhioev@chromium.org
Link to the patchset: https://codereview.chromium.org/1219763002/#ps60001
(title: "review")

[commit-bot: I haz the power, 2015-07-17 21:37:34]

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

[commit-bot: I haz the power, 2015-07-17 22:39:04]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-07-17 22:40:01]

Patchset 3 (id:??) landed as
https://crrev.com/673add86a33e1cd0c753030f9ff5faa5b11a39bc
Cr-Commit-Position: refs/heads/master@{#339346}