End to end test: async focus shouldn't trigger virtual keyboard.

End to end test: async focus shouldn't trigger virtual keyboard.

BUG=703488
TESt=try

[oka, 2017-03-23 06:17:11]

.

[oka, 2017-03-23 06:17:36]

.

[oka, 2017-03-23 06:19:54]

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

[commit-bot: I haz the power, 2017-03-23 06:20:06]

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

[oka, 2017-03-23 06:20:41]

Description was changed from

==========
End to end test: async focus shouldn't trigger virtual keyboard.
==========

to

==========
End to end test: async focus shouldn't trigger virtual keyboard.

BUG=703488
TESt=try
==========

[oka, 2017-03-23 06:21:31]

.

[oka, 2017-03-23 06:31:02]

oka@chromium.org changed reviewers:
+ oshima@chromium.org

[oka, 2017-03-23 06:31:03]

PTAL.

[oka, 2017-03-23 06:31:18]

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

[commit-bot: I haz the power, 2017-03-23 06:31:47]

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, 2017-03-23 06:45:42]

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

[commit-bot: I haz the power, 2017-03-23 06:45:43]

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

[oka, 2017-03-23 23:03:11]

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

[commit-bot: I haz the power, 2017-03-23 23:03:47]

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, 2017-03-23 23:36:06]

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

[commit-bot: I haz the power, 2017-03-23 23:36:07]

Dry run: This issue passed the CQ dry run.

[oka, 2017-03-27 07:13:19]

Oshima-san friendly ping?

[oshima, 2017-03-27 16:57:54]

https://codereview.chromium.org/2771793002/diff/60001/chrome/browser/ui/ash/k...
File chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc (right):

https://codereview.chromium.org/2771793002/diff/60001/chrome/browser/ui/ash/k...
chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc:71: // Initialized in
|SetUpOnMainThread|.
nit:

private:

and DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200 ms to
make sure virtual keyboard is hidden.
it's not clear why we need this. Shouldn't keyboard already be hidden?

[oka, 2017-03-28 09:20:43]

.

[oka, 2017-03-28 10:24:55]

.

[oka, 2017-03-28 10:28:08]

.

[oka, 2017-03-28 10:33:33]

Mark web_contents as protected.

[oka, 2017-03-28 11:13:18]

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

[oka, 2017-03-28 11:13:26]

PTAL.

https://codereview.chromium.org/2771793002/diff/60001/chrome/browser/ui/ash/k...
File chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc (right):

https://codereview.chromium.org/2771793002/diff/60001/chrome/browser/ui/ash/k...
chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc:71: // Initialized in
|SetUpOnMainThread|.
On 2017/03/27 16:57:54, oshima wrote:
> nit:
> 
> private:
> 
> and DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200 ms to
make sure virtual keyboard is hidden.
On 2017/03/27 16:57:54, oshima wrote:
> it's not clear why we need this. Shouldn't keyboard already be hidden?

Unfortunately no. The task to hide keyboard is delayed to avoid flickering when
focus is moved from a text input to another text input. In this case text input
type = NONE is sent to the keyboard controller once. If there is no delay,
keyboard controller would close the keyboard for NONE and then reopens it. To
avoid this, keyboard controller delays closing and cancels closing if a input
element gets focus within 100 ms.
Updated the test comment.

[commit-bot: I haz the power, 2017-03-28 11:13:39]

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, 2017-03-28 11:15:08]

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

[commit-bot: I haz the power, 2017-03-28 11:15:08]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[oshima, 2017-03-28 12:00:25]

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200 ms to
make sure virtual keyboard is hidden.
On 2017/03/28 11:13:26, oka wrote:
> On 2017/03/27 16:57:54, oshima wrote:
> > it's not clear why we need this. Shouldn't keyboard already be hidden?
> 
> Unfortunately no. The task to hide keyboard is delayed to avoid flickering
when
> focus is moved from a text input to another text input. In this case text
input
> type = NONE is sent to the keyboard controller once. If there is no delay,
> keyboard controller would close the keyboard for NONE and then reopens it. To
> avoid this, keyboard controller delays closing and cancels closing if a input
> element gets focus within 100 ms.

Whey it reopens it? Shouldn't it stay hidden because setting focus async won't
open keyboard?

It looks odd if html code has to wait in order to avoid showing keyboard.

> Updated the test comment.

[oka, 2017-03-28 12:50:03]

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):

