Bug 178183 – CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Brent Fulgham]

CMD+R / CMD+Q was considered as user interaction, but was mostly fixed in Bug 169995. However, we are still updating the most recent interaction time for these shortcuts, which has ramifications for anti-fraud efforts.

We should make sure we do not update interaction time or other relevant statistics unless the key event was handled.

[Brent Fulgham]

<rdar://problem/33327730>

[Brent Fulgham]

Created attachment 323458 [details]
Patch

[Build Bot]

Comment on attachment 323458 [details]
Patch

Attachment 323458 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4828084

New failing tests:
http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html
media/restricted-audio-playback-with-document-gesture.html
http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html
media/document-level-media-user-gesture-quirk.html
media/video-user-gesture-tracking.html

[Build Bot]

Created attachment 323467 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323458 [details]
Patch

Attachment 323458 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4828134

New failing tests:
media/restricted-audio-playback-with-document-gesture.html
media/document-level-media-user-gesture-quirk.html
media/video-user-gesture-tracking.html

[Build Bot]

Created attachment 323468 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 323458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323458&action=review

> Source/WebCore/dom/UserGestureIndicator.cpp:59
> -    if (document && currentToken()->processingUserGesture()) {
> +    if (document && currentToken()->processingUserGesture() && gestureType == UserGestureType::Other) {

So somehow UserGestureType::Other signifies that it's a keypress? We should rename Other to Keypress.
Are you sure waiting until keypress is Web compatible?

> Source/WebCore/page/EventHandler.cpp:3131
> -    if (topDocument && !wasHandled)
> -        topDocument->setUserDidInteractWithPage(savedUserDidInteractWithPage);
> +    if (topDocument && wasHandled) {
> +        topDocument->updateLastHandledUserGestureTimestamp(MonotonicTime::now());
> +        ResourceLoadObserver::shared().logUserInteractionWithReducedTimeResolution(topDocument->topDocument());
> +        topDocument->setUserDidInteractWithPage(true);

Why are we checking that wasHandled is true instead of false?
It looks like we never call setUserDidInteractWithPage with false now.

[Build Bot]

Comment on attachment 323458 [details]
Patch

Attachment 323458 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4828145

New failing tests:
http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html
media/restricted-audio-playback-with-document-gesture.html
http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html
media/document-level-media-user-gesture-quirk.html
media/video-user-gesture-tracking.html

[Build Bot]

Created attachment 323469 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323458 [details]
Patch

Attachment 323458 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4828152

New failing tests:
http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html

[Build Bot]

Created attachment 323470 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Brent Fulgham]

Comment on attachment 323458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323458&action=review

>> Source/WebCore/dom/UserGestureIndicator.cpp:59
>> +    if (document && currentToken()->processingUserGesture() && gestureType == UserGestureType::Other) {
> 
> So somehow UserGestureType::Other signifies that it's a keypress? We should rename Other to Keypress.
> Are you sure waiting until keypress is Web compatible?

Well, given the media test failures -- maybe it's not. I need to debug those tests.

>> Source/WebCore/page/EventHandler.cpp:3131
>> +        topDocument->setUserDidInteractWithPage(true);
> 
> Why are we checking that wasHandled is true instead of false?
> It looks like we never call setUserDidInteractWithPage with false now.

Previously, the key event would call 'setUserDidInteractWithPage' immediately, and would then call "setUserDidInteractWithPage(false)" here if we did not handle the keypress. My change defers setting this until we know if the key event was handled, so we only need to set it to true (since it was not set previously).

The previous code seems wrong to me, because we were setting updateLastHandledUserGestureTimestamp even if we later called 'setUserDidInteractWithPage(false)'.

[Brent Fulgham]

Created attachment 323545 [details]
Patch

[Ryosuke Niwa]

(In reply to Brent Fulgham from comment #12)
> Comment on attachment 323458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323458&action=review
> 
> >> Source/WebCore/dom/UserGestureIndicator.cpp:59
> >> +    if (document && currentToken()->processingUserGesture() && gestureType == UserGestureType::Other) {
> > 
> > So somehow UserGestureType::Other signifies that it's a keypress? We should rename Other to Keypress.
> > Are you sure waiting until keypress is Web compatible?
> 
> Well, given the media test failures -- maybe it's not. I need to debug those
> tests.
> 
> >> Source/WebCore/page/EventHandler.cpp:3131
> >> +        topDocument->setUserDidInteractWithPage(true);
> > 
> > Why are we checking that wasHandled is true instead of false?
> > It looks like we never call setUserDidInteractWithPage with false now.
> 
> Previously, the key event would call 'setUserDidInteractWithPage'
> immediately, and would then call "setUserDidInteractWithPage(false)" here if
> we did not handle the keypress. My change defers setting this until we know
> if the key event was handled, so we only need to set it to true (since it
> was not set previously).

I'm not certain this will be Web compatible change. What do other browsers do?

[Brent Fulgham]

Created attachment 323626 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 323626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323626&action=review

> Source/WebCore/page/EventHandler.cpp:3199
> -    UserGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType);
> +    UserKeypressGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType);

We definitely need tp keep UserGestureIndicator around because that's what we use to gate other features.

[Brent Fulgham]

Comment on attachment 323626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323626&action=review

>> Source/WebCore/page/EventHandler.cpp:3199
>> +    UserKeypressGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType);
> 
> We definitely need tp keep UserGestureIndicator around because that's what we use to gate other features.

UserKeypressGestureIndicator is a subclass of UserGestureIndicator that avoids triggering the "ResourceLoadObserver::logUserInteractionWIthReducedTimeResolution" until we confirm the key event was handled by the page. Otherwise, we don't update the statistics to credit the site with user interaction.

There are no other changes in behavior, and since only the ResourceLoadStatistics logic is modified, I don't think there can be any kind of compatibility issue.

[Ryosuke Niwa]

(In reply to Brent Fulgham from comment #17)
> Comment on attachment 323626 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323626&action=review
> 
> >> Source/WebCore/page/EventHandler.cpp:3199
> >> +    UserKeypressGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType);
> > 
> > We definitely need tp keep UserGestureIndicator around because that's what we use to gate other features.
> 
> UserKeypressGestureIndicator is a subclass of UserGestureIndicator that
> avoids triggering the
> "ResourceLoadObserver::logUserInteractionWIthReducedTimeResolution" until we
> confirm the key event was handled by the page. Otherwise, we don't update
> the statistics to credit the site with user interaction.
> 
> There are no other changes in behavior, and since only the
> ResourceLoadStatistics logic is modified, I don't think there can be any
> kind of compatibility issue.

I'm saying that we have to instantiate both UserGestureIndicator and UserKeypressGestureIndicator because UserGestureIndicator is used to guard other features.

[Brent Fulgham]

Created attachment 323734 [details]
Patch

[Brent Fulgham]

Comment on attachment 323626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323626&action=review

>>>> Source/WebCore/page/EventHandler.cpp:3199
>>>> +    UserKeypressGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType);
>>> 
>>> We definitely need tp keep UserGestureIndicator around because that's what we use to gate other features.
>> 
>> UserKeypressGestureIndicator is a subclass of UserGestureIndicator that avoids triggering the "ResourceLoadObserver::logUserInteractionWIthReducedTimeResolution" until we confirm the key event was handled by the page. Otherwise, we don't update the statistics to credit the site with user interaction.
>> 
>> There are no other changes in behavior, and since only the ResourceLoadStatistics logic is modified, I don't think there can be any kind of compatibility issue.
> 
> I'm saying that we have to instantiate both UserGestureIndicator and UserKeypressGestureIndicator because UserGestureIndicator is used to guard other features.

We can't instantiate UserGestureIndicator here, because it logs the user interaction upon construction, which I want to avoid. Instantiating both would not solve my problem.

I've revised the patch to just use a constructor argument so I can tell UserGestureIndicator that I want this ResourceLoadStatistics interaction credit to be delayed until we see that the event was handled.

[Brent Fulgham]

Created attachment 323741 [details]
Patch

[Brent Fulgham]

Committed r223307: <https://trac.webkit.org/changeset/223307>

[Ryan Haddad]

One of the LayoutTests for this change is a flaky timeout:
http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r223418%20(5048)/results.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fprevalent-resource-handled-keydown.html