Changeset 215404 in webkit

Timestamp:

Apr 16, 2017 5:56:47 PM (7 years ago)

Author:

Chris Dumez

Message:

CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown
​https://bugs.webkit.org/show_bug.cgi?id=169995
<rdar://problem/23798897>

Reviewed by Sam Weinig.

Source/WebCore:

Any key event was considered as user interaction with the page, which meant that they
would allow beforeunload alerts to be shown even when they do not represent actual
user interaction (e.g CMD+R / CMD+Q / CMD+T keyboard shortcuts).

To address the issue, we now only treat as user interaction with the page key events
that are actually handled by the page (i.e. handled by JS, typed into a field, ...).

Tests: fast/events/beforeunload-alert-handled-keydown.html

fast/events/beforeunload-alert-unhandled-keydown.html

• dom/Document.h:

(WebCore::Document::setUserDidInteractWithPage):
(WebCore::Document::userDidInteractWithPage):

• dom/UserGestureIndicator.cpp:

(WebCore::UserGestureIndicator::UserGestureIndicator):

• loader/FrameLoader.cpp:

(WebCore::shouldAskForNavigationConfirmation):

• page/EventHandler.cpp:

(WebCore::EventHandler::keyEvent):
(WebCore::EventHandler::internalKeyEvent):

• page/EventHandler.h:

LayoutTests:

Add layout test coverage.

