Bug 178183

Summary: CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: DOMAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, bfulgham, buildbot, cdumez, dbates, esprehn+autocc, joepeck, kangil.han, koivisto, rniwa, ryanhaddad, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=178472
Bug Depends on: 169995    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch rniwa: review+

Description Brent Fulgham 2017-10-11 12:43:22 PDT
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.
Comment 1 Brent Fulgham 2017-10-11 13:41:42 PDT
<rdar://problem/33327730>
Comment 2 Brent Fulgham 2017-10-11 13:49:24 PDT
Created attachment 323458 [details]
Patch
Comment 3 Build Bot 2017-10-11 14:58:29 PDT
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
Comment 4 Build Bot 2017-10-11 14:58:30 PDT
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
Comment 5 Build Bot 2017-10-11 15:10:20 PDT
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
Comment 6 Build Bot 2017-10-11 15:10:21 PDT
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
Comment 7 Ryosuke Niwa 2017-10-11 15:13:40 PDT
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.
Comment 8 Build Bot 2017-10-11 15:24:01 PDT
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
Comment 9 Build Bot 2017-10-11 15:24:03 PDT
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
Comment 10 Build Bot 2017-10-11 15:29:06 PDT
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
Comment 11 Build Bot 2017-10-11 15:29:07 PDT
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
Comment 12 Brent Fulgham 2017-10-11 16:24:04 PDT
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)'.
Comment 13 Brent Fulgham 2017-10-12 12:20:54 PDT
Created attachment 323545 [details]
Patch
Comment 14 Ryosuke Niwa 2017-10-12 14:01:02 PDT
(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?
Comment 15 Brent Fulgham 2017-10-12 20:40:24 PDT
Created attachment 323626 [details]
Patch
Comment 16 Ryosuke Niwa 2017-10-13 10:32:32 PDT
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.
Comment 17 Brent Fulgham 2017-10-13 12:29:12 PDT
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.
Comment 18 Ryosuke Niwa 2017-10-13 13:00:59 PDT
(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.
Comment 19 Brent Fulgham 2017-10-13 13:25:25 PDT
Created attachment 323734 [details]
Patch
Comment 20 Brent Fulgham 2017-10-13 13:28:40 PDT
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.
Comment 21 Brent Fulgham 2017-10-13 13:55:52 PDT
Created attachment 323741 [details]
Patch
Comment 22 Brent Fulgham 2017-10-13 16:28:03 PDT
Committed r223307: <https://trac.webkit.org/changeset/223307>