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.
<rdar://problem/33327730>
Created attachment 323458 [details] Patch
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
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 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
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 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 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
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 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
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 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)'.
Created attachment 323545 [details] Patch
(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?
Created attachment 323626 [details] Patch
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 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.
(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.
Created attachment 323734 [details] Patch
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.
Created attachment 323741 [details] Patch
Committed r223307: <https://trac.webkit.org/changeset/223307>
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