https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200 ms to
make sure virtual keyboard is hidden.
On 2017/03/28 12:00:25, oshima wrote:
> On 2017/03/28 11:13:26, oka wrote:
> > On 2017/03/27 16:57:54, oshima wrote:
> > > it's not clear why we need this. Shouldn't keyboard already be hidden?
> > 
> > Unfortunately no. The task to hide keyboard is delayed to avoid flickering
> when
> > focus is moved from a text input to another text input. In this case text
> input
> > type = NONE is sent to the keyboard controller once. If there is no delay,
> > keyboard controller would close the keyboard for NONE and then reopens it.
To
> > avoid this, keyboard controller delays closing and cancels closing if a
input
> > element gets focus within 100 ms.
> 
> Whey it reopens it? Shouldn't it stay hidden because setting focus async won't
> open keyboard?
> 
> It looks odd if html code has to wait in order to avoid showing keyboard.
> 
> > Updated the test comment.
> 

Sorry to make confusion, but in the previous comment 
"If there is no delay, keyboard controller would close the keyboard for NONE and
then reopens it."
the delay means the 100 ms of delay keyboard controller has.

There are three states in virtual keyboard; shown, willHidden and hidden.
Once NONE is received, keyboard moves to willHidden state and then it moves to
hidden state after 100ms. If some text input element has focus before that
keyboard moves to shown state. This is for preventing flickering.

By this reason, if the keyboard is opening and then 'async' is clicked, keyboard
will not be hidden otherwise the delay of 200ms is specified.
If the keyboard is not opening when user clicks 'async', then the keyboard will
not be shown even if there is no delay specified in setTimeout.

That said, I agree that the test looks strange. Current test checks clicking
'async' hides *opening* keyboard, but it requires to add 200 ms of delay. Maybe
we should change the strategy to check clicking async keeps keyboard closing. It
will work if 200 is replaced with 0. Question for this approach is how to ckeck
the keyboard is "still hidden". keyboard::WaitUntilHidden can't be used since
there is not state change.
We need a way to wait until input.focus() is called in the javascript side for
the test to be meaningful.

[oshima, 2017-03-30 04:23:15]

On 2017/03/28 12:50:03, oka wrote:
>
https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
> File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):
> 
>
https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
> chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200 ms to
> make sure virtual keyboard is hidden.
> On 2017/03/28 12:00:25, oshima wrote:
> > On 2017/03/28 11:13:26, oka wrote:
> > > On 2017/03/27 16:57:54, oshima wrote:
> > > > it's not clear why we need this. Shouldn't keyboard already be hidden?
> > > 
> > > Unfortunately no. The task to hide keyboard is delayed to avoid flickering
> > when
> > > focus is moved from a text input to another text input. In this case text
> > input
> > > type = NONE is sent to the keyboard controller once. If there is no delay,
> > > keyboard controller would close the keyboard for NONE and then reopens it.
> To
> > > avoid this, keyboard controller delays closing and cancels closing if a
> input
> > > element gets focus within 100 ms.
> > 
> > Whey it reopens it? Shouldn't it stay hidden because setting focus async
won't
> > open keyboard?
> > 
> > It looks odd if html code has to wait in order to avoid showing keyboard.
> > 
> > > Updated the test comment.
> > 
> 
> Sorry to make confusion, but in the previous comment 
> "If there is no delay, keyboard controller would close the keyboard for NONE
and
> then reopens it."
> the delay means the 100 ms of delay keyboard controller has.
> 
> There are three states in virtual keyboard; shown, willHidden and hidden.
> Once NONE is received, keyboard moves to willHidden state and then it moves to
> hidden state after 100ms. If some text input element has focus before that
> keyboard moves to shown state. This is for preventing flickering.

so my understanding was:

[sync focused] --> shown --[move focus to async: emit NONE] --> willHidden --
100ms --> hidden?

[sync focused] --> shown --[move to another sync: emit NONE and SHOW?] -->
willHiden + show --> shown

am I wrong?

> 
> By this reason, if the keyboard is opening and then 'async' is clicked,
keyboard
> will not be hidden otherwise the delay of 200ms is specified.
> If the keyboard is not opening when user clicks 'async', then the keyboard
will
> not be shown even if there is no delay specified in setTimeout.
> 
> That said, I agree that the test looks strange. Current test checks clicking
> 'async' hides *opening* keyboard, but it requires to add 200 ms of delay.
Maybe
> we should change the strategy to check clicking async keeps keyboard closing.
It
> will work if 200 is replaced with 0. Question for this approach is how to
ckeck
> the keyboard is "still hidden". keyboard::WaitUntilHidden can't be used since
> there is not state change.
> We need a way to wait until input.focus() is called in the javascript side for
> the test to be meaningful.

