WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
178183
CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page
https://bugs.webkit.org/show_bug.cgi?id=178183
Summary
CMD+R / CMD+Q keyboard shortcuts are treated as user interaction with page
Brent Fulgham
Reported
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.
Attachments
Patch
(12.79 KB, patch)
2017-10-11 13:49 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews101 for mac-elcapitan
(1.01 MB, application/zip)
2017-10-11 14:58 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
(1.14 MB, application/zip)
2017-10-11 15:10 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews114 for mac-elcapitan
(1.81 MB, application/zip)
2017-10-11 15:24 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews122 for ios-simulator-wk2
(958.54 KB, application/zip)
2017-10-11 15:29 PDT
,
Build Bot
no flags
Details
Patch
(13.99 KB, patch)
2017-10-12 12:20 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(16.46 KB, patch)
2017-10-12 20:40 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(14.84 KB, patch)
2017-10-13 13:25 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(14.88 KB, patch)
2017-10-13 13:55 PDT
,
Brent Fulgham
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Brent Fulgham
Comment 1
2017-10-11 13:41:42 PDT
<
rdar://problem/33327730
>
Brent Fulgham
Comment 2
2017-10-11 13:49:24 PDT
Created
attachment 323458
[details]
Patch
Build Bot
Comment 3
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
Build Bot
Comment 4
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
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Ryosuke Niwa
Comment 7
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.
Build Bot
Comment 8
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
Build Bot
Comment 9
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
Build Bot
Comment 10
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
Build Bot
Comment 11
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
Brent Fulgham
Comment 12
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)'.
Brent Fulgham
Comment 13
2017-10-12 12:20:54 PDT
Created
attachment 323545
[details]
Patch
Ryosuke Niwa
Comment 14
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?
Brent Fulgham
Comment 15
2017-10-12 20:40:24 PDT
Created
attachment 323626
[details]
Patch
Ryosuke Niwa
Comment 16
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.
Brent Fulgham
Comment 17
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.
Ryosuke Niwa
Comment 18
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.
Brent Fulgham
Comment 19
2017-10-13 13:25:25 PDT
Created
attachment 323734
[details]
Patch
Brent Fulgham
Comment 20
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.
Brent Fulgham
Comment 21
2017-10-13 13:55:52 PDT
Created
attachment 323741
[details]
Patch
Brent Fulgham
Comment 22
2017-10-13 16:28:03 PDT
Committed
r223307
: <
https://trac.webkit.org/changeset/223307
>
Ryan Haddad
Comment 23
2017-10-16 13:30:48 PDT
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug