CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Oct 2017 23:28:01 +0000 (23:28 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Oct 2017 23:28:01 +0000 (23:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178183
<rdar://problem/33327730>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Key events are granted user interaction credit (in terms of updating the last time of user
interaction), even if the key event was not handled. Instead, we should defer granting
access until the key event has been handled.

Add a new default constructor argument to UserGestureIndicator to be used when handling key
events, so we can delay a decision about whether to grant ResourceLoadStatistics
'hasHadUserInteraction' until we confirm that the event was handled by the page.

This change does not affect other aspects of user interaction.

Tests: fast/events
       http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html
       http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html

* dom/UserGestureIndicator.cpp:
(WebCore::UserGestureIndicator::UserGestureIndicator): Add check based on constructor argument.
Also: Drive by fix to avoid calling 'currentToken' when not on the main thread.
* dom/UserGestureIndicator.h:
* page/EventHandler.cpp:
(WebCore::EventHandler::keyEvent): If the key event was handled, grant user interaction credit
for ResourceLoadStatistics processing.
(WebCore::EventHandler::internalKeyEvent): Use the new UserGestureIndicator constructor argument.

LayoutTests:

* http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown-expected.txt: Added.
* http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html: Added.
* http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown-expected.txt: Added.
* http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html: Added.
* http/tests/resourceLoadStatistics/resources: Added.
* http/tests/resourceLoadStatistics/resources/onclick.html: Added.
* platform/ios/TestExpectations: Skip tests that require 'keyDown' support, since this is not
  available on iOS.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223307 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html [new file with mode: 0644]
LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html [new file with mode: 0644]
LayoutTests/http/tests/resourceLoadStatistics/resources/onclick.html [new file with mode: 0644]
LayoutTests/platform/ios/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/UserGestureIndicator.cpp
Source/WebCore/dom/UserGestureIndicator.h
Source/WebCore/page/EventHandler.cpp

index 7849177..f8f138e 100644 (file)
@@ -1,3 +1,20 @@
+2017-10-13  Brent Fulgham  <bfulgham@apple.com>
+
+        CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page
+        https://bugs.webkit.org/show_bug.cgi?id=178183
+        <rdar://problem/33327730>
+
+        Reviewed by Ryosuke Niwa.
+
+        * http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown-expected.txt: Added.
+        * http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html: Added.
+        * http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown-expected.txt: Added.
+        * http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html: Added.
+        * http/tests/resourceLoadStatistics/resources: Added.
+        * http/tests/resourceLoadStatistics/resources/onclick.html: Added.
+        * platform/ios/TestExpectations: Skip tests that require 'keyDown' support, since this is not
+          available on iOS.
+
 2017-10-13  Matt Lewis  <jlewis3@apple.com>
 
         Marked http/tests/inspector/network/resource-sizes-memory-cache.html as flaky.
diff --git a/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown-expected.txt b/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown-expected.txt
new file mode 100644 (file)
index 0000000..e4e32c5
--- /dev/null
@@ -0,0 +1,12 @@
+Tests that we grant User Interaction credit for handled keypresses.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Simulate user typing letter 'a' into the field.
+PASS testInput.value is "a"
+PASS Origin was granted user interaction.
+PASS successfullyParsed is true
+
+TEST COMPLETE
diff --git a/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html b/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html
new file mode 100644 (file)
index 0000000..f8e1e2e
--- /dev/null
@@ -0,0 +1,56 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description("Tests that we grant User Interaction credit for handled keypresses.");
+jsTestIsAsync = true;
+
+const statisticsUrl = "http://127.0.0.1:8000/temp";
+
+onload = function() {
+    const testFrame = document.getElementById("testFrame");
+
+    if (window.testRunner && window.internals) {
+        internals.setResourceLoadStatisticsEnabled(true);
+        testRunner.setStatisticsNotifyPagesWhenDataRecordsWereScanned(true);
+    }
+
+    testRunner.setStatisticsPrevalentResource(statisticsUrl, true);
+    if (!testRunner.isStatisticsPrevalentResource(statisticsUrl))
+        testFailed("Host did not get set as prevalent resource.");
+
+    testRunner.setStatisticsHasHadUserInteraction(statisticsUrl, false);
+    if (testRunner.isStatisticsHasHadUserInteraction(statisticsUrl))
+        testFailed("Host did not get cleared of user interaction.");
+
+    testInput = document.getElementById("testInput");
+
+    testRunner.installStatisticsDidModifyDataRecordsCallback(function() {
+        shouldBeEqualToString("testInput.value", "a");
+
+        if (!testRunner.isStatisticsHasHadUserInteraction(statisticsUrl))
+            testFailed("Origin did not get user interaction credit.");
+        else
+            testPassed("Origin was granted user interaction.");
+
+        setTimeout(function() {
+            testFrame.src = "about:blank";
+            setTimeout(finishJSTest, 0);
+        }, 0);
+    });
+    testRunner.setStatisticsShouldClassifyResourcesBeforeDataRecordsRemoval(false);
+    testRunner.setStatisticsMinimumTimeBetweenDataRecordsRemoval(0);
+    testRunner.statisticsProcessStatisticsAndDataRecords();
+
+    debug("Simulate user typing letter 'a' into the field.");
+    testInput.focus();
+    if (window.eventSender)
+        eventSender.keyDown("a");    
+}
+</script>
+<iframe id="testFrame" src="resources/onclick.html"></iframe>
+<input id="testInput" type="text"></input>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown-expected.txt b/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown-expected.txt
new file mode 100644 (file)
index 0000000..36e559b
--- /dev/null
@@ -0,0 +1,11 @@
+Tests that we do not grant User Interaction credit for unhandled keypress.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Simulate an unhandled user key press.
+PASS Origin did not get user interaction credit.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html b/LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html
new file mode 100644 (file)
index 0000000..8d61ef9
--- /dev/null
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description("Tests that we do not grant User Interaction credit for unhandled keypress.");
+jsTestIsAsync = true;
+
+const statisticsUrl = "http://127.0.0.1:8000/temp";
+
+onload = function() {
+    const testFrame = document.getElementById("testFrame");
+
+    if (window.testRunner && window.internals) {
+        internals.setResourceLoadStatisticsEnabled(true);
+        testRunner.setStatisticsNotifyPagesWhenDataRecordsWereScanned(true);
+    }
+
+    testRunner.setStatisticsPrevalentResource(statisticsUrl, true);
+    if (!testRunner.isStatisticsPrevalentResource(statisticsUrl))
+        testFailed("Host did not get set as prevalent resource.");
+
+    testRunner.setStatisticsHasHadUserInteraction(statisticsUrl, false);
+    if (testRunner.isStatisticsHasHadUserInteraction(statisticsUrl))
+        testFailed("Host did not get cleared of user interaction.");
+
+    testRunner.installStatisticsDidModifyDataRecordsCallback(function() {
+
+        if (!testRunner.isStatisticsHasHadUserInteraction(statisticsUrl))
+            testPassed("Origin did not get user interaction credit.");
+
+        setTimeout(function() {
+            testFrame.src = "about:blank";
+            setTimeout(finishJSTest, 0);
+        }, 0);
+    });
+    testRunner.setStatisticsShouldClassifyResourcesBeforeDataRecordsRemoval(false);
+    testRunner.setStatisticsMinimumTimeBetweenDataRecordsRemoval(0);
+    testRunner.statisticsProcessStatisticsAndDataRecords();
+
+    debug("Simulate an unhandled user key press.");
+    if (window.eventSender)
+        eventSender.keyDown("a", ["metaKey"]);    
+}
+</script>
+<iframe id="testFrame" src="resources/onclick.html"></iframe>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/resourceLoadStatistics/resources/onclick.html b/LayoutTests/http/tests/resourceLoadStatistics/resources/onclick.html
new file mode 100644 (file)
index 0000000..3190fcd
--- /dev/null
@@ -0,0 +1,9 @@
+<html>
+<body>
+<table><tr>
+<td id='foo'>
+<a onclick="return 'bar'" href="#">baz</a>
+</td>
+</tr></table>
+</body>
+</html>
index 5332b63..2d96d35 100644 (file)
@@ -378,6 +378,8 @@ fast/events/keyboardevent-key.html [ Skip ]
 fast/events/keyboardevent-code.html [ Skip ]
 http/tests/navigation/keyboard-events-during-provisional-navigation.html [ Skip ]
 http/tests/navigation/keyboard-events-during-provisional-subframe-navigation.html [ Skip ]
+http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html [ Skip ]
+http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html [ Skip ]
 media/audio-dealloc-crash.html [ Skip ]
 media/restricted-audio-playback-with-document-gesture.html [ Skip ]
 media/restricted-audio-playback-with-multiple-settimeouts.html [ Skip ]
index 3b7821d..59d6ac5 100644 (file)
@@ -1,3 +1,34 @@
+2017-10-13  Brent Fulgham  <bfulgham@apple.com>
+
+        CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page
+        https://bugs.webkit.org/show_bug.cgi?id=178183
+        <rdar://problem/33327730>
+
+        Reviewed by Ryosuke Niwa.
+
+        Key events are granted user interaction credit (in terms of updating the last time of user
+        interaction), even if the key event was not handled. Instead, we should defer granting
+        access until the key event has been handled.
+        
+        Add a new default constructor argument to UserGestureIndicator to be used when handling key
+        events, so we can delay a decision about whether to grant ResourceLoadStatistics
+        'hasHadUserInteraction' until we confirm that the event was handled by the page.
+
+        This change does not affect other aspects of user interaction.
+
+        Tests: fast/events
+               http/tests/resourceLoadStatistics/prevalent-resource-handled-keydown.html
+               http/tests/resourceLoadStatistics/prevalent-resource-unhandled-keydown.html
+
+        * dom/UserGestureIndicator.cpp:
+        (WebCore::UserGestureIndicator::UserGestureIndicator): Add check based on constructor argument.
+        Also: Drive by fix to avoid calling 'currentToken' when not on the main thread.
+        * dom/UserGestureIndicator.h:
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::keyEvent): If the key event was handled, grant user interaction credit
+        for ResourceLoadStatistics processing.
+        (WebCore::EventHandler::internalKeyEvent): Use the new UserGestureIndicator constructor argument.
+
 2017-10-13  Chris Dumez  <cdumez@apple.com>
 
         DOMTokenList shouldn't add empty attributes
