Reset Frame::m_hasReceivedUserGesture for main frames on navigation

Reset Frame::m_hasReceivedUserGesture for main frames on navigation

Child frames have some degree of continuity across navigations: the
contents are still embedded in the same document. Let them continue to
propagate user gesture state across navigations. However, main frames
don't have that continuity and should reset their user gesture state
on navigation.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=chromedriver:922
Review-Url: https://codereview.chromium.org/2721423003
Cr-Commit-Position: refs/heads/master@{#456134}
Committed: https://chromium.googlesource.com/chromium/src/+/7da5699334a643bf58a2a9dc7f0104a41c7f4ef5

[Nate Chapin, 2017-03-02 00:57:18]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 00:57:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 03:08:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 03:08:01]

Dry run: This issue passed the CQ dry run.

[gmanikpure, 2017-03-02 19:37:40]

On 2017/03/02 03:08:01, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Hi Nate,

Regarding https://bugs.chromium.org/p/chromedriver/issues/detail?id=922#c8, I
verified the issue against this patch and the test seems to work fine everytime.
Thanks for the fix!

However, there is an older flaky test that fails with the same issue. Test
scenario is as follows:

1) Navigate to
https://github.com/SeleniumHQ/selenium/blob/master/common/src/web/javascriptP...
2) Type "foo" in element having id "theworks"
3) Type "bar" in element having id "changeable" // Here after typing, page zooms
in and keypad is shown.
4) Again, Navigate to the same page given in step 1)  // After navigation, the
page is again displayed as zoomed in.
5) Click on element with id "checkbox" // it fails here, click does not happen.

Can you please check if it is possible to get the fix for this as well?

Thanks,
Gauri

[Nate Chapin, 2017-03-02 19:43:00]

On 2017/03/02 19:37:40, gmanikpure wrote:
> On 2017/03/02 03:08:01, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Hi Nate,
> 
> Regarding https://bugs.chromium.org/p/chromedriver/issues/detail?id=922#c8, I
> verified the issue against this patch and the test seems to work fine
everytime.
> Thanks for the fix!
> 
> However, there is an older flaky test that fails with the same issue. Test
> scenario is as follows:
> 
> 1) Navigate to
>
https://github.com/SeleniumHQ/selenium/blob/master/common/src/web/javascriptP...
> 2) Type "foo" in element having id "theworks"
> 3) Type "bar" in element having id "changeable" // Here after typing, page
zooms
> in and keypad is shown.
> 4) Again, Navigate to the same page given in step 1)  // After navigation, the
> page is again displayed as zoomed in.
> 5) Click on element with id "checkbox" // it fails here, click does not
happen.
> 
> Can you please check if it is possible to get the fix for this as well?
> 
> Thanks,
> Gauri

When you say "older flaky test", do you mean it predates
https://chromium.googlesource.com/chromium/src/+/61835ae17fb417e72d42d7e4829c...
?

[samuong, 2017-03-02 20:50:25]

samuong@chromium.org changed reviewers:
+ samuong@chromium.org

[samuong, 2017-03-02 20:50:26]

It does pre-date that CL, yes. It's not a recent regression. I just asked Gauri
to mention it because it looks like a similar bug where the virtual keyboard
comes up unexpectedly after some automated input.

If it's hard to fix or needs to be done in a separate CL, then we should land
this now and worry about the old bug later.

[Nate Chapin, 2017-03-03 22:37:24]

On 2017/03/02 20:50:26, samuong wrote:
> It does pre-date that CL, yes. It's not a recent regression. I just asked
Gauri
> to mention it because it looks like a similar bug where the virtual keyboard
> comes up unexpectedly after some automated input.
> 
> If it's hard to fix or needs to be done in a separate CL, then we should land
> this now and worry about the old bug later.

I don't immediately see the solution to that one, my guess is it's unrelated.
The again, my knowledge of the virtual keyboard is entirely limited to the one
clause in Element.cpp. :/

[Nate Chapin, 2017-03-03 22:38:13]

japhet@chromium.org changed reviewers:
+ ojan@chromium.org

[Nate Chapin, 2017-03-03 22:38:13]

ojan, would you mind reviewing this? It looks like we have use cases that want
per-document user gesture tracking, as well as framebusting wanting per-frame
tracking. Which is kind of annoying, but not terribly surprising.

[Nate Chapin, 2017-03-08 22:08:35]

Description was changed from

==========
Keep separate m_hasReceivedUserGesture bits for Frame and Document

Some use cases want the state to propagate across navigation, some don't.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=
==========

to

==========
Keep separate m_hasReceivedUserGesture bits for Frame and Document

Some use cases want the state to propagate across navigation, some don't.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=chromedriver:922
==========

[Nate Chapin, 2017-03-08 22:08:39]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2017-03-08 22:08:39]

