Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(449)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+673 lines, -158 lines) Patch
M base/win/win_util.h View 1 2 3 1 chunk +0 lines, -8 lines 0 comments Download
M base/win/win_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +0 lines, -103 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.h View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +15 lines, -42 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +12 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 6 chunks +131 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/browser/render_widget_host_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +9 lines, -0 lines 0 comments Download
M ui/base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +4 lines, -0 lines 0 comments Download
M ui/base/ui_base.gyp View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M ui/base/ui_base_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
A ui/base/win/osk_display_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +57 lines, -0 lines 0 comments Download
A ui/base/win/osk_display_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +364 lines, -0 lines 0 comments Download
A ui/base/win/osk_display_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +35 lines, -0 lines 0 comments Download
A ui/base/win/osk_display_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +28 lines, -0 lines 0 comments Download
M ui/views/controls/textfield/textfield.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +6 lines, -2 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 52 (12 generated)
ananta
4 years, 7 months ago (2016-05-18 03:20:21 UTC) #4
grt (UTC plus 2)
cool stuff. drive-by of base/win. https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.cc File base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.cc#newcode1 base/win/osk_display_manager.cc:1: // Copyright (c) 2016 ...
4 years, 7 months ago (2016-05-18 15:41:20 UTC) #6
sky
I'm not an owner of any of this code. sky->nick
4 years, 7 months ago (2016-05-18 19:26:15 UTC) #8
sky
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h#newcode19 base/win/osk_display_manager.h:19: virtual void OnKeyboardVisible(const RECT& keyboard_rect) {} RECT->gfx::Rect for these ...
4 years, 7 months ago (2016-05-18 19:54:52 UTC) #9
ncarter (slow)
https://codereview.chromium.org/1986153005/diff/40001/content/browser/renderer_host/render_view_host_impl.cc File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/40001/content/browser/renderer_host/render_view_host_impl.cc#newcode172 content/browser/renderer_host/render_view_host_impl.cc:172: #if defined(OS_WIN) This is getting to be more OS_WIN ...
4 years, 7 months ago (2016-05-18 20:09:12 UTC) #10
ananta
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.cc File base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.cc#newcode40 base/win/osk_display_manager.cc:40: ClearObservers(); On 2016/05/18 15:41:19, grt (slow) wrote: > is ...
4 years, 7 months ago (2016-05-19 01:57:13 UTC) #11
ananta
https://codereview.chromium.org/1986153005/diff/40001/content/browser/renderer_host/render_view_host_impl.cc File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/40001/content/browser/renderer_host/render_view_host_impl.cc#newcode190 content/browser/renderer_host/render_view_host_impl.cc:190: ::GetCursorPos(&cursor_pos); On 2016/05/18 19:54:51, sky wrote: > Is the ...
4 years, 7 months ago (2016-05-19 02:18:47 UTC) #12
grt (UTC plus 2)
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h#newcode59 base/win/osk_display_manager.h:59: HWND main_window_; On 2016/05/19 01:57:12, ananta wrote: > On ...
4 years, 7 months ago (2016-05-19 03:51:03 UTC) #15
sky
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h#newcode91 base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/19 03:51:02, grt (slow) wrote: > ...
4 years, 7 months ago (2016-05-19 16:05:26 UTC) #16
grt (UTC plus 2)
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h#newcode91 base/win/osk_display_manager.h:91: OnScreenKeyboardDisplayManager() {} On 2016/05/19 16:05:25, sky wrote: > On ...
4 years, 7 months ago (2016-05-19 16:53:17 UTC) #17
ncarter (slow)
Thanks for the RWHV rework, I like how it came out. content is looking good ...
4 years, 7 months ago (2016-05-19 17:45:55 UTC) #18
ncarter (slow)
https://codereview.chromium.org/1986153005/diff/160001/content/browser/renderer_host/render_widget_host_view_base.cc File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/renderer_host/render_widget_host_view_base.cc#newcode255 content/browser/renderer_host/render_widget_host_view_base.cc:255: DVLOG(1) << "FocusedNodeTouched: " << editable; I wonder if ...
4 years, 7 months ago (2016-05-19 18:03:24 UTC) #19
ananta
https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h File base/win/osk_display_manager.h (right): https://codereview.chromium.org/1986153005/diff/40001/base/win/osk_display_manager.h#newcode59 base/win/osk_display_manager.h:59: HWND main_window_; On 2016/05/19 03:51:03, grt (slow) wrote: > ...
4 years, 7 months ago (2016-05-19 19:33:38 UTC) #20
ncarter (slow)
content lgtm https://codereview.chromium.org/1986153005/diff/220001/content/browser/renderer_host/render_view_host_impl.cc File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1986153005/diff/220001/content/browser/renderer_host/render_view_host_impl.cc#newcode1271 content/browser/renderer_host/render_view_host_impl.cc:1271: // Pass this information from blink. Getting ...
4 years, 7 months ago (2016-05-19 21:46:16 UTC) #21
ananta
https://codereview.chromium.org/1986153005/diff/160001/content/browser/renderer_host/render_widget_host_view_base.cc File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/1986153005/diff/160001/content/browser/renderer_host/render_widget_host_view_base.cc#newcode255 content/browser/renderer_host/render_widget_host_view_base.cc:255: DVLOG(1) << "FocusedNodeTouched: " << editable; On 2016/05/19 19:33:38, ...
4 years, 7 months ago (2016-05-19 22:00:56 UTC) #22
sky
https://codereview.chromium.org/1986153005/diff/240001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/240001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode228 content/browser/renderer_host/render_widget_host_view_aura.cc:228: void DismissVirtualKeyboardTask() { I think this should be handled ...
4 years, 7 months ago (2016-05-19 22:25:09 UTC) #23
ananta
https://codereview.chromium.org/1986153005/diff/240001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/240001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode228 content/browser/renderer_host/render_widget_host_view_aura.cc:228: void DismissVirtualKeyboardTask() { On 2016/05/19 22:25:08, sky wrote: > ...
4 years, 7 months ago (2016-05-19 23:15:40 UTC) #24
grt (UTC plus 2)
https://codereview.chromium.org/1986153005/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode226 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: ...
4 years, 7 months ago (2016-05-20 14:56:35 UTC) #25
ananta
https://codereview.chromium.org/1986153005/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode226 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: ...
4 years, 7 months ago (2016-05-20 19:32:55 UTC) #26
sky
https://codereview.chromium.org/1986153005/diff/300001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/300001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode227 content/browser/renderer_host/render_widget_host_view_aura.cc:227: const gfx::Point& location_dips, It looks like this is in ...
4 years, 7 months ago (2016-05-20 19:57:16 UTC) #27
grt (UTC plus 2)
https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_display_manager.cc File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_display_manager.cc#newcode27 ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 14:56:35, ...
4 years, 7 months ago (2016-05-20 20:29:24 UTC) #28
ananta
https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_display_manager.cc File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/280001/ui/base/win/osk_display_manager.cc#newcode27 ui/base/win/osk_display_manager.cc:27: static const int kCheckOSKDelayMs = 1000; On 2016/05/20 14:56:35, ...
4 years, 7 months ago (2016-05-20 20:54:18 UTC) #29
sky
https://codereview.chromium.org/1986153005/diff/340001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/340001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode234 content/browser/renderer_host/render_widget_host_view_aura.cc:234: if (host_ && host_->GetView()) Remove check as it's done ...
4 years, 7 months ago (2016-05-20 22:14:56 UTC) #30
ananta
https://codereview.chromium.org/1986153005/diff/300001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/300001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode255 content/browser/renderer_host/render_widget_host_view_aura.cc:255: // The viewport needs to be moved up by ...
4 years, 7 months ago (2016-05-20 22:21:31 UTC) #31
sky
Only one question. https://codereview.chromium.org/1986153005/diff/360001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/360001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode257 content/browser/renderer_host/render_widget_host_view_aura.cc:257: host_->GetView()->SetInsets(gfx::Insets(0, 0, viewport_bottom, 0)); What happens ...
4 years, 7 months ago (2016-05-20 22:42:59 UTC) #32
ananta
https://codereview.chromium.org/1986153005/diff/360001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/360001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode257 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: ...
4 years, 7 months ago (2016-05-20 22:52:33 UTC) #33
sky
It wouldn't make it 0, it would make the inset bigger than the height of ...
4 years, 7 months ago (2016-05-20 22:58:22 UTC) #34
ananta
On 2016/05/20 22:58:22, sky wrote: > It wouldn't make it 0, it would make the ...
4 years, 7 months ago (2016-05-20 23:02:54 UTC) #35
ananta
On 2016/05/20 23:02:54, ananta wrote: > On 2016/05/20 22:58:22, sky wrote: > > It wouldn't ...
4 years, 7 months ago (2016-05-20 23:15:27 UTC) #36
sky
Almost there. https://codereview.chromium.org/1986153005/diff/380001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/380001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode246 content/browser/renderer_host/render_widget_host_view_aura.cc:246: return; If you early return in this ...
4 years, 7 months ago (2016-05-21 16:04:38 UTC) #37
ananta
https://codereview.chromium.org/1986153005/diff/380001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1986153005/diff/380001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode246 content/browser/renderer_host/render_widget_host_view_aura.cc:246: return; On 2016/05/21 16:04:38, sky wrote: > If you ...
4 years, 7 months ago (2016-05-23 19:32:07 UTC) #38
sky
LGTM
4 years, 7 months ago (2016-05-23 20:22:15 UTC) #39
grt (UTC plus 2)
lgtm with nits. is it possible to add tests for anything here? at least a ...
4 years, 7 months ago (2016-05-24 18:22:35 UTC) #40
ananta
Added a ui_base_unittest OnScreenKeyboardTest.OSKPath which lives in the newly added file osk_display_manager_unittest.cc for validating the ...
4 years, 7 months ago (2016-05-24 20:42:39 UTC) #41
sky
LGTM https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_display_manager.cc File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_display_manager.cc#newcode315 ui/base/win/osk_display_manager.cc:315: &osk_path_length, NULL) != ERROR_SUCCESS) { nullptr (and on ...
4 years, 7 months ago (2016-05-24 21:41:27 UTC) #43
ananta
https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_display_manager.cc File ui/base/win/osk_display_manager.cc (right): https://codereview.chromium.org/1986153005/diff/440001/ui/base/win/osk_display_manager.cc#newcode315 ui/base/win/osk_display_manager.cc:315: &osk_path_length, NULL) != ERROR_SUCCESS) { On 2016/05/24 21:41:26, sky ...
4 years, 7 months ago (2016-05-24 22:09:33 UTC) #44
ananta
4 years, 7 months ago (2016-05-24 22:09:37 UTC) #45
commit-bot: I haz the power
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
4 years, 7 months ago (2016-05-25 00:32:04 UTC) #48
commit-bot: I haz the power
Committed patchset #24 (id:460001)
4 years, 7 months ago (2016-05-25 00:39:32 UTC) #50
commit-bot: I haz the power
4 years, 7 months ago (2016-05-25 00:41:07 UTC) #52
Message was sent while issue was closed.
Patchset 24 (id:??) landed as
https://crrev.com/4297d7d9e0cd33ac6673ec6a839b1501baabeba8
Cr-Commit-Position: refs/heads/master@{#395754}

Powered by Google App Engine
This is Rietveld 408576698