index 4a5940d..7ae874c 100644 (file)
@@ -46,19 +46,22 @@ UserGestureToken::~UserGestureToken()
         observer(*this);
 }
 
-UserGestureIndicator::UserGestureIndicator(std::optional<ProcessingUserGestureState> state, Document* document, UserGestureType gestureType)
-    : m_previousToken(currentToken())
+UserGestureIndicator::UserGestureIndicator(std::optional<ProcessingUserGestureState> state, Document* document, UserGestureType gestureType, ProcessInteractionStyle processInteractionStyle)
 {
     // Silently ignore UserGestureIndicators on non main threads.
     if (!isMainThread())
         return;
 
+    // It is only safe to use currentToken() on the main thread.
+    m_previousToken = currentToken();
+
     if (state)
         currentToken() = UserGestureToken::create(state.value(), gestureType);
 
     if (document && currentToken()->processingUserGesture()) {
         document->updateLastHandledUserGestureTimestamp(MonotonicTime::now());
-        ResourceLoadObserver::shared().logUserInteractionWithReducedTimeResolution(document->topDocument());
+        if (processInteractionStyle == ProcessInteractionStyle::Immediate)
+            ResourceLoadObserver::shared().logUserInteractionWithReducedTimeResolution(document->topDocument());
         document->topDocument().setUserDidInteractWithPage(true);
     }
 }
