Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22831045:
Delay virtual keyboard hiding for 100ms (Closed)
|
Created:
7 years, 4 months ago by bshe Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDelay virtual keyboard hiding for 100ms
This CL should fix this issue that we are experiencing(note it is not always reproducible):
- In a maximized window, clicking button may not take any effect due to immediate
keyboard relayout
(go to http://unixpapa.com/js/testkey.html and try hit reset button after typed
a bunch of keys)
It also workarounds the other two issues:
1. keyboard flicking when quickly switch from one input field to another field
2. In webdev javascript console, the input feild temoporary lost focus when
candidate popup window shows and it may try to hide and relayout keyboard, which
makes the console unuseable with virtual keyboard.
The root problem for the above two issues are tracked in issue 281493.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220648
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments #
Total comments: 6
Patch Set 3 : #
Total comments: 8
Patch Set 4 : More comments #
Messages
Total messages: 17 (0 generated)
Hi Kevin. Would you mind to take a look at this CL? Thanks! Biao
LGTM with nits. https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller.h:67: // This functions should be called with a delay to avoid keyboard flicking keyboard flicking --> layout flicker https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller_unittest.cc:301: // Keyboard should not immediately hide itself. Probably worth noting in the comment why the keyboard hide is delayed.
Drive-by: when switching between input-fields (i.e. issue 1. listed in the CL description), why do we see a flicker? Do we get notification about the text-input-type switching from _TEXT to _NONE back to _TEXT? https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller.h:68: // when the focus of input feild quickly change. *field
Thanks for the drive-up, Sadrul. I was about to add you as my reviewer. :) Yes, the flicker that I saw at login screen is because of the event chain: _TEXT -> _NONE -> _TEXT. https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller.h:67: // This functions should be called with a delay to avoid keyboard flicking On 2013/08/26 15:50:18, kevers wrote: > keyboard flicking --> layout flicker Done. https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller.h:68: // when the focus of input feild quickly change. On 2013/08/26 17:30:31, sadrul wrote: > *field Done. https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller_unittest.cc:301: // Keyboard should not immediately hide itself. On 2013/08/26 15:50:18, kevers wrote: > Probably worth noting in the comment why the keyboard hide is delayed. Done.
> Yes, the flicker that I saw at login screen is because of the event chain: _TEXT > -> _NONE -> _TEXT. That looks like a blink/content bug. Can we fix that instead? Similarly, the candidate window stealing focus temporarily also sounds like a bug? Can we fix that?
On 2013/08/26 23:09:12, sadrul wrote: > > Yes, the flicker that I saw at login screen is because of the event chain: > _TEXT > > -> _NONE -> _TEXT. > > That looks like a blink/content bug. Can we fix that instead? > > Similarly, the candidate window stealing focus temporarily also sounds like a > bug? Can we fix that? Aha. I didn't know that it is not expected. I am not familiar with code in blink/content. Who do you suggest that I should talk to? In any case, do you think this CL is still necessary for case 2 that I mention in description? If we relayout as soon as input field lost focus, some of the click event may not received correctly.
On 2013/08/26 23:17:44, bshe wrote: > On 2013/08/26 23:09:12, sadrul wrote: > > > Yes, the flicker that I saw at login screen is because of the event chain: > > _TEXT > > > -> _NONE -> _TEXT. > > > > That looks like a blink/content bug. Can we fix that instead? > > > > Similarly, the candidate window stealing focus temporarily also sounds like a > > bug? Can we fix that? > > Aha. I didn't know that it is not expected. I am not familiar with code in > blink/content. > Who do you suggest that I should talk to? From a quick glance, it does look like the it is an issue in the renderer side (as in, when switching between two input fields, the renderer sends two messages to the browser, saying no-input-field focused in the first message, and text-input-field focused in the second message). So this is likely a bug in blink. I don't really know who to talk to about this. > > In any case, do you think this CL is still necessary for case 2 that I mention > in description? > If we relayout as soon as input field lost focus, some of the click event may > not received > correctly. What is the bug here? I see that tapping the 'reset' button clears the text-entry, removes focus from the entry, and hides the keyboard. This seems to be the case when clicking the reset button with mouse too. So I am not sure what the bug this patch is trying to fix here.
On 2013/08/26 23:49:07, sadrul wrote: > On 2013/08/26 23:17:44, bshe wrote: > > On 2013/08/26 23:09:12, sadrul wrote: > > > > Yes, the flicker that I saw at login screen is because of the event chain: > > > _TEXT > > > > -> _NONE -> _TEXT. > > > > > > That looks like a blink/content bug. Can we fix that instead? > > > > > > Similarly, the candidate window stealing focus temporarily also sounds like > a > > > bug? Can we fix that? > > > > Aha. I didn't know that it is not expected. I am not familiar with code in > > blink/content. > > Who do you suggest that I should talk to? > > From a quick glance, it does look like the it is an issue in the renderer side > (as in, when switching between two input fields, the renderer sends two messages > to the browser, saying no-input-field focused in the first message, and > text-input-field focused in the second message). So this is likely a bug in > blink. I don't really know who to talk to about this. I see. Thanks for the investigation. I will ask blink folks here tomorrow. > > > > > In any case, do you think this CL is still necessary for case 2 that I mention > > in description? > > If we relayout as soon as input field lost focus, some of the click event may > > not received > > correctly. > > What is the bug here? I see that tapping the 'reset' button clears the > text-entry, removes focus from the entry, and hides the keyboard. This seems to > be the case when clicking the reset button with mouse too. So I am not sure what > the bug this patch is trying to fix here. Did you try it in a maximized window? I think it is only reproducible with maximized window as we do a re-layout for them when hide keyboard. What I saw is the text-entry didn't clear after tapping 'reset' button. And it looks like as soon as we touch the screen, the input box lost focus. Our virtual keyboard tries to re-layout the window immediately. When my finger leaves screen, since the window already re-layout, the click event didn't fire correctly. So the event listener on the button didn't run (didn't clear the text-entry).
On 2013/08/27 00:19:27, bshe wrote: > On 2013/08/26 23:49:07, sadrul wrote: > > On 2013/08/26 23:17:44, bshe wrote: > > > On 2013/08/26 23:09:12, sadrul wrote: > > > > > Yes, the flicker that I saw at login screen is because of the event > chain: > > > > _TEXT > > > > > -> _NONE -> _TEXT. > > > > > > > > That looks like a blink/content bug. Can we fix that instead? > > > > > > > > Similarly, the candidate window stealing focus temporarily also sounds > like > > a > > > > bug? Can we fix that? > > > > > > Aha. I didn't know that it is not expected. I am not familiar with code in > > > blink/content. > > > Who do you suggest that I should talk to? > > > > From a quick glance, it does look like the it is an issue in the renderer side > > (as in, when switching between two input fields, the renderer sends two > messages > > to the browser, saying no-input-field focused in the first message, and > > text-input-field focused in the second message). So this is likely a bug in > > blink. I don't really know who to talk to about this. > I see. Thanks for the investigation. I will ask blink folks here tomorrow. > > > > > > > > > In any case, do you think this CL is still necessary for case 2 that I > mention > > > in description? > > > If we relayout as soon as input field lost focus, some of the click event > may > > > not received > > > correctly. > > > > What is the bug here? I see that tapping the 'reset' button clears the > > text-entry, removes focus from the entry, and hides the keyboard. This seems > to > > be the case when clicking the reset button with mouse too. So I am not sure > what > > the bug this patch is trying to fix here. > > Did you try it in a maximized window? I think it is only reproducible with > maximized > window as we do a re-layout for them when hide keyboard. What I saw is the > text-entry > didn't clear after tapping 'reset' button. And it looks like as soon as we touch > the > screen, the input box lost focus. Our virtual keyboard tries to re-layout the > window > immediately. When my finger leaves screen, since the window already re-layout, > the > click event didn't fire correctly. So the event listener on the button didn't > run > (didn't clear the text-entry). Hi Sadrul. I have filed Issue 281493 to track eliminating the TEXT_INPUT_TYPE_NONE problem. Since this can still fix case 2 and other cases, would you mind to take another look? Thanks!
Looks good. Left a couple of comments. Please also update the CL description to note that this fixes (2), and the fix happens to workaround (1) and (3) https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... ui/keyboard/keyboard_controller.h:28: extern const int kHideKeyboardDelayMs; You don't need to expose this. See note in the test. https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... ui/keyboard/keyboard_controller_unittest.cc:34: base::TimeDelta::FromMilliseconds(kHideKeyboardDelayMs)); Don't post a delayed task. Instead, create an aura::WindowObserver for keyboard_container, which starts a nested-loop, and terminates it when the Window is shown/hidden. That would be cleaner, and make the test easier to follow.
Thanks for review. I have changed this issue description and comments are inlined. https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... ui/keyboard/keyboard_controller_unittest.cc:34: base::TimeDelta::FromMilliseconds(kHideKeyboardDelayMs)); I did try to add an observer. But then I found that it is hard to test this case show -> hide -> show -> cancel hide. I want to verify if hide is indeed canceled. There are two options: 1. check if weak_ptr is invalidated in KeyboardController(this is probably hacky and unreliable) 2. check if keyboard is still visible after the delayed time. If it is not visible, it probably means the hide is not correctly canceled. I found a similar pattern in the unit test for PostDelayTask. https://code.google.com/p/chromium/codesearch/#chromium/src/base/message_loop... So I assumed that this is probably acceptable? I might miss something, in that case, please feel free to point out. On 2013/08/29 17:06:33, sadrul wrote: > Don't post a delayed task. Instead, create an aura::WindowObserver for > keyboard_container, which starts a nested-loop, and terminates it when the > Window is shown/hidden. That would be cleaner, and make the test easier to > follow.
https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... ui/keyboard/keyboard_controller_unittest.cc:34: base::TimeDelta::FromMilliseconds(kHideKeyboardDelayMs)); On 2013/08/29 18:19:02, bshe wrote: > I did try to add an observer. But then I found that it is hard to test this case > show -> hide -> show -> cancel hide. > > I want to verify if hide is indeed canceled. There are two options: > 1. check if weak_ptr is invalidated in KeyboardController(this is probably hacky > and unreliable) KeyboardControllerTest is already friends with KeyboardController. So it'd be OK to check that the KC is going to hide the keyboard. I agree that checking the weak_ptr from here would be ugly, so I would add a private 'bool WillHideWindow() const { return weak_ptr_.HasWeakPtrs(); }' function to KC and use that here. > 2. check if keyboard is still visible after the delayed time. If it is not > visible, it probably means the hide is not correctly canceled. > I found a similar pattern in the unit test for PostDelayTask. > https://code.google.com/p/chromium/codesearch/#chromium/src/base/message_loop... > So I assumed that this is probably acceptable? > > I might miss something, in that case, please feel free to point out. We should normally avoid tests that depend on timing. It's of course expected that the code that tests PostDelayedTask() uses the function. But in most other cases, we should avoid it in tests.
Hi Sadrul. I think I have addressed your comment. Would you mind to take another look? Thanks! https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... ui/keyboard/keyboard_controller.h:28: extern const int kHideKeyboardDelayMs; On 2013/08/29 17:06:33, sadrul wrote: > You don't need to expose this. See note in the test. Done. https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/8001/ui/keyboard/keyboard_contr... ui/keyboard/keyboard_controller_unittest.cc:34: base::TimeDelta::FromMilliseconds(kHideKeyboardDelayMs)); On 2013/08/29 20:32:07, sadrul wrote: > On 2013/08/29 18:19:02, bshe wrote: > > I did try to add an observer. But then I found that it is hard to test this > case > > show -> hide -> show -> cancel hide. > > > > I want to verify if hide is indeed canceled. There are two options: > > 1. check if weak_ptr is invalidated in KeyboardController(this is probably > hacky > > and unreliable) > > KeyboardControllerTest is already friends with KeyboardController. So it'd be OK > to check that the KC is going to hide the keyboard. I agree that checking the > weak_ptr from here would be ugly, so I would add a private 'bool > WillHideWindow() const { return weak_ptr_.HasWeakPtrs(); }' function to KC and > use that here. > > > 2. check if keyboard is still visible after the delayed time. If it is not > > visible, it probably means the hide is not correctly canceled. > > I found a similar pattern in the unit test for PostDelayTask. > > > https://code.google.com/p/chromium/codesearch/#chromium/src/base/message_loop... > > So I assumed that this is probably acceptable? > > > > I might miss something, in that case, please feel free to point out. > > We should normally avoid tests that depend on timing. It's of course expected > that the code that tests PostDelayedTask() uses the function. But in most other > cases, we should avoid it in tests. Done.
LGTM with some nits https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller.h:69: bool WillHideKeyboard(); make this const https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller_unittest.cc:173: bool visible) OVERRIDE { Fix indent https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller_unittest.cc:312: EXPECT_TRUE(keyboard_container->IsVisible()); Also, EXPECT_TRUE(WillHideKeyboard()); https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller_unittest.cc:321: input_method->SetFocusedTextInputClient(&no_input_client_1); EXPECT_TRUE(WillHideKeyboard());
Done. Thanks for review https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller.h:69: bool WillHideKeyboard(); On 2013/08/29 23:43:54, sadrul wrote: > make this const Done. https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller_unittest.cc:173: bool visible) OVERRIDE { On 2013/08/29 23:43:54, sadrul wrote: > Fix indent Done. https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller_unittest.cc:312: EXPECT_TRUE(keyboard_container->IsVisible()); On 2013/08/29 23:43:54, sadrul wrote: > Also, EXPECT_TRUE(WillHideKeyboard()); Done. https://codereview.chromium.org/22831045/diff/26001/ui/keyboard/keyboard_cont... ui/keyboard/keyboard_controller_unittest.cc:321: input_method->SetFocusedTextInputClient(&no_input_client_1); On 2013/08/29 23:43:54, sadrul wrote: > EXPECT_TRUE(WillHideKeyboard()); Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/22831045/46001
Message was sent while issue was closed.
Change committed as 220648 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
