Skip to content
Permalink
Browse files
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.

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.


Canonical link: https://commits.webkit.org/194514@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223307 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
brentfulgham committed Oct 13, 2017
1 parent 04ae1ff commit f402e39594181aeba0d7a143c27e3833518cd76e
Showing 11 changed files with 204 additions and 8 deletions.
@@ -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.
@@ -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

@@ -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>
@@ -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

@@ -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>
@@ -0,0 +1,9 @@
<html>
<body>
<table><tr>
<td id='foo'>
<a onclick="return 'bar'" href="#">baz</a>
</td>
</tr></table>
</body>
</html>
@@ -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 ]
@@ -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
@@ -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);
}
}
@@ -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 @@ class UserGestureIndicator {
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();

@@ -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())

0 comments on commit f402e39

Please sign in to comment.