• fast/events/beforeunload-alert-handled-keydown-expected.txt: Added.
• fast/events/beforeunload-alert-handled-keydown.html: Added.
• fast/events/beforeunload-alert-unhandled-keydown-expected.txt: Added.
• fast/events/beforeunload-alert-unhandled-keydown.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/events/beforeunload-alert-handled-keydown-expected.txt (added)
• LayoutTests/fast/events/beforeunload-alert-handled-keydown.html (added)
• LayoutTests/fast/events/beforeunload-alert-unhandled-keydown-expected.txt (added)
• LayoutTests/fast/events/beforeunload-alert-unhandled-keydown.html (added)
• LayoutTests/platform/ios/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.h (modified) (2 diffs)
• Source/WebCore/dom/UserGestureIndicator.cpp (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (1 diff)
• Source/WebCore/page/EventHandler.cpp (modified) (1 diff)
• Source/WebCore/page/EventHandler.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-04-16  Chris Dumez  <cdumez@apple.com>
+
+        CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown
+        https://bugs.webkit.org/show_bug.cgi?id=169995
+        <rdar://problem/23798897>
+
+        Reviewed by Sam Weinig.
+
+        Add layout test coverage.
+
+        * fast/events/beforeunload-alert-handled-keydown-expected.txt: Added.
+        * fast/events/beforeunload-alert-handled-keydown.html: Added.
+        * fast/events/beforeunload-alert-unhandled-keydown-expected.txt: Added.
+        * fast/events/beforeunload-alert-unhandled-keydown.html: Added.
+
 2017-04-16  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/LayoutTests/platform/ios/TestExpectations

@@ -366 +366 @@
 # This test relies on EventSender.keydown(), which is not supported on iOS
 webkit.org/b/155233 fast/events/max-tabindex-focus.html [ Skip ]
+fast/events/beforeunload-alert-handled-keydown.html [ Skip ]
+fast/events/beforeunload-alert-unhandled-keydown.html [ Skip ]
 fast/events/beforeunload-alert-user-interaction.html [ Skip ]
 fast/forms/validation-bubble-escape-key-dismiss.html [ Skip ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-04-16  Chris Dumez  <cdumez@apple.com>
+
+        CMD+R / CMD+Q is considered as user interaction and beforeunload alert is shown
+        https://bugs.webkit.org/show_bug.cgi?id=169995
+        <rdar://problem/23798897>
+
+        Reviewed by Sam Weinig.
+
+        Any key event was considered as user interaction with the page, which meant that they
+        would allow beforeunload alerts to be shown even when they do not represent actual
+        user interaction (e.g CMD+R / CMD+Q / CMD+T keyboard shortcuts).
+
+        To address the issue, we now only treat as user interaction with the page key events
+        that are actually handled by the page (i.e. handled by JS, typed into a field, ...).
+
+        Tests: fast/events/beforeunload-alert-handled-keydown.html
+               fast/events/beforeunload-alert-unhandled-keydown.html
+
+        * dom/Document.h:
+        (WebCore::Document::setUserDidInteractWithPage):
+        (WebCore::Document::userDidInteractWithPage):
+        * dom/UserGestureIndicator.cpp:
+        (WebCore::UserGestureIndicator::UserGestureIndicator):
+        * loader/FrameLoader.cpp:
+        (WebCore::shouldAskForNavigationConfirmation):
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::keyEvent):
+        (WebCore::EventHandler::internalKeyEvent):
+        * page/EventHandler.h:
+
 2017-04-16  Sam Weinig  <sam@webkit.org>
 

trunk/Source/WebCore/dom/Document.h

@@ -1148 +1148 @@
     void updateLastHandledUserGestureTimestamp(MonotonicTime);
 
+    void setUserDidInteractWithPage(bool userDidInteractWithPage) { ASSERT(&topDocument() == this); m_userDidInteractWithPage = userDidInteractWithPage; }
+    bool userDidInteractWithPage() const { ASSERT(&topDocument() == this); return m_userDidInteractWithPage; }
+
     // Used for testing. Count handlers in the main document, and one per frame which contains handlers.
     WEBCORE_EXPORT unsigned wheelEventHandlerCount() const;
@@ -1735 +1738 @@
 
     bool m_areDeviceMotionAndOrientationUpdatesSuspended { false };
+    bool m_userDidInteractWithPage { false };
 
 #if ENABLE(TELEPHONE_NUMBER_DETECTION)

trunk/Source/WebCore/dom/UserGestureIndicator.cpp

@@ -59 +59 @@
         document->updateLastHandledUserGestureTimestamp(MonotonicTime::now());
         ResourceLoadObserver::sharedObserver().logUserInteractionWithReducedTimeResolution(*document);
+        document->topDocument().setUserDidInteractWithPage(true);
     }
 }

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -3037 +3037 @@
 static bool shouldAskForNavigationConfirmation(Document& document, const BeforeUnloadEvent& event)
 {
-    bool userDidInteractWithPage = document.topDocument().hasHadUserInteraction();
+    bool userDidInteractWithPage = document.topDocument().userDidInteractWithPage();
     // Web pages can request we ask for confirmation before navigating by:
     // - Cancelling the BeforeUnloadEvent (modern way)

trunk/Source/WebCore/page/EventHandler.cpp

@@ -3094 +3094 @@
 #endif
 
-bool EventHandler::keyEvent(const PlatformKeyboardEvent& initialKeyEvent)
+bool EventHandler::keyEvent(const PlatformKeyboardEvent& keyEvent)
+{
+    Document* topDocument = m_frame.document() ? &m_frame.document()->topDocument() : nullptr;
+    bool savedUserDidInteractWithPage = topDocument ? topDocument->userDidInteractWithPage() : false;
+    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);
+
+    return wasHandled;
+}
+
+bool EventHandler::internalKeyEvent(const PlatformKeyboardEvent& initialKeyEvent)
 {
     Ref<Frame> protectedFrame(m_frame);

trunk/Source/WebCore/page/EventHandler.h

@@ -348 +348 @@
     WEBCORE_EXPORT bool handleMouseReleaseEvent(const MouseEventWithHitTestResults&);
 
+    bool internalKeyEvent(const PlatformKeyboardEvent&);
+
     std::optional<Cursor> selectCursor(const HitTestResult&, bool shiftKey);
     void updateCursor(FrameView&, const HitTestResult&, bool shiftKey);