[oka, 2017-03-30 05:03:18]

Your understanding is correct.

2017年3月30日(木) 13:23 <oshima@chromium.org>:

> On 2017/03/28 12:50:03, oka wrote:
> >
>
>
https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
> > File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):
> >
> >
>
>
https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
> > chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200
> ms to
> > make sure virtual keyboard is hidden.
> > On 2017/03/28 12:00:25, oshima wrote:
> > > On 2017/03/28 11:13:26, oka wrote:
> > > > On 2017/03/27 16:57:54, oshima wrote:
> > > > > it's not clear why we need this. Shouldn't keyboard already be
> hidden?
> > > >
> > > > Unfortunately no. The task to hide keyboard is delayed to avoid
> flickering
> > > when
> > > > focus is moved from a text input to another text input. In this case
> text
> > > input
> > > > type = NONE is sent to the keyboard controller once. If there is no
> delay,
> > > > keyboard controller would close the keyboard for NONE and then
> reopens it.
> > To
> > > > avoid this, keyboard controller delays closing and cancels closing
> if a
> > input
> > > > element gets focus within 100 ms.
> > >
> > > Whey it reopens it? Shouldn't it stay hidden because setting focus
> async
> won't
> > > open keyboard?
> > >
> > > It looks odd if html code has to wait in order to avoid showing
> keyboard.
> > >
> > > > Updated the test comment.
> > >
> >
> > Sorry to make confusion, but in the previous comment
> > "If there is no delay, keyboard controller would close the keyboard for
> NONE
> and
> > then reopens it."
> > the delay means the 100 ms of delay keyboard controller has.
> >
> > There are three states in virtual keyboard; shown, willHidden and hidden.
> > Once NONE is received, keyboard moves to willHidden state and then it
> moves to
> > hidden state after 100ms. If some text input element has focus before
> that
> > keyboard moves to shown state. This is for preventing flickering.
>
> so my understanding was:
>
> [sync focused] --> shown --[move focus to async: emit NONE] --> willHidden
> --
> 100ms --> hidden?
>
> [sync focused] --> shown --[move to another sync: emit NONE and SHOW?] -->
> willHiden + show --> shown
>
> am I wrong?
>
>
> >
> > By this reason, if the keyboard is opening and then 'async' is clicked,
> keyboard
> > will not be hidden otherwise the delay of 200ms is specified.
> > If the keyboard is not opening when user clicks 'async', then the
> keyboard
> will
> > not be shown even if there is no delay specified in setTimeout.
> >
> > That said, I agree that the test looks strange. Current test checks
> clicking
> > 'async' hides *opening* keyboard, but it requires to add 200 ms of delay.
> Maybe
> > we should change the strategy to check clicking async keeps keyboard
> closing.
> It
> > will work if 200 is replaced with 0. Question for this approach is how to
> ckeck
> > the keyboard is "still hidden". keyboard::WaitUntilHidden can't be used
> since
> > there is not state change.
> > We need a way to wait until input.focus() is called in the javascript
> side for
> > the test to be meaningful.
>
>
>
> https://codereview.chromium.org/2771793002/
>

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

[oshima, 2017-03-30 13:47:52]

On 2017/03/30 05:03:18, oka wrote:
> Your understanding is correct.

If so, what's the issue in simply waiting until it's hidden?