dcheng, would you mind checking this over?

[dcheng, 2017-03-08 22:21:33]

I'm worried that we'll be able to audit/correctly distinguish between the two
usages... I feel like it's easy for new code to just arbitrary pick one or the
other and not really notice if it's incorrect.

What would you think of making this an enum with three states? kNoUserGesture,
kActiveDocumentHadUserGesture, kHadUserGestureAtSomePointInThePast

(names obviously could use improvement =)

[Nate Chapin, 2017-03-08 22:37:16]

On 2017/03/08 22:21:33, dcheng wrote:
> I'm worried that we'll be able to audit/correctly distinguish between the two
> usages... I feel like it's easy for new code to just arbitrary pick one or the
> other and not really notice if it's incorrect.
> 
> What would you think of making this an enum with three states? kNoUserGesture,
> kActiveDocumentHadUserGesture, kHadUserGestureAtSomePointInThePast
> 
> (names obviously could use improvement =)

I could see that. I guess that just means one spot in Frame has to get the reset
from kActiveDocumentHadUserGesture to kHadUserGestureAtSomePointInThePast right,
and subtle logic in fewer places is better.

[ojan, 2017-03-09 00:45:31]

Sorry for the delay. This fell off the bottom of my inbox. I'm not convinced
that we need to change here. The only thing we're changing is whether a subframe
that had a gesture and was then navigated could still bring up the keyboard by
focusing an editable region, right?

I suspect we just need to update the Java tests here to expect different
behavior and that the loosening of the restriction we accidentally did before is
both web compatible and an improvement over the old behavior.

[Nate Chapin, 2017-03-09 23:50:34]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 23:51:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-03-09 23:52:15]

@ojan, what about this? reset the bit on commit for main frames only.

I think its reasonable to view iframes as "once gestured, always gestured". Main
frames, that feels a lot less intuitive, since there isn't the continuity of
being embedded by the same source.

WDYT?

[ojan, 2017-03-10 00:44:48]

Yikes. I thought this was already our behavior. I thought we recreated
LocalFrames when the top frame is navigated. I don't know why I thought that
though.

The behavior change lgtm, but I defer to dcheng on whether the code change is
correct.

[commit-bot: I haz the power, 2017-03-10 01:14:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 01:14:48]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-10 03:00:59]

On 2017/03/10 00:44:48, ojan wrote:
> Yikes. I thought this was already our behavior. I thought we recreated
> LocalFrames when the top frame is navigated. I don't know why I thought that
> though.
> 
> The behavior change lgtm, but I defer to dcheng on whether the code change is
> correct.

I believe we often change the main frame's LocalFrame on navigation (via a
process swap), but it isn't guaranteed. But dcheng would be a more definitive
source.

[Nate Chapin, 2017-03-10 19:20:14]

Description was changed from

==========
Keep separate m_hasReceivedUserGesture bits for Frame and Document

Some use cases want the state to propagate across navigation, some don't.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=chromedriver:922
==========

to

==========
Reset Frame::m_hasReceivedUserGesture for main frames on navigation

Child frames have some degree of continuity across navigations: the
contents are still embedded in the same document. Let them continue to
propagate user gesture state across navigations. However, main frames
don't have that continuity and should reset their user gesture state
on navigation.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=chromedriver:922
==========

[dcheng, 2017-03-10 19:21:15]

LGTM but update the CL description

[Nate Chapin, 2017-03-10 19:22:25]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:23:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:29:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489173745363570,
"parent_rev": "99e1012309d7f5f7eea0ff2157f06de806b3362c", "commit_rev":
"7da5699334a643bf58a2a9dc7f0104a41c7f4ef5"}

[commit-bot: I haz the power, 2017-03-10 19:30:20]

Description was changed from

==========
Reset Frame::m_hasReceivedUserGesture for main frames on navigation

Child frames have some degree of continuity across navigations: the
contents are still embedded in the same document. Let them continue to
propagate user gesture state across navigations. However, main frames
don't have that continuity and should reset their user gesture state
on navigation.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=chromedriver:922
==========

to

==========
Reset Frame::m_hasReceivedUserGesture for main frames on navigation

Child frames have some degree of continuity across navigations: the
contents are still embedded in the same document. Let them continue to
propagate user gesture state across navigations. However, main frames
don't have that continuity and should reset their user gesture state
on navigation.

TEST=DocumentUserGestureTokenTest.Navigate
BUG=chromedriver:922

Review-Url: https://codereview.chromium.org/2721423003
Cr-Commit-Position: refs/heads/master@{#456134}
Committed:
https://chromium.googlesource.com/chromium/src/+/7da5699334a643bf58a2a9dc7f01...
==========

[commit-bot: I haz the power, 2017-03-10 19:30:21]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7da5699334a643bf58a2a9dc7f01...