Fix cropped floating gesture candidate window

Fix cropped floating gesture candidate window

This CL introduces ShouldWindowOverscroll function in
keyboard_controller_proxy to check if a window should
be overscrolled. There are two types of window that we
want to avoid overscroll all the time: virtual keyboard
window and ime windows. Ime windows are created by
chrome.app.window.create API. This window is used to
show gesture typing candidate or accents characters.
These two type of windows have the same parent, which is
kShellWindowId_ImeWindowParentContainer top level window.
To disable overscroll, this CL check if a window is a child of
the toplevel window and avoid set overscroll insets if it is.

BUG=529880
Committed: https://crrev.com/c50df996d949d687c0eb7014cf1bdc4162f85c6b
Cr-Commit-Position: refs/heads/master@{#354055}

[bshe, 2015-09-09 19:01:27]

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

[bshe, 2015-09-09 19:01:28]

Hi Sadrul. Can you take a look at this CL?
Thanks!

[sadrul, 2015-09-10 18:20:17]

On 2015/09/09 19:01:28, bshe wrote:
> Hi Sadrul. Can you take a look at this CL?
> Thanks!

Can you explain what the problem is, and how this change fixes that?

[bshe, 2015-09-10 18:46:30]

On 2015/09/10 18:20:17, sadrul wrote:
> On 2015/09/09 19:01:28, bshe wrote:
> > Hi Sadrul. Can you take a look at this CL?
> > Thanks!
> 
> Can you explain what the problem is, and how this change fixes that?

I added more description in this cl, please let me know if anything is still not
clear. Also,
crbug.com/529880 contains a screenshot of the problem.

Basically, the gesture candidate window appeared to be cropped when moved to
near the top
of virtual keyboard. The crop is because of overscroll. So this cl simply
disable overscroll on
these windows by avoid set window inset to these windows.

[sadrul, 2015-09-10 19:08:23]

On 2015/09/10 18:46:30, bshe wrote:
> On 2015/09/10 18:20:17, sadrul wrote:
> > On 2015/09/09 19:01:28, bshe wrote:
> > > Hi Sadrul. Can you take a look at this CL?
> > > Thanks!
> > 
> > Can you explain what the problem is, and how this change fixes that?
> 
> I added more description in this cl, please let me know if anything is still
not
> clear. Also,
> crbug.com/529880 contains a screenshot of the problem.
> 
> Basically, the gesture candidate window appeared to be cropped when moved to
> near the top
> of virtual keyboard. The crop is because of overscroll. So this cl simply
> disable overscroll on
> these windows by avoid set window inset to these windows.

What is the code that ensures that the IME window is a sibling to the
keyboard-container window?

[bshe, 2015-09-10 19:11:42]

On 2015/09/10 19:08:23, sadrul wrote:
> On 2015/09/10 18:46:30, bshe wrote:
> > On 2015/09/10 18:20:17, sadrul wrote:
> > > On 2015/09/09 19:01:28, bshe wrote:
> > > > Hi Sadrul. Can you take a look at this CL?
> > > > Thanks!
> > > 
> > > Can you explain what the problem is, and how this change fixes that?
> > 
> > I added more description in this cl, please let me know if anything is still
> not
> > clear. Also,
> > crbug.com/529880 contains a screenshot of the problem.
> > 
> > Basically, the gesture candidate window appeared to be cropped when moved to
> > near the top
> > of virtual keyboard. The crop is because of overscroll. So this cl simply
> > disable overscroll on
> > these windows by avoid set window inset to these windows.
> 
> What is the code that ensures that the IME window is a sibling to the
> keyboard-container window?

I believe it is here:
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...

[sadrul, 2015-09-10 19:28:36]

On 2015/09/10 19:11:42, bshe wrote:
> On 2015/09/10 19:08:23, sadrul wrote:
> > On 2015/09/10 18:46:30, bshe wrote:
> > > On 2015/09/10 18:20:17, sadrul wrote:
> > > > On 2015/09/09 19:01:28, bshe wrote:
> > > > > Hi Sadrul. Can you take a look at this CL?
> > > > > Thanks!
> > > > 
> > > > Can you explain what the problem is, and how this change fixes that?
> > > 
> > > I added more description in this cl, please let me know if anything is
still
> > not
> > > clear. Also,
> > > crbug.com/529880 contains a screenshot of the problem.
> > > 
> > > Basically, the gesture candidate window appeared to be cropped when moved
to
> > > near the top
> > > of virtual keyboard. The crop is because of overscroll. So this cl simply
> > > disable overscroll on
> > > these windows by avoid set window inset to these windows.
> > 
> > What is the code that ensures that the IME window is a sibling to the
> > keyboard-container window?
> 
> I believe it is here:
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...

Some comments:

(1) Looks like that code is using the kShellWindowId_ImeWindowParentContainer
container, and the virtual keyboard uses the
kShellWindowId_VirtualKeyboardContainer container. I assume VK container is a
child of the IME container?

(2) I don't think we can make such assumptions (about the IME candidate window
being a sibling of the keyboard container) in //ui/keyboard/, which lives so far
away from the code that is doing these things (ash/, and chrome/).

