Only user gesture focus change and mouse click show autofill.

Only user gesture focus change and mouse click show autofill.

BUG=428087
Committed: https://crrev.com/3f97f1ac106950c0ffbfc5f4cee787f24cdb4d50
Cr-Commit-Position: refs/heads/master@{#347887}

[please use gerrit instead, 2015-08-21 01:27:30]

rouslan@chromium.org changed reviewers:
+ estade@chromium.org

[please use gerrit instead, 2015-08-21 01:27:31]

Evan, PTAL.

This patch prevents showing the keyboard accessory when javascript or autofocus
attribute focuses an input field. When the user actually taps an already focused
field, the keyboard accessory should show up, though. So that's why I've
modified click handling as well.

[Evan Stade, 2015-08-21 15:16:20]

your bug number points to a pretty generic bug with no discussion of desired
click/focus/display semantics

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
components/autofill/content/renderer/page_click_tracker.cc:79: void
PageClickTracker::OnMouseDown(const WebNode& mouse_down_node) {
does a tap count as a "mouse down" here? if not why is there no tap handling

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
components/autofill/content/renderer/page_click_tracker.cc:80: if
(!WebUserGestureIndicator::isProcessingUserGesture())
schwa?

How can you have onmousedown that's not a user gesture?

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
components/autofill/content/renderer/page_click_tracker.cc:88:
DoFocusChangeComplete();
remind me why we need different handling on android and desktop

if you can show the popup on desktop without a user gesture that also seems bad

[please use gerrit instead, 2015-08-24 17:58:42]

Evan, PTAL Patch Set 2.

Sorry that http://crbug.com/428087 does not provide enough context for this
particular patch. I use it as a collector for all of the patches that bring the
keyboard accessory closer to the desired state. When a patch may be not obvious,
I explain it further in the patch description and/or in the first PTAL message.
Sometimes I am not as lucid as I wish :-). See my comments below for more
explanations.

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
components/autofill/content/renderer/page_click_tracker.cc:79: void
PageClickTracker::OnMouseDown(const WebNode& mouse_down_node) {
On 2015/08/21 15:16:19, Evan Stade wrote:
> does a tap count as a "mouse down" here? if not why is there no tap handling

See http://crrev.com/1195473005 for context. Both click and tap will do
hit-testing to figure out which node is hit and call OnMouseDown(node) for the
node that was hit.

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
components/autofill/content/renderer/page_click_tracker.cc:80: if
(!WebUserGestureIndicator::isProcessingUserGesture())
On 2015/08/21 15:16:19, Evan Stade wrote:
> schwa?
> 
> How can you have onmousedown that's not a user gesture?

I expected document.getElementsByTagName('input')[0].click() to trigger a non
user gesture mouse down event, but testing revealed my assumption was wrong.
Removed.

https://codereview.chromium.org/1304923002/diff/1/components/autofill/content...
components/autofill/content/renderer/page_click_tracker.cc:88:
DoFocusChangeComplete();
On 2015/08/21 15:16:19, Evan Stade wrote:
> remind me why we need different handling on android and desktop

We need different handling for keyboard accessory and popup, not android and
desktop.

The popup is attached to the input field, so we are showing it after the page
stops zooming and scrolling the input field into view. That happens when you tap
on the field. The callback for that event is FocuseChangeComplete().

The keyboard accessory is attached to the keyboard. It makes the content area
shorter by 48dp. Therefore, it does not depend on the input field position. In
fact, it's the opposite: the input field position is affected by the keyboard
accessory presence. Showing the keyboard accessory in FocusChangeComplete()
could cause a secondary zoom and scroll animation. That would look bad.
Therefore, we should show the keyboard accessory in FocusedNodeChanged(), which
is before the zoom/scroll animation. That way only one zoom/scroll animation
will happen.

Another good question would be: why should OnMouseDown() show the keyboard
accessory, but not the popup. Both OnMouseDown() and FocusedNodeChanged() will
be followed by FocusChangeComplete(). FocusChangeComplete() is actually a bad
name, but I have not been able to come up with a better name, unfortunately. One
candidate was OnImeShownAnimationsComplete(). See http://crrev.com/886663002 for
context. Since FocusChangeComplete() shows the autofill popup, no changes are
needed here.

As for the keyboard accessory, it needs to be shown in OnMouseDown() for the
following repro case:

1) User taps on input field -> This shows keyboard and accessory.

2) User taps the triangle to hide the keyboard -> This hides the accessory, but
the input field is still focused.

3) User taps the input field again -> The keyboard shows, but the accessory will
not be shown, because there was no focus change.
 
> if you can show the popup on desktop without a user gesture that also seems
bad

I have not been able to trigger the popup on desktop without a user gesture.
However, both autofocus and JavaScript can trigger FocusedNodeChanged(), so the
keyboard accessory needs to check user gesture.

[Evan Stade, 2015-09-08 18:13:01]

https://codereview.chromium.org/1304923002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/1304923002/diff/40001/components/autofill/con...
components/autofill/content/renderer/page_click_tracker.cc:89: return;
I don't think it's right to early return before you change
was_focused_before_now_

[please use gerrit instead, 2015-09-08 21:18:08]

Evan, PTAL Patch Set 4.

https://codereview.chromium.org/1304923002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/1304923002/diff/40001/components/autofill/con...
components/autofill/content/renderer/page_click_tracker.cc:89: return;
On 2015/09/08 18:13:01, Evan Stade wrote:
> I don't think it's right to early return before you change
> was_focused_before_now_

Moved the early return before changing was_focused_before_now_.

[Evan Stade, 2015-09-08 21:26:32]

lgtm but this code looks like it's getting kinda brittle. Maybe it needs some
tests?

https://codereview.chromium.org/1304923002/diff/60001/components/autofill/con...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/1304923002/diff/60001/components/autofill/con...
components/autofill/content/renderer/page_click_tracker.cc:90: if
(!WebUserGestureIndicator::isProcessingUserGesture())
I think this makes more sense as

if (IsKeyboardAccessoryEnabled() && ...::isProcessingUserGesture()) {
  dostuff;
}

[please use gerrit instead, 2015-09-09 00:20:16]

Test work in progress @ http://crrev.com/1305353013, not ready for review yet.

Sending this to cq in the meantime.

https://codereview.chromium.org/1304923002/diff/60001/components/autofill/con...
File components/autofill/content/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/1304923002/diff/60001/components/autofill/con...
components/autofill/content/renderer/page_click_tracker.cc:90: if
(!WebUserGestureIndicator::isProcessingUserGesture())
On 2015/09/08 21:26:32, Evan Stade wrote:
> I think this makes more sense as
> 
> if (IsKeyboardAccessoryEnabled() && ...::isProcessingUserGesture()) {
>   dostuff;
> }

Done.

[please use gerrit instead, 2015-09-09 00:20:38]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2015-09-09 00:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1304923002/#ps80001
(title: "Single if statement.")

[commit-bot: I haz the power, 2015-09-09 00:21:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304923002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304923002/80001

[commit-bot: I haz the power, 2015-09-09 09:17:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-09 09:18:14]

Patchset 5 (id:??) landed as
https://crrev.com/3f97f1ac106950c0ffbfc5f4cee787f24cdb4d50
Cr-Commit-Position: refs/heads/master@{#347887}