|
|
Created:
6 years, 1 month ago by please use gerrit instead Modified:
5 years, 11 months ago Reviewers:
aelias_OOO_until_Jul13, Torne, newt (away), Ian Vollick, yukawa, jdduke (slow), Aaron Boodman, aurimas (slooooooooow), nasko, Garrett Casto, Will Harris CC:
android-webview-reviews_chromium.org, benquan, bokan, browser-components-watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Garrett Casto, gcasto+watchlist_chromium.org, Ilya Sherman, jam, mkwst+moarreviews-renderer_chromium.org, mkwst+watchlist-passwords_chromium.org, nasko+codewatch_chromium.org, piman+watch_chromium.org, rouslan+autofillwatch_chromium.org, Shu Chen Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Show autofill popup after animation.
The renderer shows autofill popups immediately upon click or focus
change. If the browser shows an IME (virtual keyboard), then it can zoom
and scroll the webpage, which will hide the popup. This behavior causes
a "flash" of autofill popup on platforms with IME (ChromeOS, Windows 8,
and Android). This patch mostly fixes Android and paves the way for
fixing Windows 8 and ChromeOS.
The fix involves roughly 5 parts.
1) If the platform does not support a virtual keyboard (Mac, Linux,
Windows 7), the renderer shows the autofill popup immediately.
2) If a virtual keyboard is supported, but disabled (ChromeOS without a
virtual keyboard and Android with a hardware keyboard), then the
browser notifies the renderer that the autofill popup can be shown.
3) If the virtual keyboard is already showing, then the browser notifies
the renderer that the autofill popup can be shown.
A corner case is the Android keyboard, which can change its own
size when switching between input fields, thus resizing the webpage
and hiding the autofill popup. The mitigation is to hide the
autofill popup only if the input field moves due to the
resize. This mitigation is not in this patch.
4) If the input field is already in a good position (visible on screen
and legible size) after the keyboard is shown, then there will be no
zoom and scroll animations. Then the renderer can show the autofill
popup.
5) If the input field is zooming and scrolling into a good position
(visible on screen and legible size) after the keyboard is shown,
then the renderer waits until the compositor notifies it that the
this animation has finished. Then the renderer can show the autofill
popup.
BUG=430318, 440161
Committed: https://crrev.com/f7ebd883699112506893fc0500a65c5a10e563ec
Cr-Commit-Position: refs/heads/master@{#312521}
Patch Set 1 #
Total comments: 22
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Hide popup when resizing the viewport resulted in movement of the focused element. #
Total comments: 2
Patch Set 4 : Fix existing tests and add some more. #
Total comments: 4
Patch Set 5 : Rebase on top of https://codereview.chromium.org/826713002/ #
Total comments: 2
Patch Set 6 : Make this Android CL independent from the ChromeOS CL. #
Total comments: 8
Patch Set 7 : Add zoomed page click test. #
Total comments: 12
Patch Set 8 : UI thread where needed. #
Total comments: 12
Patch Set 9 : Inline test functions. #
Total comments: 2
Patch Set 10 : Use javadoc style comments. #Patch Set 11 : Rebase. #Messages
Total messages: 168 (103 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:150035) has been deleted
Patchset #1 (id:190001) has been deleted
Patchset #1 (id:210001) has been deleted
Patchset #1 (id:230001) has been deleted
Patchset #1 (id:250001) has been deleted
rouslan@chromium.org changed reviewers: + aelias@chromium.org
Alexandre: What do you think about these compositor changes? I'm getting cc_unittests failures, which I will investigate. Garrett: This patch pretty much does what we need: it does not break desktop behavior and fixes behavior on Android. It's kind of hacky, though, because I haven't yet figured out how to receive actual compositor events on desktop.
https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.h:483: // If set, then page scale animation has completed, but not committed yet. A page scale animation isn't explicitly involved in a commit after creation, so this phrasing is a bit strange. I'd say ", but we haven't notified the client about it yet (this is delayed until after commit to give the scroll offset time to fully stabilize)" https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.h:484: bool did_complete_scale_animation_before_commit_; Please move this next to "pending_page_scale_animation_". https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:102: virtual void DidCompletePageScaleAnimationBeforeCommit() = 0; I don't think it's needed to say "BeforeCommit" in all these places, it's just unnecessarily verbose. The methods are called as soon as the page scale animation completes, which is when one would expect it to be called. That just happens to be before commit. https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1342: void ThreadProxy::DidCompletePageScaleAnimationBeforeCommit() { This is called from the impl thread, so it should have "OnImplThread" in the name. https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1343: layer_tree_host()->DidCompletePageScaleAnimationBeforeCommit(); You shouldn't directly access the LayerTreeHost from the impl thread, it must only be accessed from the main thread. You should post a task on the main-thread that calls this instead. https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) Why? Shouldn't we hide on resize on Android, if we hide on scroll? I assume this is fixing a problem you've observed; please explain and maybe we can come up with a cleaner fix. https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:68: void WillAnimatePageScale(bool will_animate) override; FocusChangeComplete(bool will_animate) would be a better name. https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver, observers_, It's brittle to tie the autofill showing up at all to an optional scroll-into-view action that might be suppressed before we ever get here. And it also leads you to adding "#if !defined(OS_ANDROID)" in AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid. Please make autofill observer notified based on a mandatory event that happens on every form field focus instead (might need to introduce something like ViewMsg_FocusChangeComplete for that). And store will_animate_ in a member variable that defaults to false in this method.
Alexandre: Please see my comments inline. https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.h:483: // If set, then page scale animation has completed, but not committed yet. On 2014/11/22 03:38:11, aelias wrote: > A page scale animation isn't explicitly involved in a commit after creation, so > this phrasing is a bit strange. I'd say ", but we haven't notified the client > about it yet (this is delayed until after commit to give the scroll offset time > to fully stabilize)" Done. https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.h:484: bool did_complete_scale_animation_before_commit_; On 2014/11/22 03:38:11, aelias wrote: > Please move this next to "pending_page_scale_animation_". Done. https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/715733002/diff/270001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:102: virtual void DidCompletePageScaleAnimationBeforeCommit() = 0; On 2014/11/22 03:38:11, aelias wrote: > I don't think it's needed to say "BeforeCommit" in all these places, it's just > unnecessarily verbose. The methods are called as soon as the page scale > animation completes, which is when one would expect it to be called. That just > happens to be before commit. Done. https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1342: void ThreadProxy::DidCompletePageScaleAnimationBeforeCommit() { On 2014/11/22 03:38:11, aelias wrote: > This is called from the impl thread, so it should have "OnImplThread" in the > name. Done. https://codereview.chromium.org/715733002/diff/270001/cc/trees/thread_proxy.c... cc/trees/thread_proxy.cc:1343: layer_tree_host()->DidCompletePageScaleAnimationBeforeCommit(); On 2014/11/22 03:38:11, aelias wrote: > You shouldn't directly access the LayerTreeHost from the impl thread, it must > only be accessed from the main thread. You should post a task on the > main-thread that calls this instead. Done. https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) On 2014/11/22 03:38:11, aelias wrote: > Why? Shouldn't we hide on resize on Android, if we hide on scroll? I assume > this is fixing a problem you've observed; please explain and maybe we can come > up with a cleaner fix. This is to fix the autofill popup momentarily showing and then hiding when switching between input fields. When you switch between input fields, the popup shows up immediately, because there is no animation. However, the software keyboard then resizes itself to show/hide personalized suggestions [1]. These personalized suggestions show up for the username field, but not for the password field, for example. The keyboard resize causes the render to resize as well. The resize of the renderer then causes the autofill popup to hide itself immediately after it showed up. [1] http://www.pcmag.com/article2/0,2817,2455227,00.asp https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver, observers_, On 2014/11/22 03:38:11, aelias wrote: > It's brittle to tie the autofill showing up at all to an optional > scroll-into-view action that might be suppressed before we ever get here. And > it also leads you to adding "#if !defined(OS_ANDROID)" in > AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid. > > Please make autofill observer notified based on a mandatory event that happens > on every form field focus instead (might need to introduce something like > ViewMsg_FocusChangeComplete for that). And store will_animate_ in a member > variable that defaults to false in this method. FocusChangeComplete event is a good idea. On Android this can be invoked either from here or from DidCompletePageScaleAnimation. I am working on figuring out where to issue this event on desktop platforms. Let me know if you have suggestions.
Patchset #2 (id:290001) has been deleted
https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) On 2014/11/24 20:22:31, Rouslan Solomakhin wrote: > On 2014/11/22 03:38:11, aelias wrote: > > Why? Shouldn't we hide on resize on Android, if we hide on scroll? I assume > > this is fixing a problem you've observed; please explain and maybe we can come > > up with a cleaner fix. > > This is to fix the autofill popup momentarily showing and then hiding when > switching between input fields. > > When you switch between input fields, the popup shows up immediately, because > there is no animation. However, the software keyboard then resizes itself to > show/hide personalized suggestions [1]. These personalized suggestions show up > for the username field, but not for the password field, for example. The > keyboard resize causes the render to resize as well. The resize of the renderer > then causes the autofill popup to hide itself immediately after it showed up. > > [1] http://www.pcmag.com/article2/0,2817,2455227,00.asp OK, then should we not hide on resize on all platforms, if it's safe to do so? https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver, observers_, On 2014/11/24 20:22:31, Rouslan Solomakhin wrote: > On 2014/11/22 03:38:11, aelias wrote: > > It's brittle to tie the autofill showing up at all to an optional > > scroll-into-view action that might be suppressed before we ever get here. And > > it also leads you to adding "#if !defined(OS_ANDROID)" in > > AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid. > > > > Please make autofill observer notified based on a mandatory event that happens > > on every form field focus instead (might need to introduce something like > > ViewMsg_FocusChangeComplete for that). And store will_animate_ in a member > > variable that defaults to false in this method. > > FocusChangeComplete event is a good idea. On Android this can be invoked either > from here or from DidCompletePageScaleAnimation. I am working on figuring out > where to issue this event on desktop platforms. Let me know if you have > suggestions. To be clear, what I'm suggesting is that we should have the sequence with guaranteed ordering: 1. Focus 2. ScrollIntoView (optional) 3. FocusComplete 4. DidCompletePageScaleAnimation (optional) The first three events are very close to each other in time. On ScrollIntoView, we would set a flag if a page scale animation was issued, and on FocusComplete, we would either show autofill immediately if that flag is false, or wait until DidCompletePageScaleAnimation if true. This means we wouldn't need any platform-specific logic aside from not triggering the page scale animation. I don't know the focus code well enough to offer a very specific suggestion of how it should be added, although I think it would probably be in the browser process, who sends messages to trigger the first two -- we could send another message right after that.
Patchset #3 (id:330001) has been deleted
Patchset #2 (id:310001) has been deleted
Patchset #2 (id:350001) has been deleted
Patchset #2 (id:370001) has been deleted
Patchset #2 (id:390001) has been deleted
Patchset #2 (id:410001) has been deleted
Patchset #2 (id:430001) has been deleted
Patchset #2 (id:450001) has been deleted
Patchset #2 (id:470001) has been deleted
Patchset #2 (id:490001) has been deleted
Patchset #2 (id:510001) has been deleted
Patchset #2 (id:530001) has been deleted
Patchset #2 (id:550001) has been deleted
Patchset #2 (id:570001) has been deleted
Patchset #2 (id:590001) has been deleted
Patchset #2 (id:590001) has been deleted
Patchset #2 (id:610001) has been deleted
Patchset #2 (id:630001) has been deleted
Patchset #2 (id:650001) has been deleted
Patchset #2 (id:670001) has been deleted
Patchset #2 (id:670001) has been deleted
Patchset #2 (id:690001) has been deleted
Patchset #2 (id:710001) has been deleted
rouslan@chromium.org changed reviewers: + gcasto@chromium.org
Alexandre: PTAL Patch Set 2. I was able to get rid of platform #ifdefs. Also I fixed the Chrome OS virtual keyboard behavior. I'm yet to figure out how to fix linking errors for ChromeOS and browser_tests everywhere. Also I need to add tests for the new behavior. Garrett: Patch Set 2 should fix all popup misbehavior on Android. You may want to double-check the patch in your environment. https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) On 2014/11/25 01:26:34, aelias wrote: > On 2014/11/24 20:22:31, Rouslan Solomakhin wrote: > > On 2014/11/22 03:38:11, aelias wrote: > > > Why? Shouldn't we hide on resize on Android, if we hide on scroll? I > assume > > > this is fixing a problem you've observed; please explain and maybe we can > come > > > up with a cleaner fix. > > > > This is to fix the autofill popup momentarily showing and then hiding when > > switching between input fields. > > > > When you switch between input fields, the popup shows up immediately, because > > there is no animation. However, the software keyboard then resizes itself to > > show/hide personalized suggestions [1]. These personalized suggestions show up > > for the username field, but not for the password field, for example. The > > keyboard resize causes the render to resize as well. The resize of the > renderer > > then causes the autofill popup to hide itself immediately after it showed up. > > > > [1] http://www.pcmag.com/article2/0,2817,2455227,00.asp > > OK, then should we not hide on resize on all platforms, if it's safe to do so? I solved the issue slightly differently that does not require ignoring resize. Switching between input fields now waits for resize or a timeout before showing the popup. https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/715733002/diff/270001/content/renderer/render... content/renderer/render_view_impl.cc:1513: FOR_EACH_OBSERVER(RenderViewObserver, observers_, On 2014/11/25 01:26:34, aelias wrote: > On 2014/11/24 20:22:31, Rouslan Solomakhin wrote: > > On 2014/11/22 03:38:11, aelias wrote: > > > It's brittle to tie the autofill showing up at all to an optional > > > scroll-into-view action that might be suppressed before we ever get here. > And > > > it also leads you to adding "#if !defined(OS_ANDROID)" in > > > AutofillAgent::FocusedNodeChanged, which is something we prefer to avoid. > > > > > > Please make autofill observer notified based on a mandatory event that > happens > > > on every form field focus instead (might need to introduce something like > > > ViewMsg_FocusChangeComplete for that). And store will_animate_ in a member > > > variable that defaults to false in this method. > > > > FocusChangeComplete event is a good idea. On Android this can be invoked > either > > from here or from DidCompletePageScaleAnimation. I am working on figuring out > > where to issue this event on desktop platforms. Let me know if you have > > suggestions. > > To be clear, what I'm suggesting is that we should have the sequence with > guaranteed ordering: > 1. Focus > 2. ScrollIntoView (optional) > 3. FocusComplete > 4. DidCompletePageScaleAnimation (optional) > > The first three events are very close to each other in time. On ScrollIntoView, > we would set a flag if a page scale animation was issued, and on FocusComplete, > we would either show autofill immediately if that flag is false, or wait until > DidCompletePageScaleAnimation if true. This means we wouldn't need any > platform-specific logic aside from not triggering the page scale animation. > > I don't know the focus code well enough to offer a very specific suggestion of > how it should be added, although I think it would probably be in the browser > process, who sends messages to trigger the first two -- we could send another > message right after that. The step #2 (FocusComplete) is not necessary, because there's already an event that happens at the very end: OnHandledGestureEvent/OnHandledClickEvent/etc. Also, we don't have all of the required information on whether DidCompletePageScaleAnimation will happen at the end of the focus event in renderer, because browser makes some decisions as well. The solution is for the browser to let the renderer know whether it will initiate the page scale animation or not. See https://docs.google.com/document/d/1-5bhgEYhjHeJaPBYZZTLA0goc13epj1r_7bU-zFRP... for a diagram.
If you're OK with the overall approach, perhaps I should start splitting the CL into more digestible parts.
OK, the FocusChangedComplete stuff looks OK, thanks! https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) On 2014/12/15 18:39:55, Rouslan Solomakhin wrote: > I solved the issue slightly differently that does not require ignoring resize. > Switching between input fields now waits for resize or a timeout before showing > the popup. Hmm, I don't like the new timeout stuff in ContentViewCore either, sorry. Going back to first principles on this, I think the underlying reason to hide on resize is that if the page contents size changes due to the resize (particularly due to horizontal resizes on desktop), then the form field is likely to move around. Whereas with Android keyboard that's rarely an issue because the resize is purely vertical. But, even for vertical resizes, there are corner cases like fixed-position where the form field does move around. So, I'd suggest that when a resize and its corresponding layout happens, check to see if the form field node has moved or changed size, and only if so then hide the autofill popup. https://codereview.chromium.org/715733002/diff/730001/content/public/renderer... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/715733002/diff/730001/content/public/renderer... content/public/renderer/render_view_observer.h:100: // Can be called from browser, renderer, or compositor. Please add comment "This indicates that animations to scroll the focused element into view (if any) have completed. May be called more than once for a single focus."
Patchset #3 (id:750001) has been deleted
Patchset #3 (id:770001) has been deleted
A small comment inline. https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) On 2014/12/15 21:59:14, aelias wrote: > Going back to first principles on this, I think the underlying reason to hide on > resize is that if the page contents size changes due to the resize (particularly > due to horizontal resizes on desktop), then the form field is likely to move > around. Whereas with Android keyboard that's rarely an issue because the resize > is purely vertical. But, even for vertical resizes, there are corner cases like > fixed-position where the form field does move around. > > So, I'd suggest that when a resize and its corresponding layout happens, check > to see if the form field node has moved or changed size, and only if so then > hide the autofill popup. I like the first-principles approach and have cooked up something promising here. (Will be sharing later.) There is one interesting affect of not having the timeout when switching between fields: Some webpages run javascript which changes input element position on focus change. This is such a cornercase that I'm not worried about it, though. This can be resolved when a "real" fix for comes along. (I assume the real fix will the in-compositor implementation of autofill popups, but that's way down the road.)
https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:238: #if !defined(OS_ANDROID) On 2014/12/16 02:17:08, Rouslan Solomakhin wrote: > Some webpages run javascript which > changes input element position on focus change. This is such a cornercase that > I'm not worried about it, though. This can be resolved when a "real" fix for > comes along. (I assume the real fix will the in-compositor implementation of > autofill popups, but that's way down the road.) Assuming that Javascript usually will do that change synchronously, maybe the immediate answer to that one is to wait until the event processing completes before showing autofill.
On 2014/12/16 02:35:52, aelias wrote: > https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... > File components/autofill/content/renderer/autofill_agent.cc (right): > > https://codereview.chromium.org/715733002/diff/270001/components/autofill/con... > components/autofill/content/renderer/autofill_agent.cc:238: #if > !defined(OS_ANDROID) > On 2014/12/16 02:17:08, Rouslan Solomakhin wrote: > > Some webpages run javascript which > > changes input element position on focus change. This is such a cornercase that > > I'm not worried about it, though. This can be resolved when a "real" fix for > > comes along. (I assume the real fix will the in-compositor implementation of > > autofill popups, but that's way down the road.) > > Assuming that Javascript usually will do that change synchronously, maybe the > immediate answer to that one is to wait until the event processing completes > before showing autofill. My investigations show that we already do that. I've only noticed an issue with asynchronous JavaScript on twitter's mobile signup page (https://mobile.twitter.com/signup). If you type in a username that's already taken and tap on the password field, then JavaScript will asynchronously check whether the username is taken and show a message "Username has already been taken." Showing this message will move the password field, which hides the password generation popup. I think that (1) this is such a tiny corner-case that we should not worry about it at all; (2) moving around the focused element is a bad idea for JavaScript in the first place.
OK, sounds reasonable. Let me know when you have a new patch up.
Patchset #3 (id:790001) has been deleted
Patchset #3 (id:810001) has been deleted
Alexandre: PTAL Patch Set 3. Since RenderViewObserver::Resized() was used only by AutofillAgent to hide the popup, I renamed it to RenderVieObserver::FocusedElementMovedOnResize() and made it trigger only when the bounds of the focused element have changed due to viewport resize. https://codereview.chromium.org/715733002/diff/730001/content/public/renderer... File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/715733002/diff/730001/content/public/renderer... content/public/renderer/render_view_observer.h:100: // Can be called from browser, renderer, or compositor. On 2014/12/15 21:59:15, aelias wrote: > Please add comment "This indicates that animations to scroll the focused element > into view (if any) have completed. May be called more than once for a single > focus." Done.
This looks pretty clean to me now, thanks. Please just add test coverage and fix the try failures and then we can add OWNERS for landing. (I personally won't insist on splitting into smaller parts, personally I'm fine with large-ish patches.) https://codereview.chromium.org/715733002/diff/830001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/830001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:245: void AutofillAgent::OrientationChangeEvent() { Can we delete this one? It seems redundant with the resize mechanism.
Adding test coverage and fixing test failures. https://codereview.chromium.org/715733002/diff/830001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/830001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:245: void AutofillAgent::OrientationChangeEvent() { On 2014/12/16 23:15:49, aelias wrote: > Can we delete this one? It seems redundant with the resize mechanism. Evan is removing it in https://codereview.chromium.org/809623005/.
Patchset #4 (id:850001) has been deleted
Patchset #4 (id:870001) has been deleted
Patchset #4 (id:890001) has been deleted
Patchset #4 (id:910001) has been deleted
Patchset #4 (id:930001) has been deleted
Patchset #4 (id:950001) has been deleted
Patchset #4 (id:970001) has been deleted
Patchset #4 (id:990001) has been deleted
Patchset #4 (id:1010001) has been deleted
Patchset #4 (id:1030001) has been deleted
Patchset #4 (id:1050001) has been deleted
Patchset #4 (id:1050001) has been deleted
Patchset #4 (id:1070001) has been deleted
Patchset #4 (id:1090001) has been deleted
Patchset #5 (id:1130001) has been deleted
Patchset #5 (id:1150001) has been deleted
Patchset #5 (id:1170001) has been deleted
Patchset #5 (id:1190001) has been deleted
Patchset #5 (id:1210001) has been deleted
Patchset #5 (id:1230001) has been deleted
Patchset #5 (id:1250001) has been deleted
Patchset #5 (id:1270001) has been deleted
Patchset #5 (id:1290001) has been deleted
Patchset #4 (id:1110001) has been deleted
Patchset #4 (id:1310001) has been deleted
Alexandre: PTAL cc/ in Patch Set 4. I've added test coverage in layer_tree_host_impl_unittest.cc. Let me know if you can think of more tests to add. Other cc/ OWNERS: Feel free to take a look instead of Alexandre, while he is out of office.
rouslan@chromium.org changed reviewers: - gcasto@chromium.org
rouslan@chromium.org changed reviewers: + gcasto@chromium.org
Garrett: PTAL autofill in Patch Set 4: chrome/renderer/autofill/page_click_tracker_browsertest.cc chrome/renderer/autofill/password_generation_agent_browsertest.cc components/autofill/content/renderer/autofill_agent.h components/autofill/content/renderer/autofill_agent.cc components/autofill/content/renderer/form_autofill_util.h components/autofill/content/renderer/form_autofill_util.cc components/autofill/content/renderer/page_click_tracker.h components/autofill/content/renderer/page_click_tracker.cc Other autofill OWNERS: Feel free to take a look instead of Garrett, while he is out of office.
rouslan@chromium.org changed reviewers: + newt@chromium.org
Newton: PTAL AutofillPopupWithKeyboardTest.java in Patch Set 4: chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java
rouslan@chromium.org changed reviewers: + jbauman@chromium.org
John: PTAL render_widget_compositor Patch Set 4: content/renderer/gpu/render_widget_compositor.h content/renderer/gpu/render_widget_compositor.cc Other render_widget_compositor OWNERs: Feel free to take a look instead of John, while he is out of office.
rouslan@chromium.org changed reviewers: + msw@chromium.org
Mike: PTAL textfield in Patch Set 4: ui/views/controls/textfield/textfield.h ui/views/controls/textfield/textfield.cc Other textfield OWNERs: Feel free to take a look instead of Mike, while he is out of office.
rouslan@chromium.org changed reviewers: + benm@chromium.org
Ben: PTAL android in Patch Set 4: android_webview/browser/hardware_renderer.h content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java Other android OWNERs: Feel free to take a look instead of Ben, while he is out of office.
rouslan@chromium.org changed reviewers: + kevers@chromium.org
Kevin, PTAL keyboard_controller in Patch Set 4: ui/keyboard/keyboard_controller.cc Other keyboard_controller OWNERs, feel free to take a look instead of Kevin, while he is out of office.
rouslan@chromium.org changed reviewers: + wfh@chromium.org
Will: PTAL view_messages in Patch Set 4: content/common/view_messages.h Other view_messages OWNERs, feel free to take a look instead of Will, while he is out of office.
rouslan@chromium.org changed reviewers: + yukawa@chromium.org
Yohei, PTAL ime in Patch Set 4: ui/base/ime/dummy_input_method.h ui/base/ime/dummy_input_method.cc ui/base/ime/dummy_text_input_client.h ui/base/ime/dummy_text_input_client.cc ui/base/ime/input_method.h ui/base/ime/input_method_base.h ui/base/ime/input_method_base.cc ui/base/ime/input_method_chromeos_unittest.cc ui/base/ime/mock_input_method.h ui/base/ime/mock_input_method.cc ui/base/ime/remote_input_method_win.cc ui/base/ime/text_input_client.h Other ime OWNERs, feel free to take a look instead of Yohei, while he is out of office.
new IPC message in content/common/view_messages.h LGTM
rouslan@chromium.org changed reviewers: + vollick@chromium.org
Ian, PTAL compositor in Patch Set 4: ui/compositor/compositor.cc ui/compositor/compositor.h Other compositor OWNERs, feel free to take a look instead of Ian, while he is out of office.
rouslan@chromium.org changed reviewers: + yukishiino@chromium.org
Yuki, PTAL input_method_bridge in Patch Set 4: ui/views/ime/input_method_bridge.h ui/views/ime/input_method_bridge.cc
rouslan@chromium.org changed reviewers: + aa@chromium.org
Aaron, PTAL mojo in Patch Set 4: mojo/services/html_viewer/weblayertreeview_impl.h
rouslan@chromium.org changed reviewers: + nasko@chromium.org
Nasko, PTAL content in Patch Set 4: content/public/renderer/render_view_observer.h content/public/test/render_view_test.h content/public/test/render_view_test.cc content/renderer/render_view_impl.h content/renderer/render_view_impl.cc content/renderer/render_widget.h content/renderer/render_widget.cc content/test/web_layer_tree_view_impl_for_testing.h Other content OWNERs, feel free to take a look instead of Nasko, while he is out of office.
rouslan@chromium.org changed reviewers: + thakis@chromium.org
Nico, PTAL prefix_selector in Patch Set 4: ui/views/controls/prefix_selector.h ui/views/controls/prefix_selector.cc
Sorry for the "spam," everyone. This bug-fix requires piping events through multiple layers, which creates the need to touch many files. Please let me know if you would prefer if I split your part of the review into a separate CL. Happy Holidays!
lgtm Happy Holidays! It's OK with me to add some methods to ui/base/ime/input_method.h and ui/base/ime/text_input_client.h to https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/input_meth... File ui/base/ime/input_method.h (right): https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/input_meth... ui/base/ime/input_method.h:166: virtual void SetSupportsOnScreenKeyboard(bool supported) = 0; I'm still unsure why we need setter for this property. Why is this mutable? https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/input_meth... ui/base/ime/input_method.h:170: virtual bool SupportsOnScreenKeyboard() const = 0; Related to the above comment, if the return value of this method may change at run-time, maybe we may want to clarify with method name and/or comments. https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/text_input... File ui/base/ime/text_input_client.h (right): https://codereview.chromium.org/715733002/diff/1330001/ui/base/ime/text_input... ui/base/ime/text_input_client.h:182: virtual void OnKeyboardBoundsUnchanged() = 0; Sorry, I'm not sure if this comment is clear to other Chromium developers in terms of when this method may/should/must be called back. For example, I guess we can detect the timing "when the virtual keyboard was requested to be shown, but was already showing", and call this method at that moment, but I have a feeling that calling it on Windows causes some unexpected side effects at somewhere else. If we shouldn't call this method under certain conditions, maybe we can clarify it with method name and/or comments.
Oops, sorry, previous mail was my misoperation. not lgtm yet.
Adding new methods to some methods to ui/base/ime/input_method.h and ui/base/ime/text_input_client.h is OK with me. But I also want to minimize the risk of misuse of them. Can you create another CL that copies ui/base/ime/input_method.h and ui/base/ime/text_input_client.h from this CL with including yukishino@ and shuchen@ in the reviewer list? I'm happy to discuss what we can do there. Happy Holidays!
https://codereview.chromium.org/715733002/diff/1330001/ui/keyboard/keyboard_c... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/715733002/diff/1330001/ui/keyboard/keyboard_c... ui/keyboard/keyboard_controller.cc:482: proxy_->GetInputMethod()->GetTextInputClient()->OnKeyboardBoundsUnchanged(); Why the text input client want to be notified this event? Which class implements OnKeyboardBoundsUnchanged? Could you show your design plan at CL description? I need more clues to figure out your design.
yukishiino@chromium.org changed reviewers: + shuchen@chromium.org
Thank you for the prompt responses. The overarching theme seems to be confusion due to lack of design description. As Yohei and Yuki requested, I've moved the aura part of the review into a separate CL with an improved design description: https://codereview.chromium.org/826713002/ The following files will be reviewed in this separate CL: content/browser/renderer_host/render_widget_host_view_aura.h content/browser/renderer_host/render_widget_host_view_aura.cc content/common/view_messages.h ui/base/ime/dummy_input_method.h ui/base/ime/dummy_input_method.cc ui/base/ime/dummy_text_input_client.h ui/base/ime/dummy_text_input_client.cc ui/base/ime/input_method.h ui/base/ime/input_method_base.h ui/base/ime/input_method_base.cc ui/base/ime/input_method_chromeos_unittest.cc ui/base/ime/mock_input_method.h ui/base/ime/mock_input_method.cc ui/base/ime/remote_input_method_win.cc ui/base/ime/text_input_client.h ui/keyboard/keyboard_controller.cc ui/views/controls/prefix_selector.h ui/views/controls/prefix_selector.cc ui/views/controls/textfield/textfield.h ui/views/controls/textfield/textfield.cc ui/views/ime/input_method_bridge.h ui/views/ime/input_method_bridge.cc
rouslan@chromium.org changed reviewers: - shuchen@chromium.org, thakis@chromium.org, wfh@chromium.org, yukawa@chromium.org, yukishiino@chromium.org
aelias@chromium.org changed reviewers: + wfh@chromium.org, yukawa@chromium.org
lgtm, thanks.
Doesn't this CL depend on the ChromeOS one you pointed to (https://codereview.chromium.org/826713002/)? It is the one that contains the IPC message the browser side will send to the renderer. One nit while reviewing. https://codereview.chromium.org/715733002/diff/1350001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/715733002/diff/1350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:651: void CompositorImpl::DidCompletePageScaleAnimation() {} Why add an empty method here instead of inlining it in the header?
Patchset #6 (id:1370001) has been deleted
Nasko, PTAL Patch Set 6. This patch here is now completely independent of the ChromeOS patch and vice versa. The result is that this patch fixes the issue only Android. The ChromeOS patch will fix the issue on ChromeOS, after this patch lands. (Windows 8 fix still needs to be written.) https://codereview.chromium.org/715733002/diff/1350001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/715733002/diff/1350001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:651: void CompositorImpl::DidCompletePageScaleAnimation() {} On 2015/01/05 19:38:01, nasko wrote: > Why add an empty method here instead of inlining it in the header? Good catch! Inlined.
In general, we are trying to move away from RenderView to RenderFrame. It isn't as easy to do it in this change, but please keep it in mind in the future and structure code to avoid using RenderView. LGTM https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re... content/public/test/render_view_test.cc:338: void RenderViewTest::SimulateFocusChangeCompleteMessageReceived() { Is this method needed without the IPC message? Or will it be used in the future when all the changes have landed?
https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:217: element_.reset(); Is this safe? |element_| is mostly used to display information after communication with the browser. It seems like this could race with that IPC and possibly end up null. Admittedly while the current code wouldn't crash it would be pointing to the wrong value which isn't ideal either. Philosophically this seems to make more sense, but it seems like we need to add null checks everywhere |element_| is used. https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... components/autofill/content/renderer/page_click_tracker.cc:118: &focused_element).Contains(x, y)) { Was the old code incorrect when the page was zoomed? If so, would you mind adding a test for this now? https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... File components/autofill/content/renderer/page_click_tracker.h (right): https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... components/autofill/content/renderer/page_click_tracker.h:59: base::WeakPtrFactory<PageClickTracker> weak_ptr_factory_; This is unnecessary now.
Patchset #7 (id:1410001) has been deleted
Patchset #7 (id:1430001) has been deleted
Patchset #7 (id:1450001) has been deleted
Patchset #7 (id:1470001) has been deleted
Garrett, PTAL Patch Set 7. Nasko, please see my answer to your question inline. https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:217: element_.reset(); On 2015/01/06 20:01:09, Garrett Casto wrote: > Is this safe? |element_| is mostly used to display information after > communication with the browser. It seems like this could race with that IPC and > possibly end up null. Admittedly while the current code wouldn't crash it would > be pointing to the wrong value which isn't ideal either. Philosophically this > seems to make more sense, but it seems like we need to add null checks > everywhere |element_| is used. On a second look, nulling |element_| is not required. I removed this change. https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... components/autofill/content/renderer/page_click_tracker.cc:118: &focused_element).Contains(x, y)) { On 2015/01/06 20:01:09, Garrett Casto wrote: > Was the old code incorrect when the page was zoomed? If so, would you mind > adding a test for this now? Added a test. The old code was incorrect when the page was "scaled" using a pinch movement of your fingers on a touch screen (for example). Zoom was OK. You can experience the bug in Chrome on Android when you try to use the username field on https://form870.appspot.com/smoke.html with a zoomed in page. Zoom the page using a two-finger pinch on your Android device screen. The old code does not trigger the username suggestion popup when you tap the username. The new code triggers it. https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... File components/autofill/content/renderer/page_click_tracker.h (right): https://codereview.chromium.org/715733002/diff/1390001/components/autofill/co... components/autofill/content/renderer/page_click_tracker.h:59: base::WeakPtrFactory<PageClickTracker> weak_ptr_factory_; On 2015/01/06 20:01:09, Garrett Casto wrote: > This is unnecessary now. Removed. https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/715733002/diff/1390001/content/public/test/re... content/public/test/render_view_test.cc:338: void RenderViewTest::SimulateFocusChangeCompleteMessageReceived() { On 2015/01/05 21:49:23, nasko wrote: > Is this method needed without the IPC message? Or will it be used in the future > when all the changes have landed? Upon further investigation (which took a bit longer than I expected), this method is not necessary. Instead of simulating the event here, I can get the actual event by (1) changing SimulateMouseClick to send MouseUp after MouseDown and (2) registering a user gesture after loading a testing web page. The user gesture unlocks the "show-ime" event, which autofill agent now uses to determine when it's OK to show the autofill popup. The user gesture that I've decided to use is a mouse click on a dummy button on the webpage. This does not alter the result of the browser tests.
rouslan@chromium.org changed reviewers: - jbauman@chromium.org, kevers@chromium.org, msw@chromium.org, yukawa@chromium.org
Newton, ping for review of chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java. Ben, ping for review of: android_webview/browser/hardware_renderer.h content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java Ian, ping for review of ui/compositor/compositor.h. Aaron, ping for review of mojo/services/html_viewer/weblayertreeview_impl.h.
rouslan@chromium.org changed reviewers: - benm@chromium.org
rouslan@chromium.org changed reviewers: + jdduke@chromium.org, torne@chromium.org
Jared, PTAL in Patch Set 7: content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java Richard, PTAL in Patch Set 7: android_webview/browser/hardware_renderer.h
jdduke@chromium.org changed reviewers: + aurimas@chromium.org
> Jared, PTAL in Patch Set 7: > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java > > Owner lgtm, but I'd like aurimas@ to take a look at those changes as well. https://codereview.chromium.org/715733002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/715733002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1573: mWebContents.scrollFocusedEditableNodeIntoView(); Do we still need this logic?
Aurimas, PTAL in Patch Set 7: content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java
On 2015/01/14 23:04:02, Rouslan Solomakhin wrote: > Aurimas, PTAL in Patch Set 7: > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java ui/compositor lgtm.
Jared, please see my reply inline. Let me know if I misunderstood your comment. https://codereview.chromium.org/715733002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/715733002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1573: mWebContents.scrollFocusedEditableNodeIntoView(); On 2015/01/14 22:59:51, jdduke wrote: > Do we still need this logic? This logic scrolls the focused element into view after the virtual keyboard animates onto the screen. Without this logic, the focused element may end up behind the keyboard. My changes do not affect this logic. It's still necessary.
components/autofill/... and chrome/renderer/autofill/... LGTM
aurimas@chromium.org changed reviewers: + yukawa@chromium.org
On 2015/01/14 at 23:04:02, rouslan wrote: > Aurimas, PTAL in Patch Set 7: > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > content/public/android/javatests/src/org/chromium/content/browser/input/AdapterInputConnectionTest.java content/public/android/java/src/org/chromium/content/browser/* looks reasonable to me - LTGM https://codereview.chromium.org/715733002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/715733002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:298: == Configuration.KEYBOARD_NOKEYS) { Does KEYBOARD_NOKEYS catch bluetooth keyboards?
Aurimas, please see my reply inline. https://codereview.chromium.org/715733002/diff/1490001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/715733002/diff/1490001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:298: == Configuration.KEYBOARD_NOKEYS) { On 2015/01/15 00:50:16, aurimas wrote: > Does KEYBOARD_NOKEYS catch bluetooth keyboards? I've tested with a wired USB keyboard only. (I hooked up the USB keyboard to my tablet.) I assume that bluetooth keyboards behave the same as USB keyboards. If you have a bluetooth keyboard handy to test this patch, please be my guest :-)
On 2015/01/15 at 01:09:13, rouslan wrote: > Aurimas, please see my reply inline. > > https://codereview.chromium.org/715733002/diff/1490001/content/public/android... > File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): > > https://codereview.chromium.org/715733002/diff/1490001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:298: == Configuration.KEYBOARD_NOKEYS) { > On 2015/01/15 00:50:16, aurimas wrote: > > Does KEYBOARD_NOKEYS catch bluetooth keyboards? > > I've tested with a wired USB keyboard only. (I hooked up the USB keyboard to my tablet.) I assume that bluetooth keyboards behave the same as USB keyboards. If you have a bluetooth keyboard handy to test this patch, please be my guest :-) Verified it locally, it works fine with a bluetooth keyboard.
lgtm
lgtm (as the aura part is stripped out)
android_webview LGTM
https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java (right): https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:60: new AutofillTestHelper().setProfile(new AutofillProfile("", "https://www.example.com", These objects all live on the UI thread. You should access them only on the UI thread or risk flakiness down the road. Like this: ThreadUtils.runOnUiThreadBlocking(new Runnable() { @Override public void run() { ... } }); (This is an unending source of flakiness in our existing tests, which follow this rule only haphazardly.) https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:80: public boolean isSatisfied() { This should run on the UI thread, too. Also, lines 70, 85-88, 93 https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:89: assertTrue("Autofill Popup anchor view was never added.", this message doesn't seem quite right https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:108: viewCore.onSizeChanged(viewCore.getViewportWidthPix(), UI thread, etc. You get the point :)
Patchset #9 (id:1530001) has been deleted
Patchset #8 (id:1510001) has been deleted
Newton, PTAL Patch Set 8. https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java (right): https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:60: new AutofillTestHelper().setProfile(new AutofillProfile("", "https://www.example.com", On 2015/01/15 17:36:45, newt wrote: > These objects all live on the UI thread. You should access them only on the UI > thread or risk flakiness down the road. Like this: > > ThreadUtils.runOnUiThreadBlocking(new Runnable() { > @Override > public void run() { > ... > } > }); > > (This is an unending source of flakiness in our existing tests, which follow > this rule only haphazardly.) AutofillTestHelper.setProfile() handles threading correctly already. It posts a task on UI thread and then waits for it on the originating thread. https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:80: public boolean isSatisfied() { On 2015/01/15 17:36:45, newt wrote: > This should run on the UI thread, too. Also, lines 70, 85-88, 93 Done. https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:89: assertTrue("Autofill Popup anchor view was never added.", On 2015/01/15 17:36:45, newt wrote: > this message doesn't seem quite right Done. https://codereview.chromium.org/715733002/diff/1490001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:108: viewCore.onSizeChanged(viewCore.getViewportWidthPix(), On 2015/01/15 17:36:45, newt wrote: > UI thread, etc. You get the point :) Done.
AutofillPopupWithKeyboardTest.java looks better. Just some suggestions for making it shorter and easier to read. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java (right): https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:65: class Info { Another option would be to use AtomicReferences instead of declaring this Info class, e.g. final AtomicReference<ContentViewCore> viewCoreRef = new AtomicReference<ContentViewCore>(); ... ThreadUtils.runOnUiThreadBlocking(new Runnable() { @Override public void run() { viewCoreRef.set(getActivity().getActiveContentViewCore()); ... } }); Either way is fine. Sadly, neither is especially concise. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:163: private void waitForKeyboardShownOnUiThreadBlocking() throws InterruptedException { you could add a "visible" parameter to this method and use it for both the keyboard shown and keyboard hidden cases. e.g. private void waitForKeyboardVisibility(boolean isVisible) { ... } https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:164: assertTrue("Keyboard was never shown", CriteriaHelper.pollForCriteria(new Criteria() { use pollForUIThreadCriteria() here, then you can simplify isKeyboardShowingOnUiThreadBlocking() and get rid of the try/catch here https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:179: private void notifyViewHeightReducedOnUiThreadBlocking() { I'd remove "OnUiThreadBlocking" from these methods since they should *not* be called on the UI thread (which is what that naming scheme suggests to me). https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:192: private Boolean isKeyboardShowingOnUiThreadBlocking() This should return "boolean" (not Boolean). Same below. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:206: return ThreadUtils.runOnUiThreadBlocking(new Callable<View>() { I'd use runOnUiThreadBlockingNoException(). Then you don't have to add a "throws" clause or worry about catching exceptions when you call this method. Same below.
Patchset #9 (id:1570001) has been deleted
Newton, PTAL Patch Set 9. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java (right): https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:65: class Info { On 2015/01/20 21:28:09, newt wrote: > Another option would be to use AtomicReferences instead of declaring this Info > class, e.g. > > final AtomicReference<ContentViewCore> viewCoreRef = new > AtomicReference<ContentViewCore>(); > ... > ThreadUtils.runOnUiThreadBlocking(new Runnable() { > @Override > public void run() { > viewCoreRef.set(getActivity().getActiveContentViewCore()); > ... > } > }); > > Either way is fine. Sadly, neither is especially concise. Done, that saves 2 lines :-). https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:163: private void waitForKeyboardShownOnUiThreadBlocking() throws InterruptedException { On 2015/01/20 21:28:10, newt wrote: > you could add a "visible" parameter to this method and use it for both the > keyboard shown and keyboard hidden cases. e.g. > > private void waitForKeyboardVisibility(boolean isVisible) { > ... > } Done. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:164: assertTrue("Keyboard was never shown", CriteriaHelper.pollForCriteria(new Criteria() { On 2015/01/20 21:28:09, newt wrote: > use pollForUIThreadCriteria() here, then you can simplify > isKeyboardShowingOnUiThreadBlocking() and get rid of the try/catch here Done. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:179: private void notifyViewHeightReducedOnUiThreadBlocking() { On 2015/01/20 21:28:09, newt wrote: > I'd remove "OnUiThreadBlocking" from these methods since they should *not* be > called on the UI thread (which is what that naming scheme suggests to me). Done. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:192: private Boolean isKeyboardShowingOnUiThreadBlocking() On 2015/01/20 21:28:09, newt wrote: > This should return "boolean" (not Boolean). Same below. No longer relevant, as this method got inlined. https://codereview.chromium.org/715733002/diff/1550001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:206: return ThreadUtils.runOnUiThreadBlocking(new Callable<View>() { On 2015/01/20 21:28:09, newt wrote: > I'd use runOnUiThreadBlockingNoException(). Then you don't have to add a > "throws" clause or worry about catching exceptions when you call this method. > Same below. No longer necessary, as this got inlined.
AutofillPopupWithKeyboardTest.java lgtm https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java (right): https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:122: // Wait until the keyboard is showing and notify the ContentViewCore that its height was changed Nit: use javadoc style comment /** */
The CQ bit was checked by rouslan@chromium.org
https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java (right): https://codereview.chromium.org/715733002/diff/1590001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupWithKeyboardTest.java:122: // Wait until the keyboard is showing and notify the ContentViewCore that its height was changed On 2015/01/21 01:41:27, newt wrote: > Nit: use javadoc style comment /** */ Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/715733002/1610001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/autofill/content/renderer/autofill_agent.cc: While running git apply --index -3 -p1; error: patch failed: components/autofill/content/renderer/autofill_agent.cc:228 Falling back to three-way merge... Applied patch to 'components/autofill/content/renderer/autofill_agent.cc' with conflicts. U components/autofill/content/renderer/autofill_agent.cc Patch: components/autofill/content/renderer/autofill_agent.cc Index: components/autofill/content/renderer/autofill_agent.cc diff --git a/components/autofill/content/renderer/autofill_agent.cc b/components/autofill/content/renderer/autofill_agent.cc index 6afbd4ca8dd79d0d60c860e2d41e584424cf114d..7b27b8fff54b9cb813bde16cfe22f1f5b8f80387 100644 --- a/components/autofill/content/renderer/autofill_agent.cc +++ b/components/autofill/content/renderer/autofill_agent.cc @@ -49,6 +49,7 @@ using blink::WebAutofillClient; using blink::WebConsoleMessage; +using blink::WebDocument; using blink::WebElement; using blink::WebElementCollection; using blink::WebFormControlElement; @@ -214,12 +215,6 @@ void AutofillAgent::FocusedNodeChanged(const WebNode& node) { if (node.document().frame() != render_frame()->GetWebFrame()) return; - if (password_generation_agent_ && - password_generation_agent_->FocusedNodeHasChanged(node)) { - is_popup_possibly_visible_ = true; - return; - } - WebElement web_element = node.toConst<WebElement>(); if (!web_element.document().frame()) @@ -228,16 +223,28 @@ void AutofillAgent::FocusedNodeChanged(const WebNode& node) { const WebInputElement* element = toWebInputElement(&web_element); if (!element || !element->isEnabled() || element->isReadOnly() || - !element->isTextField() || element->isPasswordField()) + !element->isTextField()) return; element_ = *element; } -void AutofillAgent::Resized() { +void AutofillAgent::FocusedElementMovedOnResize() { HidePopup(); } +void AutofillAgent::FocusChangeComplete() { + WebDocument doc = render_frame()->GetWebFrame()->document(); + WebElement focused_element; + if (!doc.isNull()) + focused_element = doc.focusedElement(); + + if (!focused_element.isNull() && password_generation_agent_ && + password_generation_agent_->FocusedNodeHasChanged(focused_element)) { + is_popup_possibly_visible_ = true; + } +} + void AutofillAgent::didRequestAutocomplete( const WebFormElement& form) { DCHECK_EQ(form.document().frame(), render_frame()->GetWebFrame()); @@ -316,12 +323,6 @@ void AutofillAgent::FormControlElementClicked( switches::kEnableSingleClickAutofill)) { // Show full suggestions when clicking on an already-focused form field. On // the initial click (not focused yet), only show password suggestions. -#if defined(OS_ANDROID) - // TODO(gcasto): Remove after crbug.com/430318 has been fixed. - if (!was_focused) - return; -#endif - options.show_full_suggestion_list = options.show_full_suggestion_list || was_focused; options.show_password_suggestions_only = !was_focused; @@ -785,8 +786,12 @@ void AutofillAgent::LegacyAutofillAgent::FocusedNodeChanged( agent_->FocusedNodeChanged(node); } -void AutofillAgent::LegacyAutofillAgent::Resized() { - agent_->Resized(); +void AutofillAgent::LegacyAutofillAgent::FocusedElementMovedOnResize() { + agent_->FocusedElementMovedOnResize(); +} + +void AutofillAgent::LegacyAutofillAgent::FocusChangeComplete() { + agent_->FocusChangeComplete(); } } // namespace autofill
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/715733002/1630001
Message was sent while issue was closed.
Committed patchset #11 (id:1630001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f7ebd883699112506893fc0500a65c5a10e563ec Cr-Commit-Position: refs/heads/master@{#312521} |