index 98bc0f0..cf8e393 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -84,7 +84,8 @@ public:
     WEBCORE_EXPORT static bool processingUserGestureForMedia();
 
     // If a document is provided, its last known user gesture timestamp is updated.
-    WEBCORE_EXPORT explicit UserGestureIndicator(std::optional<ProcessingUserGestureState>, Document* = nullptr, UserGestureType = UserGestureType::Other);
+    enum class ProcessInteractionStyle { Immediate, Delayed };
+    WEBCORE_EXPORT explicit UserGestureIndicator(std::optional<ProcessingUserGestureState>, Document* = nullptr, UserGestureType = UserGestureType::Other, ProcessInteractionStyle = ProcessInteractionStyle::Immediate);
     WEBCORE_EXPORT explicit UserGestureIndicator(RefPtr<UserGestureToken>);
     WEBCORE_EXPORT ~UserGestureIndicator();
 
index 202f60b..999f1d1 100644 (file)
@@ -78,6 +78,7 @@
 #include "RenderTextControlSingleLine.h"
 #include "RenderView.h"
 #include "RenderWidget.h"
+#include "ResourceLoadObserver.h"
 #include "RuntimeApplicationChecks.h"
 #include "SVGDocument.h"
 #include "SVGNames.h"
@@ -3125,8 +3126,12 @@ bool EventHandler::keyEvent(const PlatformKeyboardEvent& keyEvent)
     bool wasHandled = internalKeyEvent(keyEvent);
 
     // If the key event was not handled, do not treat it as user interaction with the page.
-    if (topDocument && !wasHandled)
-        topDocument->setUserDidInteractWithPage(savedUserDidInteractWithPage);
+    if (topDocument) {
+        if (!wasHandled)
+            topDocument->setUserDidInteractWithPage(savedUserDidInteractWithPage);
+        else
+            ResourceLoadObserver::shared().logUserInteractionWithReducedTimeResolution(topDocument->topDocument());
+    }
 
     return wasHandled;
 }
@@ -3191,7 +3196,7 @@ bool EventHandler::internalKeyEvent(const PlatformKeyboardEvent& initialKeyEvent
     if (initialKeyEvent.windowsVirtualKeyCode() == VK_ESCAPE)
         gestureType = UserGestureType::EscapeKey;
 
-    UserGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType);
+    UserGestureIndicator gestureIndicator(ProcessingUserGesture, m_frame.document(), gestureType, UserGestureIndicator::ProcessInteractionStyle::Delayed);
     UserTypingGestureIndicator typingGestureIndicator(m_frame);
 
     if (FrameView* view = m_frame.view())