> 
> 2017年3月30日(木) 13:23 <mailto:oshima@chromium.org>:
> 
> > On 2017/03/28 12:50:03, oka wrote:
> > >
> >
> >
>
https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
> > > File chrome/test/data/chromeos/virtual_keyboard/inputs.html (right):
> > >
> > >
> >
> >
>
https://codereview.chromium.org/2771793002/diff/60001/chrome/test/data/chrome...
> > > chrome/test/data/chromeos/virtual_keyboard/inputs.html:16: // wait 200
> > ms to
> > > make sure virtual keyboard is hidden.
> > > On 2017/03/28 12:00:25, oshima wrote:
> > > > On 2017/03/28 11:13:26, oka wrote:
> > > > > On 2017/03/27 16:57:54, oshima wrote:
> > > > > > it's not clear why we need this. Shouldn't keyboard already be
> > hidden?
> > > > >
> > > > > Unfortunately no. The task to hide keyboard is delayed to avoid
> > flickering
> > > > when
> > > > > focus is moved from a text input to another text input. In this case
> > text
> > > > input
> > > > > type = NONE is sent to the keyboard controller once. If there is no
> > delay,
> > > > > keyboard controller would close the keyboard for NONE and then
> > reopens it.
> > > To
> > > > > avoid this, keyboard controller delays closing and cancels closing
> > if a
> > > input
> > > > > element gets focus within 100 ms.
> > > >
> > > > Whey it reopens it? Shouldn't it stay hidden because setting focus
> > async
> > won't
> > > > open keyboard?
> > > >
> > > > It looks odd if html code has to wait in order to avoid showing
> > keyboard.
> > > >
> > > > > Updated the test comment.
> > > >
> > >
> > > Sorry to make confusion, but in the previous comment
> > > "If there is no delay, keyboard controller would close the keyboard for
> > NONE
> > and
> > > then reopens it."
> > > the delay means the 100 ms of delay keyboard controller has.
> > >
> > > There are three states in virtual keyboard; shown, willHidden and hidden.
> > > Once NONE is received, keyboard moves to willHidden state and then it
> > moves to
> > > hidden state after 100ms. If some text input element has focus before
> > that
> > > keyboard moves to shown state. This is for preventing flickering.
> >
> > so my understanding was:
> >
> > [sync focused] --> shown --[move focus to async: emit NONE] --> willHidden
> > --
> > 100ms --> hidden?
> >
> > [sync focused] --> shown --[move to another sync: emit NONE and SHOW?] -->
> > willHiden + show --> shown
> >
> > am I wrong?
> >
> >
> > >
> > > By this reason, if the keyboard is opening and then 'async' is clicked,
> > keyboard
> > > will not be hidden otherwise the delay of 200ms is specified.
> > > If the keyboard is not opening when user clicks 'async', then the
> > keyboard
> > will
> > > not be shown even if there is no delay specified in setTimeout.
> > >
> > > That said, I agree that the test looks strange. Current test checks
> > clicking
> > > 'async' hides *opening* keyboard, but it requires to add 200 ms of delay.
> > Maybe
> > > we should change the strategy to check clicking async keeps keyboard
> > closing.
> > It
> > > will work if 200 is replaced with 0. Question for this approach is how to
> > ckeck
> > > the keyboard is "still hidden". keyboard::WaitUntilHidden can't be used
> > since
> > > there is not state change.
> > > We need a way to wait until input.focus() is called in the javascript
> > side for
> > > the test to be meaningful.
> >
> >
> >
> > https://codereview.chromium.org/2771793002/
> >
> 
> -- 
> 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.

[oka, 2017-07-06 23:35:23]

nit

[oka, 2017-07-06 23:37:17]

fix lint error

[oka, 2017-07-06 23:38:19]

fix lint error

[oshima, 2017-07-07 00:45:24]

On 2017/07/06 23:38:19, oka wrote:
> fix lint error

is this ready to review?

[oka, 2017-07-07 01:11:35]

Not yet. Sorry

2017年7月7日(金) 12:45 <oshima@chromium.org>:

> On 2017/07/06 23:38:19, oka wrote:
> > fix lint error
>
> is this ready to review?
>
> https://codereview.chromium.org/2771793002/
>

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

[oka, 2017-07-07 08:17:20]

Description was changed from

==========
End to end test: async focus shouldn't trigger virtual keyboard.

BUG=703488
TESt=try
==========

to

==========
End to end test: async focus shouldn't trigger virtual keyboard.

BUG=703488
TESt=try
==========

[oka, 2017-07-07 08:17:21]

oka@chromium.org changed reviewers:
- oshima@chromium.org

[oka, 2017-07-08 00:26:03]

Rebase on top of another change.

[oka, 2017-07-09 06:11:23]

Clean up

[oka, 2017-07-09 06:54:32]

nit

[oka, 2017-07-10 01:48:42]

rebase

[oka, 2017-07-13 02:06:30]

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

[commit-bot: I haz the power, 2017-07-13 02:06:45]

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, 2017-07-13 02:34:13]

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

[commit-bot: I haz the power, 2017-07-13 02:34:14]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)