Changeset 287860 in webkit

Timestamp:

Jan 10, 2022, 2:58:49 PM (5 years ago)

Author:

Wenson Hsieh

Message:

Modal container observer should classify elements that are styled like clickable controls
​https://bugs.webkit.org/show_bug.cgi?id=235022

Reviewed by Tim Horton.

Source/WebCore:

Broaden the criteria when considering whether or not an element inside of a detected modal container is a
"clickable control". In the case where there are event listeners on the modal container, an element inside of
the modal container that has cursor: pointer; may trigger an action on the modal container when clicked, even
if it does not have event listeners itself. Handle this scenario by considering the element to be a "clickable
control", and extract text from the element for the purposes of control classification.

Test: ModalContainerObservation.DetectControlsWithEventListenersOnModalContainer

• page/ModalContainerObserver.cpp:

(WebCore::listensToUserActivation):

Factor out this logic into a separate helper function.

(WebCore::isClickableControl):
(WebCore::ModalContainerObserver::collectClickableElements):

Tools:

Add a new API test to exercise the change, and adjust the test harness to allow tests using the harness to
additionally add an event listener on the modal container.

• TestWebKitAPI/Tests/WebKit/modal-container-custom.html:
• TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm:

(TestWebKitAPI::TEST):

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/ModalContainerObserver.cpp (modified) (4 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKit/modal-container-custom.html (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Modal container observer should classify elements that are styled like clickable controls
+        https://bugs.webkit.org/show_bug.cgi?id=235022
+
+        Reviewed by Tim Horton.
+
+        Broaden the criteria when considering whether or not an element inside of a detected modal container is a
+        "clickable control". In the case where there are event listeners on the modal container, an element inside of
+        the modal container that has `cursor: pointer;` may trigger an action on the modal container when clicked, even
+        if it does not have event listeners itself. Handle this scenario by considering the element to be a "clickable
+        control", and extract text from the element for the purposes of control classification.
+
+        Test: ModalContainerObservation.DetectControlsWithEventListenersOnModalContainer
+
+        * page/ModalContainerObserver.cpp:
+        (WebCore::listensToUserActivation):
+
+        Factor out this logic into a separate helper function.
+
+        (WebCore::isClickableControl):
+        (WebCore::ModalContainerObserver::collectClickableElements):
+
 2022-01-10  Eric Carlson  <eric.carlson@apple.com>
 

trunk/Source/WebCore/page/ModalContainerObserver.cpp

@@ -60 +60 @@
 
 static constexpr size_t maxLengthForClickableElementText = 100;
+static constexpr double maxWidthForElementsThatLookClickable = 200;
+static constexpr double maxHeightForElementsThatLookClickable = 100;
 
 bool ModalContainerObserver::isNeededFor(const Document& document)
@@ -296 +298 @@
 }
 
-static bool isClickableControl(const HTMLElement& element)
+static bool listensForUserActivation(const Element& element)
+{
+    return element.hasEventListeners(eventNames().clickEvent) || element.hasEventListeners(eventNames().mousedownEvent) || element.hasEventListeners(eventNames().mouseupEvent)
+        || element.hasEventListeners(eventNames().touchstartEvent) || element.hasEventListeners(eventNames().touchendEvent)
+        || element.hasEventListeners(eventNames().pointerdownEvent) || element.hasEventListeners(eventNames().pointerupEvent);
+}
+
+enum class ContainerListensForUserActivation : bool { No, Yes };
+static bool isClickableControl(const HTMLElement& element, ContainerListensForUserActivation containerListensForUserActivation)
 {
     if (element.isActuallyDisabled())
@@ -327 +337 @@
     }
 
-    return element.hasEventListeners(eventNames().clickEvent) || element.hasEventListeners(eventNames().mousedownEvent) || element.hasEventListeners(eventNames().mouseupEvent)
-        || element.hasEventListeners(eventNames().touchstartEvent) || element.hasEventListeners(eventNames().touchendEvent)
-        || element.hasEventListeners(eventNames().pointerdownEvent) || element.hasEventListeners(eventNames().pointerupEvent);
+    if (listensForUserActivation(element))
+        return true;
+
+    if (containerListensForUserActivation == ContainerListensForUserActivation::No)
+        return false;
+
+    auto rendererAndRect = element.boundingAbsoluteRectWithoutLayout();
+    if (!rendererAndRect)
+        return false;
+
+    auto [renderer, rect] = *rendererAndRect;
+    if (!renderer || rect.isEmpty())
+        return false;
+
+    // If the modal container itself has event listeners for user activation, continue looking for elements that look like
+    // clickable elements (e.g. small nodes with pointer-style cursor).
+    if (renderer->style().cursor() == CursorType::Pointer) {
+        if (rect.width() <= maxWidthForElementsThatLookClickable && rect.height() <= maxHeightForElementsThatLookClickable)
+            return true;
+    }
+
+    return false;
 }
 
@@ -710 +739 @@
         return { };
 
+    auto containerListensForUserActivation = listensForUserActivation(*containerForControls) ? ContainerListensForUserActivation::Yes : ContainerListensForUserActivation::No;
     Vector<Ref<HTMLElement>> clickableControls;
     for (auto& child : descendantsOfType<HTMLElement>(*containerForControls)) {
-        if (isClickableControl(child))
+        if (isClickableControl(child, containerListensForUserActivation))
             clickableControls.append(child);
     }

trunk/Tools/ChangeLog

@@ +1 @@
+2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Modal container observer should classify elements that are styled like clickable controls
+        https://bugs.webkit.org/show_bug.cgi?id=235022
+
+        Reviewed by Tim Horton.
+
+        Add a new API test to exercise the change, and adjust the test harness to allow tests using the harness to
+        additionally add an event listener on the modal container.
+
+        * TestWebKitAPI/Tests/WebKit/modal-container-custom.html:
+        * TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm:
+        (TestWebKitAPI::TEST):
+
 2022-01-10  Jonathan Bedard  <jbedard@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKit/modal-container-custom.html

@@ -30 +30 @@
     fixedContainer.style.display = "block";
 }
+
+function showWithEventListener(markup, eventType, callback) {
+    const fixedContainer = document.getElementById("fixed");
+    fixedContainer.addEventListener(eventType, callback);
+    fixedContainer.innerHTML = markup;
+    fixedContainer.style.display = "block";
+}
 </script>
 </head>

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm

@@ -329 +329 @@
 }
 
+TEST(ModalContainerObservation, DetectControlsWithEventListenersOnModalContainer)
+{
+    auto webView = createModalContainerWebView();
+    [webView loadBundlePage:@"modal-container-custom"];
+    auto script = @"showWithEventListener(`<div>Hello world <span style='cursor: pointer;'>yes</span></div>`, 'click', () => window.testPassed = true)";
+    [webView evaluate:script andDecidePolicy:_WKModalContainerDecisionHideAndAllow];
+    [webView waitForNextPresentationUpdate];
+    EXPECT_FALSE([[webView contentsAsString] containsString:@"Hello world"]);
+    EXPECT_EQ([webView lastModalContainerInfo].availableTypes, _WKModalContainerControlTypePositive);
+    EXPECT_TRUE([[webView objectByEvaluatingJavaScript:@"window.testPassed"] boolValue]);
+}
+
 } // namespace TestWebKitAPI