(3) Why does ash::RootWindowController::IsVirtualKeyboardWindow() look at the
ime container, instead of the VK container? Should the function return true for
an ime-candidate window?

[bshe, 2015-09-14 20:23:51]

Thanks for review! PTAL. I have addressed your comment inline. If it looks good,
I will then try to add test.

On 2015/09/10 19:28:36, sadrul wrote:
> On 2015/09/10 19:11:42, bshe wrote:
> > On 2015/09/10 19:08:23, sadrul wrote:
> > > On 2015/09/10 18:46:30, bshe wrote:
> > > > On 2015/09/10 18:20:17, sadrul wrote:
> > > > > On 2015/09/09 19:01:28, bshe wrote:
> > > > > > Hi Sadrul. Can you take a look at this CL?
> > > > > > Thanks!
> > > > > 
> > > > > Can you explain what the problem is, and how this change fixes that?
> > > > 
> > > > I added more description in this cl, please let me know if anything is
> still
> > > not
> > > > clear. Also,
> > > > crbug.com/529880 contains a screenshot of the problem.
> > > > 
> > > > Basically, the gesture candidate window appeared to be cropped when
moved
> to
> > > > near the top
> > > > of virtual keyboard. The crop is because of overscroll. So this cl
simply
> > > > disable overscroll on
> > > > these windows by avoid set window inset to these windows.
> > > 
> > > What is the code that ensures that the IME window is a sibling to the
> > > keyboard-container window?
> > 
> > I believe it is here:
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...
> 
> Some comments:
> 
> (1) Looks like that code is using the kShellWindowId_ImeWindowParentContainer
> container, and the virtual keyboard uses the
> kShellWindowId_VirtualKeyboardContainer container. I assume VK container is a
> child of the IME container?
Yes, VK Container is a child of the IME container.

> 
> (2) I don't think we can make such assumptions (about the IME candidate window
> being a sibling of the keyboard container) in //ui/keyboard/, which lives so
far
> away from the code that is doing these things (ash/, and chrome/).
Make sense. I added a function in keyboard_controller_proxy and moved the logic
to ash_keyboard_controller_proxy. Not sure if this is the best way so feel free
to
give any advice.

> 
> (3) Why does ash::RootWindowController::IsVirtualKeyboardWindow() look at the
> ime container, instead of the VK container? Should the function return true
for
> an ime-candidate window?
Ime windows are currently only used to show UI. They controlled by virtual
keyboard
window and dont have any event listener as far a I know. So this function
should not return true for them. I changed it id to
kShellWindowId_VirtualKeyboardContainer.

[sadrul, 2015-09-14 20:28:09]

Nice! This looks much better.

https://codereview.chromium.org/1323053005/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/1323053005/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:681: aura::Window* parent =
GetContainer(kShellWindowId_VirtualKeyboardContainer);
It's unclear to me if this can break existing code that makes incorrect
assumptions about vk-window and ime-candidate windows. So this can possibly be a
separate CL, so that it can be reverted if something breaks.

[sadrul, 2015-09-14 20:36:50]

On 2015/09/14 20:23:51, bshe wrote:
> Thanks for review! PTAL. I have addressed your comment inline. If it looks
good,
> I will then try to add test.

Also, please make sure to update the CL description.

[bshe, 2015-09-15 13:09:33]

Done. PTAL. Thanks!

https://codereview.chromium.org/1323053005/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/1323053005/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:681: aura::Window* parent =
GetContainer(kShellWindowId_VirtualKeyboardContainer);
On 2015/09/14 20:28:09, sadrul wrote:
> It's unclear to me if this can break existing code that makes incorrect
> assumptions about vk-window and ime-candidate windows. So this can possibly be
a
> separate CL, so that it can be reverted if something breaks.

make sense. reverted it in this CL and will upload a separate CL.

