Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553603002:
New accessibility virtual keyboard behavior in non-sticky mode. (Closed)
|
Created:
4 years ago by yhanada Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, sadrul, yusukes+watch_chromium.org, tfarina, shuchen+watch_chromium.org, hidehiko+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, lhchavez+watch_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew accessibility virtual keyboard behavior in non-sticky mode.
Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The
kUseNewVirtualKeyboardBehavior switch must be enabled for this
behavior.
BUG=624521
TEST=Unit tests are added in render_widget_host_view_aura_unittest.cc
and textfield_unittest.cc.
Review-Url: https://codereview.chromium.org/2553603002
Cr-Commit-Position: refs/heads/master@{#470517}
Committed: https://chromium.googlesource.com/chromium/src/+/268490faabdd39308b8f6955408172f29e590bdd
Patch Set 1 #Patch Set 2 : reupload #
Total comments: 2
Patch Set 3 : address the comment #Patch Set 4 : fix test crashes #
Total comments: 11
Patch Set 5 : address the comments #
Total comments: 14
Patch Set 6 : address oshima@'s comments and fixed test crashes #Patch Set 7 : add a dependency patchset #Patch Set 8 #Patch Set 9 : fix a test failure #Patch Set 10 #Patch Set 11 : create ime_util in wm/core #Patch Set 12 : rebase #
Total comments: 4
Patch Set 13 : rebase #Patch Set 14 : Remove TextInputClient::OnClientFocusLost() #
Total comments: 8
Patch Set 15 #Patch Set 16 : Remove unused variable #
Total comments: 7
Patch Set 17 : create wm::EnsureWindowNotInRect #Patch Set 18 : Fix the build error on Linux and Windows #
Total comments: 8
Patch Set 19 : Renamed to ime_util_chromeos and added the unittests #
Total comments: 15
Patch Set 20 : address sky@'s comments #Patch Set 21 : address oshima@'s comments #Patch Set 22 : rebase #
Total comments: 12
Patch Set 23 : address sadrul@'s comments #Patch Set 24 : rebase #Messages
Total messages: 146 (103 generated)
Description was changed from
==========
New accessibility virtual keyboard behavior in non-sticky mode.
Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The
kUseNewVirtualKeyboardBehavior switch must be enabled for this
behavior.
BUG=624521
==========
to
==========
New accessibility virtual keyboard behavior in non-sticky mode.
Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The
kUseNewVirtualKeyboardBehavior switch must be enabled for this
behavior.
BUG=624521
TEST=Unit tests are added in render_widget_host_view_aura_unittest.cc
and textfield_unittest.cc.
==========
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yhanada@chromium.org changed reviewers: + oshima@chromium.org, sadrul@chromium.org, sky@chromium.org
This comes from https://crrev.com/2259033003. PTAL. Thank you!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2553603002/diff/20001/ui/base/ime/input_method.h File ui/base/ime/input_method.h (right): https://codereview.chromium.org/2553603002/diff/20001/ui/base/ime/input_metho... ui/base/ime/input_method.h:165: virtual void SetCoveredBounds(const gfx::Rect& new_bounds) {} InputMethod::SetCoveredBounds() doesn't make a lot of sense without the context that it's meant for virtual/onscreen keyboard. Can this be renamed to make that more obvious? Also, what does 'covered screen bounds' mean? The bounds of the onscreen keyboard? Or the bounds of the occluded region of some window?
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm sorry for the late reply. PTAL. https://codereview.chromium.org/2553603002/diff/20001/ui/base/ime/input_method.h File ui/base/ime/input_method.h (right): https://codereview.chromium.org/2553603002/diff/20001/ui/base/ime/input_metho... ui/base/ime/input_method.h:165: virtual void SetCoveredBounds(const gfx::Rect& new_bounds) {} On 2016/12/08 02:57:07, sadrul wrote: > InputMethod::SetCoveredBounds() doesn't make a lot of sense without the context > that it's meant for virtual/onscreen keyboard. Can this be renamed to make that > more obvious? Also, what does 'covered screen bounds' mean? The bounds of the > onscreen keyboard? Or the bounds of the occluded region of some window? Renamed to |SetOnScreenKeyboardBounds|.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1436: OnClientFocusLost(); Can this be called when VK size changed for some reason? If so, you need to compute based on the original window bounds for cros. https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1463: } This moving/restoring logic should be almost same for TextField and contents. Can you define a utility functions/class in ui/chromeos/ime and use it here? https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1485: aura::client::kVirtualKeyboardRestoreBoundsKey); a user might have moved a window, and in that case, we may want to just leave it. Can you add TODO and discuss with Tom? https://codereview.chromium.org/2553603002/diff/60001/ui/base/ime/text_input_... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2553603002/diff/60001/ui/base/ime/text_input_... ui/base/ime/text_input_client.h:175: virtual void OnClientFocusLost() {} Won't this happens when a user closes VK, or VK is moved to another display?
I'm sorry for the delay. I'm still working on some test failures. https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1436: OnClientFocusLost(); On 2016/12/13 18:15:07, oshima wrote: > Can this be called when VK size changed for some reason? If so, you need to > compute based on the original window bounds for cros. Yes. Changed to use the original window bounds if exists. https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1463: } On 2016/12/13 18:15:07, oshima wrote: > This moving/restoring logic should be almost same for TextField and contents. > Can you define a utility functions/class in ui/chromeos/ime and use it here? ui/chromeos/ime depends on ui/views, so this logic can't be in ui/chromeos/ime. How about ui/base/ime? Can I create a utility function in ui/base/ime? https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1485: aura::client::kVirtualKeyboardRestoreBoundsKey); On 2016/12/13 18:15:07, oshima wrote: > a user might have moved a window, and in that case, we may want to just leave > it. Can you add TODO and discuss with Tom? Done. https://codereview.chromium.org/2553603002/diff/60001/ui/base/ime/text_input_... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2553603002/diff/60001/ui/base/ime/text_input_... ui/base/ime/text_input_client.h:175: virtual void OnClientFocusLost() {} On 2016/12/13 18:15:07, oshima wrote: > Won't this happens when a user closes VK, or VK is moved to another display? This will be also called when a user closes VK by |EnsureCaretNotInRect|. Should I change the name?
https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1463: } On 2017/01/12 02:19:56, yhanada wrote: > On 2016/12/13 18:15:07, oshima wrote: > > This moving/restoring logic should be almost same for TextField and contents. > > Can you define a utility functions/class in ui/chromeos/ime and use it here? > > ui/chromeos/ime depends on ui/views, so this logic can't be in ui/chromeos/ime. > How about ui/base/ime? Can I create a utility function in ui/base/ime? ui/base shouldn't depend on aura. How about ui/wm/ime? Please consult with sky@ as well. https://codereview.chromium.org/2553603002/diff/60001/ui/base/ime/text_input_... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2553603002/diff/60001/ui/base/ime/text_input_... ui/base/ime/text_input_client.h:175: virtual void OnClientFocusLost() {} On 2017/01/12 02:19:56, yhanada wrote: > On 2016/12/13 18:15:07, oshima wrote: > > Won't this happens when a user closes VK, or VK is moved to another display? > > This will be also called when a user closes VK by |EnsureCaretNotInRect|. Should > I change the name? On second thought, let's keep it. https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1271: gfx::Rect RenderWidgetHostViewAura::ConvertRectFromScreen( this no longer depends on RWHVAura. Can you move this to anonymous namespace in this cc file? https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1437: const gfx::Rect hiding_area_in_screen = how about hidden_window_bounds_in_screen? https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1453: const int vertical_displacement = std::max(0, hiding_area_in_screen.height()); the height should be always positive, right? https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1455: top_level_window->GetBoundsInRootWindow().y()); how about: int top_y = std::max(vk_bounds.y() - toplevel_bounds.height(), 0); if (toplevel_bounds.y() != top_y) { // move } ? https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1488: LOG(ERROR) << "OnClientFocusLost is called"; nit: remove debug log https://codereview.chromium.org/2553603002/diff/80001/ui/base/ime/input_metho... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2553603002/diff/80001/ui/base/ime/input_metho... ui/base/ime/input_method_base.cc:67: LOG(ERROR) << "Called SetOnScreenKeyboardBounds: " << new_bounds.ToString(); nit: remove debug log https://codereview.chromium.org/2553603002/diff/80001/ui/base/ime/input_metho... ui/base/ime/input_method_base.cc:157: << ", new = " << client; nit: debug log
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. I added a refactoring change (http://crrev.com/2660873002) as a dependency CL of this CL. Please also take a look at it. Thanks! https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1463: } On 2017/01/18 02:37:19, oshima wrote: > On 2017/01/12 02:19:56, yhanada wrote: > > On 2016/12/13 18:15:07, oshima wrote: > > > This moving/restoring logic should be almost same for TextField and > contents. > > > Can you define a utility functions/class in ui/chromeos/ime and use it here? > > > > ui/chromeos/ime depends on ui/views, so this logic can't be in > ui/chromeos/ime. > > How about ui/base/ime? Can I create a utility function in ui/base/ime? > > ui/base shouldn't depend on aura. How about ui/wm/ime? Please consult with sky@ > as well. I understand. sky@: I intend to create a utility function under ui/wm/ime or ui/wm/core/ime. What do you think? https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1271: gfx::Rect RenderWidgetHostViewAura::ConvertRectFromScreen( On 2017/01/18 02:37:19, oshima wrote: > this no longer depends on RWHVAura. Can you move this to anonymous namespace in > this cc file? Done. https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1437: const gfx::Rect hiding_area_in_screen = On 2017/01/18 02:37:19, oshima wrote: > how about hidden_window_bounds_in_screen? Done. https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1453: const int vertical_displacement = std::max(0, hiding_area_in_screen.height()); On 2017/01/18 02:37:19, oshima wrote: > the height should be always positive, right? Removed this variable. https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1455: top_level_window->GetBoundsInRootWindow().y()); On 2017/01/18 02:37:19, oshima wrote: > how about: > > int top_y = std::max(vk_bounds.y() - toplevel_bounds.height(), 0); > > if (toplevel_bounds.y() != top_y) { > // move > } > > ? Done. https://codereview.chromium.org/2553603002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1488: LOG(ERROR) << "OnClientFocusLost is called"; On 2017/01/18 02:37:19, oshima wrote: > nit: remove debug log Done. https://codereview.chromium.org/2553603002/diff/80001/ui/base/ime/input_metho... File ui/base/ime/input_method_base.cc (right): https://codereview.chromium.org/2553603002/diff/80001/ui/base/ime/input_metho... ui/base/ime/input_method_base.cc:67: LOG(ERROR) << "Called SetOnScreenKeyboardBounds: " << new_bounds.ToString(); On 2017/01/18 02:37:19, oshima wrote: > nit: remove debug log Done. https://codereview.chromium.org/2553603002/diff/80001/ui/base/ime/input_metho... ui/base/ime/input_method_base.cc:157: << ", new = " << client; On 2017/01/18 02:37:19, oshima wrote: > nit: debug log Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another look. I just created ime_util.h/cc in wm/core for utility functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another look. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shuchen@chromium.org changed reviewers: + shuchen@chromium.org
https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... File ui/base/ime/input_method.h (right): https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... ui/base/ime/input_method.h:165: virtual void SetOnScreenKeyboardBounds(const gfx::Rect& new_bounds) {} Looks like this interface method is added only for storing the keyboard_bounds in InputMethod. It would be better to avoid adding this and instead let the TextInputClient store it by itself. https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... ui/base/ime/text_input_client.h:164: virtual void OnClientFocusLost() {} Why adding this? RWHVA is a FocusChangeObserver and TextField is a View which has View::OnBlur.
Thank you for reviewing. https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... File ui/base/ime/input_method.h (right): https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... ui/base/ime/input_method.h:165: virtual void SetOnScreenKeyboardBounds(const gfx::Rect& new_bounds) {} On 2017/03/14 14:42:30, Shu Chen wrote: > Looks like this interface method is added only for storing the keyboard_bounds > in InputMethod. > It would be better to avoid adding this and instead let the TextInputClient > store it by itself. The stored keyboard bounds is used when switching TextInputClient (please see InputMethodBase::SetFocusedTextInputClient). I chose to store it in InputMethod because the keyboard bounds depends on what IME is used and the bounds is used across multiple TextInputClient. What do you think? https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... ui/base/ime/text_input_client.h:164: virtual void OnClientFocusLost() {} On 2017/03/14 14:42:30, Shu Chen wrote: > Why adding this? RWHVA is a FocusChangeObserver and TextField is a View which > has View::OnBlur. This will also be called when VK size is changed for some reason or VK moves to another display.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
friendly ping?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I assume you're pinging Oshima on this.
On 2017/03/15 12:54:32, yhanada wrote: > Thank you for reviewing. > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... > File ui/base/ime/input_method.h (right): > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... > ui/base/ime/input_method.h:165: virtual void SetOnScreenKeyboardBounds(const > gfx::Rect& new_bounds) {} > On 2017/03/14 14:42:30, Shu Chen wrote: > > Looks like this interface method is added only for storing the keyboard_bounds > > in InputMethod. > > It would be better to avoid adding this and instead let the TextInputClient > > store it by itself. > > The stored keyboard bounds is used when switching TextInputClient (please see > InputMethodBase::SetFocusedTextInputClient). I chose to store it in InputMethod > because the keyboard bounds depends on what IME is used and the bounds is used > across multiple TextInputClient. > > What do you think? Sounds good. Thanks for the clarification. > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... > File ui/base/ime/text_input_client.h (right): > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... > ui/base/ime/text_input_client.h:164: virtual void OnClientFocusLost() {} > On 2017/03/14 14:42:30, Shu Chen wrote: > > Why adding this? RWHVA is a FocusChangeObserver and TextField is a View which > > has View::OnBlur. > > This will also be called when VK size is changed for some reason or VK moves to > another display. I think the focus updates can only be from Views/Aura to IMF. This interface adds the complexity and reduce the maintainability. I would not recommend to leverage IMF as a "bridge" to pass around focus events. Instead, leveraging VK component seems more reasonable to me.
I'm sorry for the long delay. shuchen@, could you take another look? On 2017/03/28 02:25:34, Shu Chen wrote: > On 2017/03/15 12:54:32, yhanada wrote: > > Thank you for reviewing. > > > > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... > > File ui/base/ime/input_method.h (right): > > > > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/input_meth... > > ui/base/ime/input_method.h:165: virtual void SetOnScreenKeyboardBounds(const > > gfx::Rect& new_bounds) {} > > On 2017/03/14 14:42:30, Shu Chen wrote: > > > Looks like this interface method is added only for storing the > keyboard_bounds > > > in InputMethod. > > > It would be better to avoid adding this and instead let the TextInputClient > > > store it by itself. > > > > The stored keyboard bounds is used when switching TextInputClient (please see > > InputMethodBase::SetFocusedTextInputClient). I chose to store it in > InputMethod > > because the keyboard bounds depends on what IME is used and the bounds is used > > across multiple TextInputClient. > > > > What do you think? > Sounds good. Thanks for the clarification. > > > > > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... > > File ui/base/ime/text_input_client.h (right): > > > > > https://codereview.chromium.org/2553603002/diff/220001/ui/base/ime/text_input... > > ui/base/ime/text_input_client.h:164: virtual void OnClientFocusLost() {} > > On 2017/03/14 14:42:30, Shu Chen wrote: > > > Why adding this? RWHVA is a FocusChangeObserver and TextField is a View > which > > > has View::OnBlur. > > > > This will also be called when VK size is changed for some reason or VK moves > to > > another display. > I think the focus updates can only be from Views/Aura to IMF. > This interface adds the complexity and reduce the maintainability. > I would not recommend to leverage IMF as a "bridge" to pass around focus events. > Instead, leveraging VK component seems more reasonable to me. I agree with you. removed |ClientFocusLost| method from TextInputClient.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2553603002/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1424: ::switches::kUseNewVirtualKeyboardBehavior)) won't this skip overscroll? https://codereview.chromium.org/2553603002/diff/260001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2553603002/diff/260001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:101: kVirtualKeyboardRestoreBoundsKey; Can you define this in ui/wm/core/ime_util? https://codereview.chromium.org/2553603002/diff/260001/ui/wm/core/ime_util.cc File ui/wm/core/ime_util.cc (right): https://codereview.chromium.org/2553603002/diff/260001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:23: rect_in_root_window.y() - window->GetBoundsInRootWindow().height(), 0); you can use just bounds. Or store to local variable as you're using it in multiple places. https://codereview.chromium.org/2553603002/diff/260001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:33: gfx::Point new_origin(window->GetBoundsInRootWindow().x(), top_y); copy the bounds in root, and set_y
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
https://codereview.chromium.org/2553603002/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1424: ::switches::kUseNewVirtualKeyboardBehavior)) On 2017/04/11 04:48:50, oshima wrote: > won't this skip overscroll? Yes... Fixed. https://codereview.chromium.org/2553603002/diff/260001/ui/aura/client/aura_co... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/2553603002/diff/260001/ui/aura/client/aura_co... ui/aura/client/aura_constants.h:101: kVirtualKeyboardRestoreBoundsKey; On 2017/04/11 04:48:50, oshima wrote: > Can you define this in ui/wm/core/ime_util? > Done. https://codereview.chromium.org/2553603002/diff/260001/ui/wm/core/ime_util.cc File ui/wm/core/ime_util.cc (right): https://codereview.chromium.org/2553603002/diff/260001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:23: rect_in_root_window.y() - window->GetBoundsInRootWindow().height(), 0); On 2017/04/11 04:48:50, oshima wrote: > you can use just bounds. Or store to local variable as you're using it in > multiple places. Done. https://codereview.chromium.org/2553603002/diff/260001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:33: gfx::Point new_origin(window->GetBoundsInRootWindow().x(), top_y); On 2017/04/11 04:48:50, oshima wrote: > copy the bounds in root, and set_y Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for ui/base/ime/...
https://codereview.chromium.org/2553603002/diff/300001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_ui.cc (right): https://codereview.chromium.org/2553603002/diff/300001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_ui.cc:34: if (GetInputMethod()) { nit: quit early https://codereview.chromium.org/2553603002/diff/300001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2553603002/diff/300001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1524: gfx::IntersectRects(rect_in_screen, original_window_bounds); These code aren't necessary (if you want to keep this here)? https://codereview.chromium.org/2553603002/diff/300001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1529: #endif // defined(OS_CHROMEOS) The fundamental logic must be same as one in RWHVAura. Isn't it possible to define a method in ime util bool wm::EnsureWindowNotInRect(aura::Window* , const gfx::Rect& rect_in_screen, gfx::Rect* hidden_window_bounds_in_screen_out); ? https://codereview.chromium.org/2553603002/diff/300001/ui/wm/core/ime_util.h File ui/wm/core/ime_util.h (right): https://codereview.chromium.org/2553603002/diff/300001/ui/wm/core/ime_util.h#... ui/wm/core/ime_util.h:20: kVirtualKeyboardRestoreBoundsKey; I might have given you a wrong advise. sky@ mentioned that he want to keep a property definition in aura/client (probably for mus+ash). Please consult with him.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for reviewing! sky@: I added a property definition in wm/core/ime_util because it is used only in this class. Should I move the definition to aura/client? oshima@: Please take another look. Thanks! https://codereview.chromium.org/2553603002/diff/300001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_ui.cc (right): https://codereview.chromium.org/2553603002/diff/300001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_ui.cc:34: if (GetInputMethod()) { On 2017/04/17 13:24:22, oshima wrote: > nit: quit early Done. https://codereview.chromium.org/2553603002/diff/300001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2553603002/diff/300001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1524: gfx::IntersectRects(rect_in_screen, original_window_bounds); On 2017/04/17 13:24:22, oshima wrote: > These code aren't necessary (if you want to keep this here)? Done. https://codereview.chromium.org/2553603002/diff/300001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1529: #endif // defined(OS_CHROMEOS) On 2017/04/17 13:24:22, oshima wrote: > The fundamental logic must be same as one in RWHVAura. Isn't it possible to > define a method in ime util > > bool wm::EnsureWindowNotInRect(aura::Window* , const gfx::Rect& rect_in_screen, > gfx::Rect* hidden_window_bounds_in_screen_out); > > ? I moved the logic to ime_util. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Having the property definition in the place using it is fine. https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc File ui/wm/core/ime_util.cc (right): https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:16: #if defined(OS_CHROMEOS) It seems that all this code is chromeos specific. Can it move to a chromeos specific location?
https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1430: rect_in_screen, top_level_window->GetBoundsInScreen()); don't you have to check if this is empty, and return early? https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1433: wm::ConvertRectFromScreen(window_, &visible_area_in_local_space); visible area can still be empty in non ash environment, right? https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc File ui/wm/core/ime_util.cc (right): https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:16: #if defined(OS_CHROMEOS) On 2017/04/26 15:24:36, sky wrote: > It seems that all this code is chromeos specific. Can it move to a chromeos > specific location? These are used in content/browser and ui/views. They do not depend on ui/chromeos/ and want to keep them that way. How about keeping interface here and inject using delegate? (the impl should probably be in ash as its a part of wm)
https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc File ui/wm/core/ime_util.cc (right): https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc... ui/wm/core/ime_util.cc:16: #if defined(OS_CHROMEOS) On 2017/04/26 18:03:42, oshima wrote: > On 2017/04/26 15:24:36, sky wrote: > > It seems that all this code is chromeos specific. Can it move to a chromeos > > specific location? > > These are used in content/browser and ui/views. They do not depend on > ui/chromeos/ and want to keep them that way. > > How about keeping interface here and inject using delegate? (the impl should > probably be in ash as its a part of wm) I'm ok with them living here, but how about making file ime_util_chromeos and only compile on chromeos?
On 2017/04/26 19:58:20, sky wrote: > https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc > File ui/wm/core/ime_util.cc (right): > > https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc... > ui/wm/core/ime_util.cc:16: #if defined(OS_CHROMEOS) > On 2017/04/26 18:03:42, oshima wrote: > > On 2017/04/26 15:24:36, sky wrote: > > > It seems that all this code is chromeos specific. Can it move to a chromeos > > > specific location? > > > > These are used in content/browser and ui/views. They do not depend on > > ui/chromeos/ and want to keep them that way. > > > > How about keeping interface here and inject using delegate? (the impl should > > probably be in ash as its a part of wm) > > I'm ok with them living here, but how about making file ime_util_chromeos and > only compile on chromeos? Also, please add test coverage of the new function.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/26 19:58:43, sky wrote: > On 2017/04/26 19:58:20, sky wrote: > > https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc > > File ui/wm/core/ime_util.cc (right): > > > > > https://codereview.chromium.org/2553603002/diff/340001/ui/wm/core/ime_util.cc... > > ui/wm/core/ime_util.cc:16: #if defined(OS_CHROMEOS) > > On 2017/04/26 18:03:42, oshima wrote: > > > On 2017/04/26 15:24:36, sky wrote: > > > > It seems that all this code is chromeos specific. Can it move to a > chromeos > > > > specific location? > > > > > > These are used in content/browser and ui/views. They do not depend on > > > ui/chromeos/ and want to keep them that way. > > > > > > How about keeping interface here and inject using delegate? (the impl > should > > > probably be in ash as its a part of wm) > > > > I'm ok with them living here, but how about making file ime_util_chromeos and > > only compile on chromeos? > > Also, please add test coverage of the new function. I changed to compile ime_util_chromeos only for chromeos and added the unittests. Please take another look.
https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.cc (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:17: // Returns whether the window is actually moved or not. 'is actually' -> 'was' https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:18: // This function has to be in ifdef to prevent unused function error. Remove this line. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:27: // Calculate vertial window shift vertial -> vertical Also, end sentences with punctuation. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:33: std::max(rect_in_root_window.y() - bounds_in_root_window.height(), 0); Why do you use bounds_in_root_window.height() here? I would expect bottom(). https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:35: // No need to move up the window No need to move the window up. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.h (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.h:5: #ifndef UI_WM_CORE_IME_UTIL_CHROMEOS_H_ Thanks for renaming the files. One more comment though. I think these files would better fit in ui/base/ime. I believe that can be used from content as well, right? https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos_unittest.cc (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos_unittest.cc:36: }; DISALLOW...
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.cc (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:17: // Returns whether the window is actually moved or not. On 2017/04/28 20:17:51, sky wrote: > 'is actually' -> 'was' Done. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:18: // This function has to be in ifdef to prevent unused function error. On 2017/04/28 20:17:51, sky wrote: > Remove this line. Done. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:27: // Calculate vertial window shift On 2017/04/28 20:17:51, sky wrote: > vertial -> vertical > Also, end sentences with punctuation. Done. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:33: std::max(rect_in_root_window.y() - bounds_in_root_window.height(), 0); On 2017/04/28 20:17:51, sky wrote: > Why do you use bounds_in_root_window.height() here? I would expect bottom(). (rect_in_root_window.y() - bounds_in_root_window.height()) is enough not to cover the window by the rectangle. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:35: // No need to move up the window On 2017/04/28 20:17:51, sky wrote: > No need to move the window up. Done. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.h (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.h:5: #ifndef UI_WM_CORE_IME_UTIL_CHROMEOS_H_ On 2017/04/28 20:17:51, sky wrote: > Thanks for renaming the files. One more comment though. I think these files > would better fit in ui/base/ime. I believe that can be used from content as > well, right? These functions use aura::Window and ui/aura depends on ui/base/ime, so they cannot be moved to ui/base/ime. https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos_unittest.cc (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos_unittest.cc:36: }; On 2017/04/28 20:17:51, sky wrote: > DISALLOW... Done.
Right you are about this code not being able to live in ui/base/ime. LGTM https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.cc (right): https://codereview.chromium.org/2553603002/diff/360001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:33: std::max(rect_in_root_window.y() - bounds_in_root_window.height(), 0); On 2017/05/01 09:12:45, yhanada wrote: > On 2017/04/28 20:17:51, sky wrote: > > Why do you use bounds_in_root_window.height() here? I would expect bottom(). > > (rect_in_root_window.y() - bounds_in_root_window.height()) is enough not to > cover the window by the rectangle. Acknowledged.
https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1430: rect_in_screen, top_level_window->GetBoundsInScreen()); On 2017/04/26 18:03:42, oshima wrote: > don't you have to check if this is empty, and return early? ping?
https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1430: rect_in_screen, top_level_window->GetBoundsInScreen()); On 2017/05/01 21:57:18, oshima wrote: > On 2017/04/26 18:03:42, oshima wrote: > > don't you have to check if this is empty, and return early? > > ping? Sorry, I missed the comments. I added early return. https://codereview.chromium.org/2553603002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1433: wm::ConvertRectFromScreen(window_, &visible_area_in_local_space); On 2017/04/26 18:03:42, oshima wrote: > visible area can still be empty in non ash environment, right? Yes.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@: Could you review the changes in content/browser/renderer_host as the OWNER?
https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:338: virtual aura::Window* GetToplevelWindow(); I don't think you need this. See comment in the FakeRenderWidgetHostViewAura. https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:506: aura::Window* GetToplevelWindow() override { return window(); } Do you need this override? Because Window::GetToplevelWindow() should work for this too. https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.cc (right): https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:63: if (vk_restore_bounds) { Early return instead https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:98: RestoreWindowBoundsOnClientFocusLost(window); Do you need the restore here? https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.h (right): https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.h:26: // Restores the window bounds when input client loses the focus on the window . at the end
+ comment nit https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.h (right): https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.h:22: // Moves the window to ensure it not it the rect if needed. Moves the window, if needed, to ensure it does not intersect with |rect_in_screen|.
https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:338: virtual aura::Window* GetToplevelWindow(); On 2017/05/08 15:47:26, sadrul wrote: > I don't think you need this. See comment in the FakeRenderWidgetHostViewAura. It was removed. https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2553603002/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:506: aura::Window* GetToplevelWindow() override { return window(); } On 2017/05/08 15:47:26, sadrul wrote: > Do you need this override? Because Window::GetToplevelWindow() should work for > this too. I removed this override. It was needed because I initialized the view wrong in the added test. https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.cc (right): https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:63: if (vk_restore_bounds) { On 2017/05/08 15:47:26, sadrul wrote: > Early return instead Done. https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.cc:98: RestoreWindowBoundsOnClientFocusLost(window); On 2017/05/08 15:47:26, sadrul wrote: > Do you need the restore here? No. I didn't realize that. Thank you for pointing it out. https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... File ui/wm/core/ime_util_chromeos.h (right): https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.h:22: // Moves the window to ensure it not it the rect if needed. On 2017/05/08 15:48:48, sadrul wrote: > Moves the window, if needed, to ensure it does not intersect with > |rect_in_screen|. Done. https://codereview.chromium.org/2553603002/diff/420001/ui/wm/core/ime_util_ch... ui/wm/core/ime_util_chromeos.h:26: // Restores the window bounds when input client loses the focus on the window On 2017/05/08 15:47:26, sadrul wrote: > . at the end Done.
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yhanada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yhanada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, sky@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2553603002/#ps460001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you all for reviewing!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by yhanada@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yhanada@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 460001, "attempt_start_ts": 1494398528199090,
"parent_rev": "e120c2fd1e0c07b721caddef640aabf835d75840", "commit_rev":
"268490faabdd39308b8f6955408172f29e590bdd"}
Message was sent while issue was closed.
Description was changed from
==========
New accessibility virtual keyboard behavior in non-sticky mode.
Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The
kUseNewVirtualKeyboardBehavior switch must be enabled for this
behavior.
BUG=624521
TEST=Unit tests are added in render_widget_host_view_aura_unittest.cc
and textfield_unittest.cc.
==========
to
==========
New accessibility virtual keyboard behavior in non-sticky mode.
Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The
kUseNewVirtualKeyboardBehavior switch must be enabled for this
behavior.
BUG=624521
TEST=Unit tests are added in render_widget_host_view_aura_unittest.cc
and textfield_unittest.cc.
Review-Url: https://codereview.chromium.org/2553603002
Cr-Commit-Position: refs/heads/master@{#470517}
Committed:
https://chromium.googlesource.com/chromium/src/+/268490faabdd39308b8f69554081...
==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://chromium.googlesource.com/chromium/src/+/268490faabdd39308b8f69554081... |
