Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986153005:
The on screen keyboard on Windows 8+ should not obscure the input field. (Closed)
|
Created:
4 years, 7 months ago by ananta Modified:
4 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, grt+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe on screen keyboard on Windows 8+ should not obscure the input field.
The behavior we want is for the input field to scroll up when it
is obscured by the keyboard.
We attempt to mimic the behavior in Chrome OS where the viewport
is changed to the bounds of the OSK and the input field is
scrolled into the intersecting rectangle.
Added the following classes which live in ui\base\win\osk_display_manager.h/.cc
1. OnScreenKeyboardDisplayManager
2. OnScreenKeyboardDetector
These classes provide functionality to detect whether the
keyboard was displayed or hidden via a posted delayed task
They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can
detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives
in render_widget_host_view_aura.cc
BUG=608579
TEST=Partially covered by ui_base_unittest
OnScreenKeyboardTest.OSKPath
Committed: https://crrev.com/4297d7d9e0cd33ac6673ec6a839b1501baabeba8
Cr-Commit-Position: refs/heads/master@{#395754}
Patch Set 1 #Patch Set 2 : Finish up the implementation of the OnScreenKeyboardDisplayManager and OnScreenKeyboardDetector cla… #Patch Set 3 : Remove include #
Total comments: 71
Patch Set 4 : Address review comments #Patch Set 5 : Fix clang builder #Patch Set 6 : Remove include #Patch Set 7 : Fix presubmit #Patch Set 8 : Fix winclang build error #Patch Set 9 : Fix more winclang build errors #
Total comments: 22
Patch Set 10 : Address review comments #Patch Set 11 : git cl format #Patch Set 12 : Update comment #
Total comments: 4
Patch Set 13 : Address review comments and update comment #
Total comments: 12
Patch Set 14 : Address sky review comments #Patch Set 15 : git cl format #
Total comments: 19
Patch Set 16 : Address review comments #
Total comments: 17
Patch Set 17 : Move constants into a namespace and address some comments from sky #Patch Set 18 : Address more sky comments #
Total comments: 4
Patch Set 19 : Set the bounds of the viewport correctly #
Total comments: 2
Patch Set 20 : Add a check for viewport bottom bigger than view height #
Total comments: 6
Patch Set 21 : Address review comments #Patch Set 22 : Remove tabs #
Total comments: 10
Patch Set 23 : Address grt review comments and add unittest #
Total comments: 6
Patch Set 24 : Address sky unittest review comments #Depends on Patchset: Messages
Total messages: 52 (12 generated)
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. BUG=608579 ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. BUG=608579 ==========
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. BUG=608579 ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_view_host_impl.cc BUG=608579 ==========
ananta@chromium.org changed reviewers: + sky@chromium.org
grt@chromium.org changed reviewers: + grt@chromium.org
cool stuff. drive-by of base/win. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. (c) https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:40: ClearObservers(); is this needed? can you just let the list self-destruct without emptying it (and set the check_empty template param to false in the .h)? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:81: if (!osk_visible_notification_received_) { nit: omit braces https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:88: // Two cases here. looks like the code handles three cases. could you document the PostDelayedTask case here, too? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:146: return base::Singleton<OnScreenKeyboardDisplayManager, is a leaky lazy instance appropriate here? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:159: static LazyInstance<string16>::Leaky osk_path = LAZY_INSTANCE_INITIALIZER; OnScreenKeyboardDisplayManager is a singleton. why does the path need to be a local lazy instance rather than an instance member of the class? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:174: WriteInto(&osk_path.Get(), osk_path_length), note that WriteInto doesn't update the size of the string, so osk_path.size() will return 1024 at this point, and operations will look at the full size of the buffer. i propose trimming the string with something like osk_path.resize(base::string16::traits_type::length(osk_path.c_str())); https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:181: osk_path.Get().find(L"%CommonProgramFiles%"); case-insensitive? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:203: GetEnvironmentVariable(L"CommonProgramW6432", NULL, 0); nullptr https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:207: common_program_files_wow6432.get(), WriteInto(&common_program_files_path, buffer_size)? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:214: if (FAILED(SHGetKnownFolderPath(FOLDERID_ProgramFilesCommon, 0, NULL, nullptr https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:221: osk_path.Get().insert(1, common_program_files_path); 1 -> common_program_files_offset? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:225: HINSTANCE ret = ::ShellExecuteW(NULL, nullptr all the things ;-) https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:234: keyboard_detector_.reset(new OnScreenKeyboardDetector); since this is a singleton, two callers to DisplayVirtualKeyboard will stomp on one another. is this what you really want? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no "(c)" in new header (http://www.chromium.org/developers/coding-style#TOC-File-headers) https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:4: #ifndef BASE_WIN_OSK_DISPLAY_MANAGER_H_ nit: blank line before this https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:30: // Runs a task which detects if the on screen keyboard was displayed. nit: "Schedules a delayed task that will..." since it doesn't actually run the task. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:47: void CheckOSKState(bool check_for_activation); does this detection run periodically for the entire duration that the OSK is visible to detect when it is hidden? if so, please document it as such. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:49: // Notifies observers that the keyboard was displayed. please also note that a recurring task is started to detect when the OSK disappears. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:59: HWND main_window_; = nullptr; https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:62: bool osk_visible_notification_received_; = false; https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:77: // Windows 8+. It ensures that the content on the page is scrolled if the how does this do the scrolling? is that the responsibility of the observer? https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} nit: = default;
sky@chromium.org changed reviewers: + jam@chromium.org, nick@chromium.org
I'm not an owner of any of this code. sky->nick
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:19: virtual void OnKeyboardVisible(const RECT& keyboard_rect) {} RECT->gfx::Rect for these too. Also, you should make it clear what coordinates these are. I think it's screen pixels, in which case you might want to convert to screen dip. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:25: class BASE_EXPORT OnScreenKeyboardDetector { Is there a reason you need this in base? I would be inclined to put it in ui/base. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:190: ::GetCursorPos(&cursor_pos); Is the cursor position always updated for touch? None-the-less it would be better if you could make OnFocusedNodeTouched take the location so you don't have to look it up. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:200: gfx::Insets(0, 0, display_rect.height(), 0)); Shouldn't this depend upon the region of the renderer view that intersects the keyboard? https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:214: widget_host->GetView()->SetInsets(gfx::Insets()); Should this be done in the constructor too? https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:220: }; DISALLOW... https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1360: keyboard_observer_.reset(new OnScreenKeyboardObserver(this)); Is it possible the keyboard is already visible at the time you create OnScreenKeyboardObserver? I'm wondering if it's constructor needs to check visibility and do what OnKeyboardVisible does?
https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:172: #if defined(OS_WIN) This is getting to be more OS_WIN code in this file than seems tolerable. I wonder if the guts of on-screen keyboard handling would make more sense living in RenderWidgetHostViewAura? RenderWidgetHostView is supposed to be our platform abstraction here, and this is a platform behavior. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:178: OnScreenKeyboardObserver(RenderViewHostImpl* host) explicit https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1367: osk_display_manager->DismissVirtualKeyboard(keyboard_observer_.get()); It's not clear to me why this DismissVirtualKeyboard event needs to pass keyboard_observer_, but the void DismissVirtualKeyboardTask() {} function doesn't. Is that inconsistent?
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:40: ClearObservers(); On 2016/05/18 15:41:19, grt (slow) wrote: > is this needed? can you just let the list self-destruct without emptying it (and > set the check_empty template param to false in the .h)? Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:81: if (!osk_visible_notification_received_) { On 2016/05/18 15:41:19, grt (slow) wrote: > nit: omit braces Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:88: // Two cases here. On 2016/05/18 15:41:19, grt (slow) wrote: > looks like the code handles three cases. could you document the PostDelayedTask > case here, too? Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:146: return base::Singleton<OnScreenKeyboardDisplayManager, On 2016/05/18 15:41:19, grt (slow) wrote: > is a leaky lazy instance appropriate here? That would work as well. Changed. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:159: static LazyInstance<string16>::Leaky osk_path = LAZY_INSTANCE_INITIALIZER; On 2016/05/18 15:41:19, grt (slow) wrote: > OnScreenKeyboardDisplayManager is a singleton. why does the path need to be a > local lazy instance rather than an instance member of the class? Yes. Moved this function into a class without inspecting the code fully. Thanks for finding this. Changed. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:174: WriteInto(&osk_path.Get(), osk_path_length), On 2016/05/18 15:41:19, grt (slow) wrote: > note that WriteInto doesn't update the size of the string, so osk_path.size() > will return 1024 at this point, and operations will look at the full size of the > buffer. i propose trimming the string with something like > osk_path.resize(base::string16::traits_type::length(osk_path.c_str())); Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:203: GetEnvironmentVariable(L"CommonProgramW6432", NULL, 0); On 2016/05/18 15:41:19, grt (slow) wrote: > nullptr Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:207: common_program_files_wow6432.get(), On 2016/05/18 15:41:20, grt (slow) wrote: > WriteInto(&common_program_files_path, buffer_size)? Thanks. done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:214: if (FAILED(SHGetKnownFolderPath(FOLDERID_ProgramFilesCommon, 0, NULL, On 2016/05/18 15:41:19, grt (slow) wrote: > nullptr Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:221: osk_path.Get().insert(1, common_program_files_path); On 2016/05/18 15:41:19, grt (slow) wrote: > 1 -> common_program_files_offset? Yes. done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:225: HINSTANCE ret = ::ShellExecuteW(NULL, On 2016/05/18 15:41:19, grt (slow) wrote: > nullptr all the things ;-) Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.cc:234: keyboard_detector_.reset(new OnScreenKeyboardDetector); On 2016/05/18 15:41:20, grt (slow) wrote: > since this is a singleton, two callers to DisplayVirtualKeyboard will stomp on > one another. is this what you really want? It does not matter in this use case as the callers are on the UI thread and hence will be serialized. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/18 15:41:20, grt (slow) wrote: > nit: no "(c)" in new header > (http://www.chromium.org/developers/coding-style#TOC-File-headers) Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:4: #ifndef BASE_WIN_OSK_DISPLAY_MANAGER_H_ On 2016/05/18 15:41:20, grt (slow) wrote: > nit: blank line before this Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:19: virtual void OnKeyboardVisible(const RECT& keyboard_rect) {} On 2016/05/18 19:54:51, sky wrote: > RECT->gfx::Rect for these too. Also, you should make it clear what coordinates > these are. I think it's screen pixels, in which case you might want to convert > to screen dip. Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:25: class BASE_EXPORT OnScreenKeyboardDetector { On 2016/05/18 19:54:51, sky wrote: > Is there a reason you need this in base? I would be inclined to put it in > ui/base. Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:30: // Runs a task which detects if the on screen keyboard was displayed. On 2016/05/18 15:41:20, grt (slow) wrote: > nit: "Schedules a delayed task that will..." since it doesn't actually run the > task. Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:47: void CheckOSKState(bool check_for_activation); On 2016/05/18 15:41:20, grt (slow) wrote: > does this detection run periodically for the entire duration that the OSK is > visible to detect when it is hidden? if so, please document it as such. Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:49: // Notifies observers that the keyboard was displayed. On 2016/05/18 15:41:20, grt (slow) wrote: > please also note that a recurring task is started to detect when the OSK > disappears. Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:59: HWND main_window_; On 2016/05/18 15:41:20, grt (slow) wrote: > = nullptr; This is initialized in the ctor. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:62: bool osk_visible_notification_received_; On 2016/05/18 15:41:20, grt (slow) wrote: > = false; ditto https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:77: // Windows 8+. It ensures that the content on the page is scrolled if the On 2016/05/18 15:41:20, grt (slow) wrote: > how does this do the scrolling? is that the responsibility of the observer? Thanks it does not. I started out with a impl which did the scrolling and then moved that to the observers. The comment was stale. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/18 15:41:20, grt (slow) wrote: > nit: = default; Please clarify what you mean here. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:172: #if defined(OS_WIN) On 2016/05/18 20:09:11, ncarter wrote: > This is getting to be more OS_WIN code in this file than seems tolerable. > > I wonder if the guts of on-screen keyboard handling would make more sense living > in RenderWidgetHostViewAura? RenderWidgetHostView is supposed to be our platform > abstraction here, and this is a platform behavior. Moved this RenderWidgetHostViewAura. This involved adding a virtual method FocusedNodeTouched() to the base RenderWidgetHostView class and so on. The FocusedNodeTouched() and FocusedNodeChanged() methods are overridden by RWHVA. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:178: OnScreenKeyboardObserver(RenderViewHostImpl* host) On 2016/05/18 20:09:11, ncarter wrote: > explicit Done. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:214: widget_host->GetView()->SetInsets(gfx::Insets()); On 2016/05/18 19:54:52, sky wrote: > Should this be done in the constructor too? Done. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:220: }; On 2016/05/18 19:54:51, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1360: keyboard_observer_.reset(new OnScreenKeyboardObserver(this)); On 2016/05/18 19:54:51, sky wrote: > Is it possible the keyboard is already visible at the time you create > OnScreenKeyboardObserver? I'm wondering if it's constructor needs to check > visibility and do what OnKeyboardVisible does? I don't think that would be needed. The DisplayVirtualKeyboard call below would eventually run the task which would notify our observer about the keyboard becoming visible. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:1367: osk_display_manager->DismissVirtualKeyboard(keyboard_observer_.get()); On 2016/05/18 20:09:11, ncarter wrote: > It's not clear to me why this DismissVirtualKeyboard event needs to pass > keyboard_observer_, but the void DismissVirtualKeyboardTask() {} function > doesn't. Is that inconsistent? Yes. Removed the observer parameter.
https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:190: ::GetCursorPos(&cursor_pos); On 2016/05/18 19:54:51, sky wrote: > Is the cursor position always updated for touch? None-the-less it would be > better if you could make OnFocusedNodeTouched take the location so you don't > have to look it up. It is updated for touch. I pass the cursor position to the newly added FocusedNodeTouched function in RenderWidgetHostView. https://codereview.chromium.org/1986153005/diff/40001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:200: gfx::Insets(0, 0, display_rect.height(), 0)); On 2016/05/18 19:54:52, sky wrote: > Shouldn't this depend upon the region of the renderer view that intersects the > keyboard? Yeah. Updated. PTAL
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_view_host_impl.cc BUG=608579 ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_view_host_impl.cc BUG=608579 ==========
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_view_host_impl.cc BUG=608579 ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 ==========
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:59: HWND main_window_; On 2016/05/19 01:57:12, ananta wrote: > On 2016/05/18 15:41:20, grt (slow) wrote: > > = nullptr; > > This is initialized in the ctor. with the new C++-11 style, this sort of initialization should be in the class; see "Non-Static Class Member Initializers" in http://chromium-cpp.appspot.com/#core-whitelist https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/19 01:57:12, ananta wrote: > On 2016/05/18 15:41:20, grt (slow) wrote: > > nit: = default; > > Please clarify what you mean here. Gangam style! uh, i mean C++-11 style: OnScreenKeyboardDisplayManager() = default;
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/19 03:51:02, grt (slow) wrote: > On 2016/05/19 01:57:12, ananta wrote: > > On 2016/05/18 15:41:20, grt (slow) wrote: > > > nit: = default; > > > > Please clarify what you mean here. > > Gangam style! uh, i mean C++-11 style: > OnScreenKeyboardDisplayManager() = default; I'm curious here as well as it has come up in other reviews. What benefit do we get by using '= default' of {} here? https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:19: class UI_BASE_EXPORT OnScreenKeyboardObserver { nit: move to own header. https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:90: class UI_BASE_EXPORT OnScreenKeyboardDisplayManager { Why do we need two classes for managing keyboards? I get that the keyboard is different in win8, but that's an implementation detail. Can we have one public class here, and if that needs to call off to differently implementations, fine. But consumers only care about the single class.
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/19 16:05:25, sky wrote: > On 2016/05/19 03:51:02, grt (slow) wrote: > > On 2016/05/19 01:57:12, ananta wrote: > > > On 2016/05/18 15:41:20, grt (slow) wrote: > > > > nit: = default; > > > > > > Please clarify what you mean here. > > > > Gangam style! uh, i mean C++-11 style: > > OnScreenKeyboardDisplayManager() = default; > > I'm curious here as well as it has come up in other reviews. What benefit do we > get by using '= default' of {} here? My reason for suggesting it is so that there's just one way we do things in the code. I don't know if there's a compelling reason such as better codegen from the compiler or something. Perhaps Dana or one of the other C++-11 gurus would know. I don't feel strongly either way.
Thanks for the RWHV rework, I like how it came out. content is looking good for the most part, but I'm happy to do more passes if the api evolves. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:222: static int virtual_keyboard_display_retries = 0; I believe was preexisting behavior, but isn't it possible to enter a state where two tabs both have a DismissVirtualKeyboardTask posted? If that happens can it frustrate the user somehow? If there's a real bug here it's fine addressing it in a follow-up patch -- just pointing out that this use of static for the counter seems a little strange. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:968: ui::OnScreenKeyboardDisplayManager::GetInstance(); Indent += 2 https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2132: base::Bind(base::IgnoreResult(&DismissVirtualKeyboardTask)), IgnoreResult shouldn't be necessary -- this function returns void. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2133: base::TimeDelta::FromMilliseconds(kVirtualKeyboardDisplayWaitTimeoutMs)); This indentation looks off (git cl format?) https://codereview.chromium.org/1986153005/diff/160001/content/public/browser... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/1986153005/diff/160001/content/public/browser... content/public/browser/render_widget_host_view.h:165: // The |location| parameter contains the location where the touch occurred |location_dips| https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:247: keyboard_detector_.reset(new OnScreenKeyboardDetector); Is this the right behavior, if keyboard_detector_ already was allocated? Seems like that would lose the existing observers, if any? https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:259: DCHECK(keyboard_detector_.get()); It seems like we'll crash if DisplayVirtualKeyboard is never called before Dismiss happens, but it seems plausible that one might want to call Dismiss unconditionally, say, in some cleanup path, or on a fullscreen transition, or something like that? https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:273: GetInstance()->DismissVirtualKeyboard(); // !keyboard_observer. This comment seems out of date.
https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:255: DVLOG(1) << "FocusedNodeTouched: " << editable; I wonder if this will be reachable with oopifs? You can try the following test: run content_shell (or chrome) with the --site-per-process flag, and visit http://csreis.github.io/tests/cross-site-iframe-simple.html. The input elements there are in a different process from the main page. You may have to add a FocusedNodeTouched to RenderWidgetHostViewChildFrame, to forward it upwards -- kenrb or lfg can help you figure out the right way to structure that. Oopifs will start shipping for extensions in the next milestone, so it's important to get this right.
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:59: HWND main_window_; On 2016/05/19 03:51:03, grt (slow) wrote: > On 2016/05/19 01:57:12, ananta wrote: > > On 2016/05/18 15:41:20, grt (slow) wrote: > > > = nullptr; > > > > This is initialized in the ctor. > > with the new C++-11 style, this sort of initialization should be in the class; > see "Non-Static Class Member Initializers" in > http://chromium-cpp.appspot.com/#core-whitelist Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:62: bool osk_visible_notification_received_; On 2016/05/18 15:41:20, grt (slow) wrote: > = false; Done. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_ma... base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/19 16:53:17, grt (slow) wrote: > On 2016/05/19 16:05:25, sky wrote: > > On 2016/05/19 03:51:02, grt (slow) wrote: > > > On 2016/05/19 01:57:12, ananta wrote: > > > > On 2016/05/18 15:41:20, grt (slow) wrote: > > > > > nit: = default; > > > > > > > > Please clarify what you mean here. > > > > > > Gangam style! uh, i mean C++-11 style: > > > OnScreenKeyboardDisplayManager() = default; > > > > I'm curious here as well as it has come up in other reviews. What benefit do > we > > get by using '= default' of {} here? > > My reason for suggesting it is so that there's just one way we do things in the > code. I don't know if there's a compelling reason such as better codegen from > the compiler or something. Perhaps Dana or one of the other C++-11 gurus would > know. I don't feel strongly either way. I left it as is as clang needs the destructor to be in the cc file. It seems better to have the ctor and dtor together. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:222: static int virtual_keyboard_display_retries = 0; On 2016/05/19 17:45:55, ncarter wrote: > I believe was preexisting behavior, but isn't it possible to enter a state where > two tabs both have a DismissVirtualKeyboardTask posted? If that happens can it > frustrate the user somehow? > > If there's a real bug here it's fine addressing it in a follow-up patch -- just > pointing out that this use of static for the counter seems a little strange. Thanks for identifying this issue. Added a comment in the code and a TODO to fix it. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:968: ui::OnScreenKeyboardDisplayManager::GetInstance(); On 2016/05/19 17:45:55, ncarter wrote: > Indent += 2 Done. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2132: base::Bind(base::IgnoreResult(&DismissVirtualKeyboardTask)), On 2016/05/19 17:45:55, ncarter wrote: > IgnoreResult shouldn't be necessary -- this function returns void. Done. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2133: base::TimeDelta::FromMilliseconds(kVirtualKeyboardDisplayWaitTimeoutMs)); On 2016/05/19 17:45:54, ncarter wrote: > This indentation looks off (git cl format?) Done. https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:255: DVLOG(1) << "FocusedNodeTouched: " << editable; On 2016/05/19 18:03:24, ncarter wrote: > I wonder if this will be reachable with oopifs? > > You can try the following test: run content_shell (or chrome) with the > --site-per-process flag, and visit > http://csreis.github.io/tests/cross-site-iframe-simple.html. The input elements > there are in a different process from the main page. You may have to add a > FocusedNodeTouched to RenderWidgetHostViewChildFrame, to forward it upwards -- > kenrb or lfg can help you figure out the right way to structure that. > > Oopifs will start shipping for extensions in the next milestone, so it's > important to get this right. Will look into this https://codereview.chromium.org/1986153005/diff/160001/content/public/browser... File content/public/browser/render_widget_host_view.h (right): https://codereview.chromium.org/1986153005/diff/160001/content/public/browser... content/public/browser/render_widget_host_view.h:165: // The |location| parameter contains the location where the touch occurred On 2016/05/19 17:45:55, ncarter wrote: > |location_dips| Done. https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:247: keyboard_detector_.reset(new OnScreenKeyboardDetector); On 2016/05/19 17:45:55, ncarter wrote: > Is this the right behavior, if keyboard_detector_ already was allocated? Seems > like that would lose the existing observers, if any? In the ideal case we want a list of detectors. However the use case which triggers this is via a touch. So if we have multiple touches occurring one after the other the last one would win. I added a comment here. I don't think this is a huge concern at the moment given that they all would happen on the same thread. https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:273: GetInstance()->DismissVirtualKeyboard(); // !keyboard_observer. On 2016/05/19 17:45:55, ncarter wrote: > This comment seems out of date. Done. https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:19: class UI_BASE_EXPORT OnScreenKeyboardObserver { On 2016/05/19 16:05:26, sky wrote: > nit: move to own header. Done. https://codereview.chromium.org/1986153005/diff/160001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:90: class UI_BASE_EXPORT OnScreenKeyboardDisplayManager { On 2016/05/19 16:05:26, sky wrote: > Why do we need two classes for managing keyboards? I get that the keyboard is > different in win8, but that's an implementation detail. > Can we have one public class here, and if that needs to call off to differently > implementations, fine. But consumers only care about the single class. Done.
content lgtm https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1271: // Pass this information from blink. Getting the info from blink probably makes sense (assuming we can trust it). Fwiw, it might also be possible to grab it from the InputRouter (kind of like GetLastKeyboardEvent()) but I don't know which is a better design. https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:236: base::Bind(base::IgnoreResult(&DismissVirtualKeyboardTask)), nit: can drop the IgnoreResult here too.
https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.cc:255: DVLOG(1) << "FocusedNodeTouched: " << editable; On 2016/05/19 19:33:38, ananta wrote: > On 2016/05/19 18:03:24, ncarter wrote: > > I wonder if this will be reachable with oopifs? > > > > You can try the following test: run content_shell (or chrome) with the > > --site-per-process flag, and visit > > http://csreis.github.io/tests/cross-site-iframe-simple.html. The input > elements > > there are in a different process from the main page. You may have to add a > > FocusedNodeTouched to RenderWidgetHostViewChildFrame, to forward it upwards -- > > kenrb or lfg can help you figure out the right way to structure that. > > > > Oopifs will start shipping for extensions in the next milestone, so it's > > important to get this right. > > Will look into this I logged a bug https://bugs.chromium.org/p/chromium/issues/detail?id=613326 and referenced this in a TODO in RenderViewHostImpl::OnFocusedNodeTouched. It may make sense to move the handling of these messages to RenderWidgetHostImpl and call the view from there. We would still need a place to add in the implementation on Windows to display and dismiss the on screen keyboard. https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:1271: // Pass this information from blink. On 2016/05/19 21:46:15, ncarter wrote: > Getting the info from blink probably makes sense (assuming we can trust it). > Fwiw, it might also be possible to grab it from the InputRouter (kind of like > GetLastKeyboardEvent()) but I don't know which is a better design. Yes. We can revisit this in the near future. https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:236: base::Bind(base::IgnoreResult(&DismissVirtualKeyboardTask)), On 2016/05/19 21:46:16, ncarter wrote: > nit: can drop the IgnoreResult here too. Done.
https://codereview.chromium.org/1986153005/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:228: void DismissVirtualKeyboardTask() { I think this should be handled in the display manager, not here. By that I mean clients should be able to call dismiss and assume it took. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:101: base::MessageLoop::current()->PostDelayedTask( Please document why we need to delay here. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:137: gfx::ConvertRectToDIP(display::win::GetDPIScale(), gfx::Rect(osk_rect)); Use the functions in ScreenWin that take the hwnd into account. But it looks like you need pixels in the one place using this, so don't convert, keep it pixels and clearly indicate in the observer it's pixels. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:239: return false; If we return any where in here should we cache the value so we don't keep trying? https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:19: class UI_BASE_EXPORT OnScreenKeyboardObserver { Move to own header. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:62: UI_BASE_EXPORT bool DisplayVirtualKeyboard(); Can the single place that calls this be changed to: OnScreenKeyboardDisplayManager::GetInstance()->Display...(nullptr)? Similar comment about Dismiss. Alternatively don't make OnScreenKeyboardDisplayManager and have the display/dismiss functions only here.
https://codereview.chromium.org/1986153005/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:228: void DismissVirtualKeyboardTask() { On 2016/05/19 22:25:08, sky wrote: > I think this should be handled in the display manager, not here. By that I mean > clients should be able to call dismiss and assume it took. Done. Handled in the DismissKeyboard function. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:101: base::MessageLoop::current()->PostDelayedTask( On 2016/05/19 22:25:08, sky wrote: > Please document why we need to delay here. Done. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:137: gfx::ConvertRectToDIP(display::win::GetDPIScale(), gfx::Rect(osk_rect)); On 2016/05/19 22:25:08, sky wrote: > Use the functions in ScreenWin that take the hwnd into account. But it looks > like you need pixels in the one place using this, so don't convert, keep it > pixels and clearly indicate in the observer it's pixels. Done. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:239: return false; On 2016/05/19 22:25:08, sky wrote: > If we return any where in here should we cache the value so we don't keep > trying? We could. However this is old code and mostly safe. We can leave this as is. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:19: class UI_BASE_EXPORT OnScreenKeyboardObserver { On 2016/05/19 22:25:08, sky wrote: > Move to own header. Done. Was a stale declaration. https://codereview.chromium.org/1986153005/diff/240001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:62: UI_BASE_EXPORT bool DisplayVirtualKeyboard(); On 2016/05/19 22:25:08, sky wrote: > Can the single place that calls this be changed to: > OnScreenKeyboardDisplayManager::GetInstance()->Display...(nullptr)? > Similar comment about Dismiss. > > Alternatively don't make OnScreenKeyboardDisplayManager and have the > display/dismiss functions only here. Done.
https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:226: explicit WinScreenKeyboardObserver(RenderWidgetHostImpl* host, nit: omit "explicit" as per https://google.github.io/styleguide/cppguide.html#Implicit_Conversions: "Constructors that cannot be called with a single argument should usually omit explicit." https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:240: DCHECK(host_); it's counter to style to both DCHECK that a ptr is not null and check for nullness at the point of use (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ti040zXE_A8/JXfq_...). can the host ever really be null? if not, remove the check below. otherwise, remove the DCHECK. https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:959: osk_display_manager->DisplayVirtualKeyboard(keyboard_observer_.get()); looks like this RWHVA could be destroyed while the manager is doing its work, which would lead the manager holding onto a dangling observer pointer. should the dtor remove the observer? https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; nit: use constexpr for compile-time constants (https://google.github.io/styleguide/cppguide.html#Use_of_constexpr) https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; put these privates in an unnamed namespace and omit "static" https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:241: return base::Singleton< since the only access of the singleton is from within this one function and you want to leak the instance on exit, base::Singleton is overkill. if all callers will be on the same thread (i.e., the UI thread), then you are safe with: ...::GetInstance() { static OnScreenKeyboardDisplayManager* instance = nullptr; if (!instance) { instance = new OnScreenKeyboardDisplayManager; ANNOTATE_LEAKING_OBJECT_PTR(instance); } return instance; } otherwise, if callers may be on multiple threads, you can go with: ...::GetInstance() { static LazyInstance<OnScreenKeyboardDisplayManager> instance = LAZY_INSTANCE_INITIALIZER; return instance.Pointer(); } https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:337: bool ret = false; nit: return keyboard_detector_ ? keyboard_detector_->DismissKeyboard() : false; https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:32: // hidden etc. it looks like the observer is automagically removed once the keyboard is dismissed. please document this.
https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:226: explicit WinScreenKeyboardObserver(RenderWidgetHostImpl* host, On 2016/05/20 14:56:35, grt (slow) wrote: > nit: omit "explicit" as per > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions: > "Constructors that cannot be called with a single argument should usually omit > explicit." Done. https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:240: DCHECK(host_); On 2016/05/20 14:56:35, grt (slow) wrote: > it's counter to style to both DCHECK that a ptr is not null and check for > nullness at the point of use > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ti040zXE_A8/JXfq_...). > can the host ever really be null? if not, remove the check below. otherwise, > remove the DCHECK. Removed the DCHECK https://codereview.chromium.org/1986153005/diff/280001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:959: osk_display_manager->DisplayVirtualKeyboard(keyboard_observer_.get()); On 2016/05/20 14:56:34, grt (slow) wrote: > looks like this RWHVA could be destroyed while the manager is doing its work, > which would lead the manager holding onto a dangling observer pointer. should > the dtor remove the observer? Done. https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:241: return base::Singleton< On 2016/05/20 14:56:35, grt (slow) wrote: > since the only access of the singleton is from within this one function and you > want to leak the instance on exit, base::Singleton is overkill. if all callers > will be on the same thread (i.e., the UI thread), then you are safe with: > ...::GetInstance() { > static OnScreenKeyboardDisplayManager* instance = nullptr; > if (!instance) { > instance = new OnScreenKeyboardDisplayManager; > ANNOTATE_LEAKING_OBJECT_PTR(instance); > } > return instance; > } > > otherwise, if callers may be on multiple threads, you can go with: > ...::GetInstance() { > static LazyInstance<OnScreenKeyboardDisplayManager> instance = > LAZY_INSTANCE_INITIALIZER; > return instance.Pointer(); > } Done. https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:337: bool ret = false; On 2016/05/20 14:56:35, grt (slow) wrote: > nit: > return keyboard_detector_ ? keyboard_detector_->DismissKeyboard() : false; Done. https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:32: // hidden etc. On 2016/05/20 14:56:35, grt (slow) wrote: > it looks like the observer is automagically removed once the keyboard is > dismissed. please document this. Done.
https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:227: const gfx::Point& location_dips, It looks like this is in screen coordinates too. Please name appropriately. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:234: if (host_ && host_->GetView()) How about only creating if host_ and host_->Getview()? That way you don't have all the checks in this class. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:241: // We use the PtInRect API to determine if the touch occurred in the Why? Why can't you use keyboard_rect_pixels.Contains(touch_location), or perhaps Intersects() and avoid the conversions. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:255: // The viewport needs to be moved up by the height of the intersection. I don't think this is quite right. I think you need to move up by keyboard_rect_pixels.y() - location_in_pixels.y() (probably plus some extra space as well), and you want to set the bottom inset to keyboard_rect_pixels.y(). https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:258: host_->GetView()->SetInsets(gfx::Insets(0, 0, intersect.height(), 0)); What happens if the height is > height of view? https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:112: main_window_ = main_window; With all the delayed tasks should DetectKeyboard and DismissKeyboard both invalidate weak ptrs on the keyboard_detector_factory_ to ensure you don't have a pending dismiss? For example, lets say you have a pending call to Dismiss as scheduled on line 140 and then Detect is called, I think the dismiss should be dropped. https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:161: void OnScreenKeyboardDetector::CheckOSKState(bool check_for_activation) { This function has two very different purposes that are hard to follow because you both in the function. I think it's worth splitting it up to improve clarity. You have: CheckIfKeyboardVisible and HideIfNecessary. Doing this means you can move HandleKeyboardVisible into CheckIfKeyboardVisible. https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... File ui/base/win/osk_display_observer.h (right): https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... ui/base/win/osk_display_observer.h:14: virtual ~OnScreenKeyboardObserver() {} nit: move to protected section.
https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 14:56:35, grt (slow) wrote: > put these privates in an unnamed namespace and omit "static" ping
https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 14:56:35, grt (no reviews until May 24) wrote: > put these privates in an unnamed namespace and omit "static" Done. https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 14:56:35, grt (no reviews until May 24) wrote: > nit: use constexpr for compile-time constants > (https://google.github.io/styleguide/cppguide.html#Use_of_constexpr) Done. https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 20:29:24, grt (no reviews until May 24) wrote: > On 2016/05/20 14:56:35, grt (slow) wrote: > > put these privates in an unnamed namespace and omit "static" > > ping Done. https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 14:56:35, grt (no reviews until May 24) wrote: > put these privates in an unnamed namespace and omit "static" Done. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:227: const gfx::Point& location_dips, On 2016/05/20 19:57:16, sky wrote: > It looks like this is in screen coordinates too. Please name appropriately. Done. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:234: if (host_ && host_->GetView()) On 2016/05/20 19:57:16, sky wrote: > How about only creating if host_ and host_->Getview()? That way you don't have > all the checks in this class. Done. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:241: // We use the PtInRect API to determine if the touch occurred in the On 2016/05/20 19:57:16, sky wrote: > Why? Why can't you use keyboard_rect_pixels.Contains(touch_location), or perhaps > Intersects() and avoid the conversions. Done. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:255: // The viewport needs to be moved up by the height of the intersection. On 2016/05/20 19:57:16, sky wrote: > I don't think this is quite right. I think you need to move up by > keyboard_rect_pixels.y() - location_in_pixels.y() (probably plus some extra > space as well), and you want to set the bottom inset to > keyboard_rect_pixels.y(). Will look into this. https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:112: main_window_ = main_window; On 2016/05/20 19:57:16, sky wrote: > With all the delayed tasks should DetectKeyboard and DismissKeyboard both > invalidate weak ptrs on the keyboard_detector_factory_ to ensure you don't have > a pending dismiss? For example, lets say you have a pending call to Dismiss as > scheduled on line 140 and then Detect is called, I think the dismiss should be > dropped. Currently a new instance of the OnScreenKeyboardDetector class is created for every call to DisplayVirtualKeyboard which means that pending tasks will be dropped. https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:161: void OnScreenKeyboardDetector::CheckOSKState(bool check_for_activation) { On 2016/05/20 19:57:16, sky wrote: > This function has two very different purposes that are hard to follow because > you both in the function. I think it's worth splitting it up to improve clarity. > You have: CheckIfKeyboardVisible and HideIfNecessary. Doing this means you can > move HandleKeyboardVisible into CheckIfKeyboardVisible. Done. https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... File ui/base/win/osk_display_observer.h (right): https://codereview.chromium.org/1986153005/diff/300001/ui/base/win/osk_displa... ui/base/win/osk_display_observer.h:14: virtual ~OnScreenKeyboardObserver() {} On 2016/05/20 19:57:16, sky wrote: > nit: move to protected section. Done.
https://codereview.chromium.org/1986153005/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:234: if (host_ && host_->GetView()) Remove check as it's done at call site. https://codereview.chromium.org/1986153005/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:262: if (host_ && host_->GetView()) remove check.
https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:255: // The viewport needs to be moved up by the height of the intersection. On 2016/05/20 20:54:17, ananta wrote: > On 2016/05/20 19:57:16, sky wrote: > > I don't think this is quite right. I think you need to move up by > > keyboard_rect_pixels.y() - location_in_pixels.y() (probably plus some extra > > space as well), and you want to set the bottom inset to > > keyboard_rect_pixels.y(). > > Will look into this. Done. Changed this as per our discussion. https://codereview.chromium.org/1986153005/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:258: host_->GetView()->SetInsets(gfx::Insets(0, 0, intersect.height(), 0)); On 2016/05/20 19:57:16, sky wrote: > What happens if the height is > height of view? The logic above has changed. https://codereview.chromium.org/1986153005/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:234: if (host_ && host_->GetView()) On 2016/05/20 22:14:55, sky wrote: > Remove check as it's done at call site. Done. https://codereview.chromium.org/1986153005/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:262: if (host_ && host_->GetView()) On 2016/05/20 22:14:55, sky wrote: > remove check. Done.
Only one question. https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:257: host_->GetView()->SetInsets(gfx::Insets(0, 0, viewport_bottom, 0)); What happens if viewport_bottom is > height of view?
https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:257: host_->GetView()->SetInsets(gfx::Insets(0, 0, viewport_bottom, 0)); On 2016/05/20 22:42:59, sky wrote: > What happens if viewport_bottom is > height of view? That would make the height of the view 0. https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/re... Maybe we can add a check here and not set the viewport in this case? with a comment to that effect.
It wouldn't make it 0, it would make the inset bigger than the height of the viewport. Seems like if the keyboard y position is above the view you shouldn't do anything. On Fri, May 20, 2016 at 3:52 PM, <ananta@chromium.org> wrote: > > https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_aura.cc > (right): > > https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:257: > host_->GetView()->SetInsets(gfx::Insets(0, 0, viewport_bottom, 0)); > On 2016/05/20 22:42:59, sky wrote: >> What happens if viewport_bottom is > height of view? > > That would make the height of the view 0. > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/re... > > Maybe we can add a check here and not set the viewport in this case? > with a comment to that effect. > > https://codereview.chromium.org/1986153005/ -- 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.
On 2016/05/20 22:58:22, sky wrote: > It wouldn't make it 0, it would make the inset bigger than the height > of the viewport. Seems like if the keyboard y position is above the > view you shouldn't do anything. > > On Fri, May 20, 2016 at 3:52 PM, <mailto:ananta@chromium.org> wrote: > > > > > https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... > > File content/browser/renderer_host/render_widget_host_view_aura.cc > > (right): > > > > > https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... > > content/browser/renderer_host/render_widget_host_view_aura.cc:257: > > host_->GetView()->SetInsets(gfx::Insets(0, 0, viewport_bottom, 0)); > > On 2016/05/20 22:42:59, sky wrote: > >> What happens if viewport_bottom is > height of view? > > > > That would make the height of the view 0. > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/re... > > > > Maybe we can add a check here and not set the viewport in this case? > > with a comment to that effect. > > > > https://codereview.chromium.org/1986153005/ > > -- > 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. > The way the keyboard is displayed today, it is in response to a tap i.e. cursor location. Don't see how you could have a tap occurring on a field which is below the keyboard y()?
On 2016/05/20 23:02:54, ananta wrote: > On 2016/05/20 22:58:22, sky wrote: > > It wouldn't make it 0, it would make the inset bigger than the height > > of the viewport. Seems like if the keyboard y position is above the > > view you shouldn't do anything. > > > > On Fri, May 20, 2016 at 3:52 PM, <mailto:ananta@chromium.org> wrote: > > > > > > > > > https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... > > > File content/browser/renderer_host/render_widget_host_view_aura.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1986153005/diff/360001/content/browser/render... > > > content/browser/renderer_host/render_widget_host_view_aura.cc:257: > > > host_->GetView()->SetInsets(gfx::Insets(0, 0, viewport_bottom, 0)); > > > On 2016/05/20 22:42:59, sky wrote: > > >> What happens if viewport_bottom is > height of view? > > > > > > That would make the height of the view 0. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/re... > > > > > > Maybe we can add a check here and not set the viewport in this case? > > > with a comment to that effect. > > > > > > https://codereview.chromium.org/1986153005/ > > > > -- > > 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. > > > > The way the keyboard is displayed today, it is in response to a tap i.e. cursor > location. Don't see how you could have a tap occurring on > a field which is below the keyboard y()? I added a check for the viewport bottom bigger than the height of the view with a comment that a possible solution might be to move the window above the OSK.
Almost there. https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:246: return; If you early return in this function do you need to reset the insets? If not, why do you reset the insets on 275? https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:256: abs(keyboard_rect_dips.y() - bounds_in_screen.bottom()); Why do you do abs here? Shouldn't bounds_in_screen.bottom() always be > keyboard_rect_dips.y()? In other words I would expect no abs here and flip the order (with a possible DCHECK). Also, I find it moderately confusing that you have somethings here named dip, some pixels, and some with nothing. Generally everything is dips unless said otherwise. For example, this line looks confusing because you're subtracting dips from what looks like non-dips, when that isn't the case. So, keyboard_rect_dips should just keyboard_rect, similarly location_dips_screen just location_in_screen...
https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:246: return; On 2016/05/21 16:04:38, sky wrote: > If you early return in this function do you need to reset the insets? If not, > why do you reset the insets on 275? Reset the view port at the top of this function. https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:256: abs(keyboard_rect_dips.y() - bounds_in_screen.bottom()); On 2016/05/21 16:04:38, sky wrote: > Why do you do abs here? Shouldn't bounds_in_screen.bottom() always be > > keyboard_rect_dips.y()? In other words I would expect no abs here and flip the > order (with a possible DCHECK). > > Also, I find it moderately confusing that you have somethings here named dip, > some pixels, and some with nothing. Generally everything is dips unless said > otherwise. For example, this line looks confusing because you're subtracting > dips from what looks like non-dips, when that isn't the case. So, > keyboard_rect_dips should just keyboard_rect, similarly location_dips_screen > just location_in_screen... Done. https://codereview.chromium.org/1986153005/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:256: abs(keyboard_rect_dips.y() - bounds_in_screen.bottom()); On 2016/05/21 16:04:38, sky wrote: > Why do you do abs here? Shouldn't bounds_in_screen.bottom() always be > > keyboard_rect_dips.y()? In other words I would expect no abs here and flip the > order (with a possible DCHECK). > > Also, I find it moderately confusing that you have somethings here named dip, > some pixels, and some with nothing. Generally everything is dips unless said > otherwise. For example, this line looks confusing because you're subtracting > dips from what looks like non-dips, when that isn't the case. So, > keyboard_rect_dips should just keyboard_rect, similarly location_dips_screen > just location_in_screen... Done. https://codereview.chromium.org/1986153005/diff/380001/ui/base/win/osk_displa... File ui/base/win/osk_display_observer.h (right): https://codereview.chromium.org/1986153005/diff/380001/ui/base/win/osk_displa... ui/base/win/osk_display_observer.h:14: virtual ~OnScreenKeyboardObserver() {} Could not make this protected as per your previous review comment as it gives a compile error while allocating the derived class WinScreenKeyboardObserver
LGTM
lgtm with nits. is it possible to add tests for anything here? at least a unittest for the osk_path_ computation that would go red if the method for finding TabTip.exe ever stops working. https://codereview.chromium.org/1986153005/diff/420001/base/win/win_util.cc File base/win/win_util.cc (right): https://codereview.chromium.org/1986153005/diff/420001/base/win/win_util.cc#n... base/win/win_util.cc:37: #include "base/memory/singleton.h" unused https://codereview.chromium.org/1986153005/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:260: int viewport_bottom = bounds_in_screen.bottom() - keyboard_rect.y(); nit: indentation https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:8: #include "base/memory/singleton.h" unused https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:43: friend struct base::DefaultSingletonTraits<OnScreenKeyboardDisplayManager>; unused https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... File ui/base/win/osk_display_observer.h (right): https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... ui/base/win/osk_display_observer.h:7: namespace gfx { class Rect; }
Added a ui_base_unittest OnScreenKeyboardTest.OSKPath which lives in the newly added file osk_display_manager_unittest.cc for validating the OSK path. https://codereview.chromium.org/1986153005/diff/420001/base/win/win_util.cc File base/win/win_util.cc (right): https://codereview.chromium.org/1986153005/diff/420001/base/win/win_util.cc#n... base/win/win_util.cc:37: #include "base/memory/singleton.h" On 2016/05/24 18:22:34, grt (slow) wrote: > unused Done. https://codereview.chromium.org/1986153005/diff/420001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/420001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:260: int viewport_bottom = bounds_in_screen.bottom() - keyboard_rect.y(); On 2016/05/24 18:22:34, grt (slow) wrote: > nit: indentation Done. https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:8: #include "base/memory/singleton.h" On 2016/05/24 18:22:34, grt (slow) wrote: > unused Done. https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.h:43: friend struct base::DefaultSingletonTraits<OnScreenKeyboardDisplayManager>; On 2016/05/24 18:22:34, grt (slow) wrote: > unused Done. https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... File ui/base/win/osk_display_observer.h (right): https://codereview.chromium.org/1986153005/diff/420001/ui/base/win/osk_displa... ui/base/win/osk_display_observer.h:7: On 2016/05/24 18:22:34, grt (slow) wrote: > namespace gfx { > class Rect; > } Done.
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 TEST=Partially covered by ui_base_unittest OnScreenKeyboardTest.OSKPath ==========
LGTM https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:315: &osk_path_length, NULL) != ERROR_SUCCESS) { nullptr (and on previous line). https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager_unittest.cc (right): https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... ui/base/win/osk_display_manager_unittest.cc:29: if (osk_path.front() == L'"') { You should check the size too (otherwise 31 may crash). https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... ui/base/win/osk_display_manager_unittest.cc:31: osk_path.erase(osk_path.size() - 1); // erase the last character. Can this be simplified to: osk_path = osk_path.substr(1, os_path.size() - 2) ?
https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... ui/base/win/osk_display_manager.cc:315: &osk_path_length, NULL) != ERROR_SUCCESS) { On 2016/05/24 21:41:26, sky wrote: > nullptr (and on previous line). Done. https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... File ui/base/win/osk_display_manager_unittest.cc (right): https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... ui/base/win/osk_display_manager_unittest.cc:29: if (osk_path.front() == L'"') { On 2016/05/24 21:41:26, sky wrote: > You should check the size too (otherwise 31 may crash). Added some more checks above this if which removes the need to check the size. https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_displa... ui/base/win/osk_display_manager_unittest.cc:31: osk_path.erase(osk_path.size() - 1); // erase the last character. On 2016/05/24 21:41:26, sky wrote: > Can this be simplified to: > osk_path = osk_path.substr(1, os_path.size() - 2) ? Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, grt@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1986153005/#ps460001 (title: "Address sky unittest review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986153005/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986153005/460001
Message was sent while issue was closed.
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 TEST=Partially covered by ui_base_unittest OnScreenKeyboardTest.OSKPath ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 TEST=Partially covered by ui_base_unittest OnScreenKeyboardTest.OSKPath ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 TEST=Partially covered by ui_base_unittest OnScreenKeyboardTest.OSKPath ========== to ========== The on screen keyboard on Windows 8+ should not obscure the input field. The behavior we want is for the input field to scroll up when it is obscured by the keyboard. We attempt to mimic the behavior in Chrome OS where the viewport is changed to the bounds of the OSK and the input field is scrolled into the intersecting rectangle. Added the following classes which live in ui\base\win\osk_display_manager.h/.cc 1. OnScreenKeyboardDisplayManager 2. OnScreenKeyboardDetector These classes provide functionality to detect whether the keyboard was displayed or hidden via a posted delayed task They optionally notify observers who implement the OnScreenKeyboardObserver interface. The observers in turn can detect if the keyboard is obscuring the view and can change the viewport and scroll the input field. This behavior is implemented by the OnScreenKeyboardObserver class which lives in render_widget_host_view_aura.cc BUG=608579 TEST=Partially covered by ui_base_unittest OnScreenKeyboardTest.OSKPath Committed: https://crrev.com/4297d7d9e0cd33ac6673ec6a839b1501baabeba8 Cr-Commit-Position: refs/heads/master@{#395754} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/4297d7d9e0cd33ac6673ec6a839b1501baabeba8 Cr-Commit-Position: refs/heads/master@{#395754} |