[sadrul, 2015-09-15 16:19:16]

On 2015/09/14 20:23:51, bshe wrote:
> Thanks for review! PTAL. I have addressed your comment inline. If it looks
good,
> I will then try to add test.

Did you have a test?

[bshe, 2015-09-23 12:42:15]

Patchset #4 (id:60001) has been deleted

[bshe, 2015-09-23 12:43:23]

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

[bshe, 2015-09-23 12:43:24]

Added a test. PTAL. Thanks

also +oshima for owner Thanks!

[oshima, 2015-09-23 19:57:27]

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:190: if
(!root_window_controller)
can this happen? If not, please use DCHECK instead.

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller.cc:282: window->GetRootWindow() == root_window)
{
looks like this check is a dup?

[bshe, 2015-09-24 11:56:15]

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:190: if
(!root_window_controller)
On 2015/09/23 19:57:27, oshima wrote:
> can this happen? If not, please use DCHECK instead.

I think so. One window (I am not sure which) doesnt seem to have a root window
and crashed Chrome without the check.

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller.cc:282: window->GetRootWindow() == root_window)
{
On 2015/09/23 19:57:27, oshima wrote:
> looks like this check is a dup?

I think this check preventing overscoll on windows that are not appear in the
same root window as the virtual keyboard window. So it is probably not a dup? I
can move the logic into "ShouldWindowOverscroll" though. WDYT?

[oshima, 2015-09-24 17:28:07]

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:190: if
(!root_window_controller)
On 2015/09/24 11:56:15, bshe wrote:
> On 2015/09/23 19:57:27, oshima wrote:
> > can this happen? If not, please use DCHECK instead.
> 
> I think so. One window (I am not sure which) doesnt seem to have a root window
> and crashed Chrome without the check.

can you check the root window instead then?

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller.cc:282: window->GetRootWindow() == root_window)
{
On 2015/09/24 11:56:15, bshe wrote:
> On 2015/09/23 19:57:27, oshima wrote:
> > looks like this check is a dup?
> 
> I think this check preventing overscoll on windows that are not appear in the
> same root window as the virtual keyboard window. So it is probably not a dup?
I
> can move the logic into "ShouldWindowOverscroll" though. WDYT?

Yeah,that sounds better because it's checking against the keyboard window
obtained from the pxory.

[bshe, 2015-09-28 14:42:40]

PTAL, Thanks!

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:190: if
(!root_window_controller)
On 2015/09/24 17:28:07, oshima wrote:
> On 2015/09/24 11:56:15, bshe wrote:
> > On 2015/09/23 19:57:27, oshima wrote:
> > > can this happen? If not, please use DCHECK instead.
> > 
> > I think so. One window (I am not sure which) doesnt seem to have a root
window
> > and crashed Chrome without the check.
> 
> can you check the root window instead then?

Done.

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/1323053005/diff/80001/ui/keyboard/keyboard_co...
ui/keyboard/keyboard_controller.cc:282: window->GetRootWindow() == root_window)
{
On 2015/09/24 17:28:07, oshima wrote:
> On 2015/09/24 11:56:15, bshe wrote:
> > On 2015/09/23 19:57:27, oshima wrote:
> > > looks like this check is a dup?
> > 
> > I think this check preventing overscoll on windows that are not appear in
the
> > same root window as the virtual keyboard window. So it is probably not a
dup?
> I
> > can move the logic into "ShouldWindowOverscroll" though. WDYT?
> 
> Yeah,that sounds better because it's checking against the keyboard window
> obtained from the pxory.

Done.

[oshima, 2015-09-30 19:29:03]

lgtm

[sadrul, 2015-10-01 04:16:20]

lgtm

[bshe, 2015-10-13 14:29:01]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2015-10-13 14:36:10]

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

[commit-bot: I haz the power, 2015-10-13 14:46:41]

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

[commit-bot: I haz the power, 2015-10-13 14:46:42]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bshe, 2015-10-14 15:36:47]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-10-14 15:36:47]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1323053005/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-14 15:37:35]

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

[commit-bot: I haz the power, 2015-10-14 15:50:17]

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

[commit-bot: I haz the power, 2015-10-14 15:50:18]

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

[bshe, 2015-10-14 17:31:04]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-10-14 17:31:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1323053005/#ps140001
(title: "fix compile")

[commit-bot: I haz the power, 2015-10-14 17:31:27]

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

[commit-bot: I haz the power, 2015-10-14 17:37:10]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-10-14 17:38:59]

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