- Revision
- 287505
- Author
- wenson_hs...@apple.com
- Date
- 2022-01-01 15:06:26 -0800 (Sat, 01 Jan 2022)
Log Message
Modal containers are incorrectly detected in navigation elements and fixed document elements
https://bugs.webkit.org/show_bug.cgi?id=234669
rdar://87030613
Reviewed by Darin Adler.
Source/WebCore:
Avoid false positives when detecting modal containers in the following scenarios:
- Fixed-position document elements that contain the search term.
- Text that contains the search term inside fixed-position navigation elements.
Additionally, ensure that we unhide the current modal container in the event of a false positive where we find
no element that fulfills the criteria for being a classifiable control.
Tests: ModalContainerObservation.IgnoreFixedDocumentElement
ModalContainerObservation.IgnoreNavigationElements
ModalContainerObservation.ShowModalContainerAfterFalsePositive
* page/ModalContainerObserver.cpp:
(WebCore::accessibilityRole):
Move this static helper function farther up this source file, so that we can use it inside
`updateModalContainerIfNeeded()`.
(WebCore::isInsideNavigationElement):
(WebCore::ModalContainerObserver::updateModalContainerIfNeeded):
(WebCore::ModalContainerPolicyDecisionScope::ModalContainerPolicyDecisionScope):
(WebCore::ModalContainerPolicyDecisionScope::~ModalContainerPolicyDecisionScope):
(WebCore::ModalContainerPolicyDecisionScope::continueHidingModalContainerAfterScope):
(WebCore::ModalContainerPolicyDecisionScope::document const):
Add a RAII helper object to ensure that the modal container is revealed at the end of the modal container policy
decision scope, unless `continueHidingModalContainerAfterScope()` is invoked. Since this class contains a
`WeakPtr<Document>` already, we can replace the `WeakPtr<Document>` we're currently plumbing through each of the
async callbacks with only the `ModalContainerPolicyDecisionScope`, and just grab the document (or null if it was
destroyed) from the scope object.
This helper object allows us to avoid sprinkling ad-hoc calls to `revealModalContainer()` when exiting modal
container classification codepaths.
(WebCore::ModalContainerObserver::collectClickableElementsTimerFired):
* page/ModalContainerObserver.h:
Tools:
Add several new API tests to exercise the changes.
* TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm:
(-[ModalContainerWebView initWithFrame:configuration:]):
(-[ModalContainerWebView loadHTML:]):
(-[ModalContainerWebView _webView:decidePolicyForModalContainer:decisionHandler:]):
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (287504 => 287505)
--- trunk/Source/WebCore/ChangeLog 2022-01-01 22:54:52 UTC (rev 287504)
+++ trunk/Source/WebCore/ChangeLog 2022-01-01 23:06:26 UTC (rev 287505)
@@ -1,3 +1,47 @@
+2022-01-01 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Modal containers are incorrectly detected in navigation elements and fixed document elements
+ https://bugs.webkit.org/show_bug.cgi?id=234669
+ rdar://87030613
+
+ Reviewed by Darin Adler.
+
+ Avoid false positives when detecting modal containers in the following scenarios:
+ - Fixed-position document elements that contain the search term.
+ - Text that contains the search term inside fixed-position navigation elements.
+
+ Additionally, ensure that we unhide the current modal container in the event of a false positive where we find
+ no element that fulfills the criteria for being a classifiable control.
+
+ Tests: ModalContainerObservation.IgnoreFixedDocumentElement
+ ModalContainerObservation.IgnoreNavigationElements
+ ModalContainerObservation.ShowModalContainerAfterFalsePositive
+
+ * page/ModalContainerObserver.cpp:
+ (WebCore::accessibilityRole):
+
+ Move this static helper function farther up this source file, so that we can use it inside
+ `updateModalContainerIfNeeded()`.
+
+ (WebCore::isInsideNavigationElement):
+ (WebCore::ModalContainerObserver::updateModalContainerIfNeeded):
+ (WebCore::ModalContainerPolicyDecisionScope::ModalContainerPolicyDecisionScope):
+ (WebCore::ModalContainerPolicyDecisionScope::~ModalContainerPolicyDecisionScope):
+ (WebCore::ModalContainerPolicyDecisionScope::continueHidingModalContainerAfterScope):
+ (WebCore::ModalContainerPolicyDecisionScope::document const):
+
+ Add a RAII helper object to ensure that the modal container is revealed at the end of the modal container policy
+ decision scope, unless `continueHidingModalContainerAfterScope()` is invoked. Since this class contains a
+ `WeakPtr<Document>` already, we can replace the `WeakPtr<Document>` we're currently plumbing through each of the
+ async callbacks with only the `ModalContainerPolicyDecisionScope`, and just grab the document (or null if it was
+ destroyed) from the scope object.
+
+ This helper object allows us to avoid sprinkling ad-hoc calls to `revealModalContainer()` when exiting modal
+ container classification codepaths.
+
+ (WebCore::ModalContainerObserver::collectClickableElementsTimerFired):
+ * page/ModalContainerObserver.h:
+
2022-01-01 Alan Bujtas <za...@apple.com>
[LFC][IFC] Unexpected line break with leading collapsed whitespace
Modified: trunk/Source/WebCore/page/ModalContainerObserver.cpp (287504 => 287505)
--- trunk/Source/WebCore/page/ModalContainerObserver.cpp 2022-01-01 22:54:52 UTC (rev 287504)
+++ trunk/Source/WebCore/page/ModalContainerObserver.cpp 2022-01-01 23:06:26 UTC (rev 287505)
@@ -49,6 +49,7 @@
#include "RenderView.h"
#include "SimulatedClickOptions.h"
#include "Text.h"
+#include <wtf/Noncopyable.h>
#include <wtf/RobinHoodHashSet.h>
#include <wtf/Scope.h>
#include <wtf/URL.h>
@@ -130,6 +131,20 @@
return true;
}
+static AccessibilityRole accessibilityRole(const HTMLElement& element)
+{
+ return AccessibilityObject::ariaRoleToWebCoreRole(element.attributeWithoutSynchronization(HTMLNames::roleAttr));
+}
+
+static bool isInsideNavigationElement(const Text& textNode)
+{
+ for (auto& parent : ancestorsOfType<HTMLElement>(textNode)) {
+ if (parent.hasTagName(HTMLNames::navTag) || accessibilityRole(parent) == AccessibilityRole::LandmarkNavigation)
+ return true;
+ }
+ return false;
+}
+
void ModalContainerObserver::updateModalContainerIfNeeded(const FrameView& view)
{
if (m_container)
@@ -155,6 +170,9 @@
return;
for (auto& renderer : *view.viewportConstrainedObjects()) {
+ if (renderer.isDocumentElementRenderer())
+ continue;
+
RefPtr element = renderer.element();
if (!element || is<HTMLBodyElement>(*element) || element->isDocumentNode())
continue;
@@ -164,6 +182,9 @@
for (auto& textRenderer : descendantsOfType<RenderText>(renderer)) {
if (RefPtr textNode = textRenderer.textNode(); textNode && matchesSearchTerm(*textNode, searchTerm)) {
+ if (isInsideNavigationElement(*textNode))
+ break;
+
m_container = element.get();
element->invalidateStyle();
scheduleClickableElementCollection();
@@ -173,11 +194,6 @@
}
}
-static AccessibilityRole accessibilityRole(const HTMLElement& element)
-{
- return AccessibilityObject::ariaRoleToWebCoreRole(element.attributeWithoutSynchronization(HTMLNames::roleAttr));
-}
-
static bool isClickableControl(const HTMLElement& element)
{
if (element.isActuallyDisabled())
@@ -281,13 +297,41 @@
m_collectClickableElementsTimer.startOneShot(200_ms);
}
+class ModalContainerPolicyDecisionScope {
+ WTF_MAKE_NONCOPYABLE(ModalContainerPolicyDecisionScope);
+ WTF_MAKE_FAST_ALLOCATED;
+public:
+ ModalContainerPolicyDecisionScope(Document& document)
+ : m_document { document }
+ {
+ }
+
+ ModalContainerPolicyDecisionScope(ModalContainerPolicyDecisionScope&&) = default;
+
+ ~ModalContainerPolicyDecisionScope()
+ {
+ if (m_continueHidingModalContainerAfterScope || !m_document)
+ return;
+
+ if (auto observer = m_document->modalContainerObserverIfExists())
+ observer->revealModalContainer();
+ }
+
+ void continueHidingModalContainerAfterScope() { m_continueHidingModalContainerAfterScope = true; }
+ Document* document() const { return m_document.get(); }
+
+private:
+ WeakPtr<Document> m_document;
+ bool m_continueHidingModalContainerAfterScope { false };
+};
+
void ModalContainerObserver::collectClickableElementsTimerFired()
{
if (!m_container)
return;
- m_container->document().eventLoop().queueTask(TaskSource::InternalAsyncTask, [observer = this, weakDocument = WeakPtr { m_container->document() }]() mutable {
- RefPtr document = weakDocument.get();
+ m_container->document().eventLoop().queueTask(TaskSource::InternalAsyncTask, [observer = this, decisionScope = ModalContainerPolicyDecisionScope { m_container->document() }]() mutable {
+ RefPtr document = decisionScope.document();
if (!document)
return;
@@ -306,11 +350,8 @@
return;
}
- page->chrome().client().classifyModalContainerControls(WTFMove(controlTextsToClassify), [weakDocument = WTFMove(weakDocument), observer, controls = WTFMove(classifiableControls)] (auto&& types) mutable {
- if (types.size() != controls.size())
- return;
-
- RefPtr document = weakDocument.get();
+ page->chrome().client().classifyModalContainerControls(WTFMove(controlTextsToClassify), [decisionScope = WTFMove(decisionScope), observer, controls = WTFMove(classifiableControls)] (auto&& types) mutable {
+ RefPtr document = decisionScope.document();
if (!document)
return;
@@ -325,6 +366,9 @@
return;
}
+ if (types.size() != controls.size())
+ return;
+
struct ClassifiedControls {
Vector<WeakPtr<HTMLElement>> positive;
Vector<WeakPtr<HTMLElement>> neutral;
@@ -395,26 +439,26 @@
return;
auto clickableControlTypes = classifiedControls.types();
- if (clickableControlTypes.isEmpty()) {
- observer->revealModalContainer();
+ if (clickableControlTypes.isEmpty())
return;
- }
- page->chrome().client().decidePolicyForModalContainer(clickableControlTypes, [weakDocument = WTFMove(weakDocument), observer, classifiedControls = WTFMove(classifiedControls)](auto decision) mutable {
- RefPtr document = weakDocument.get();
+ page->chrome().client().decidePolicyForModalContainer(clickableControlTypes, [decisionScope = WTFMove(decisionScope), observer, classifiedControls = WTFMove(classifiedControls)](auto decision) mutable {
+ RefPtr document = decisionScope.document();
if (!document)
return;
- if (observer != document->modalContainerObserverIfExists())
+ if (observer != document->modalContainerObserverIfExists()) {
+ ASSERT_NOT_REACHED();
return;
+ }
- if (decision == ModalContainerDecision::Show) {
- observer->revealModalContainer();
+ if (decision == ModalContainerDecision::Show)
return;
- }
if (RefPtr controlToClick = classifiedControls.controlToClick(decision))
controlToClick->dispatchSimulatedClick(nullptr, SendMouseUpDownEvents, DoNotShowPressedLook);
+
+ decisionScope.continueHidingModalContainerAfterScope();
});
});
});
Modified: trunk/Source/WebCore/page/ModalContainerObserver.h (287504 => 287505)
--- trunk/Source/WebCore/page/ModalContainerObserver.h 2022-01-01 22:54:52 UTC (rev 287504)
+++ trunk/Source/WebCore/page/ModalContainerObserver.h 2022-01-01 23:06:26 UTC (rev 287505)
@@ -54,6 +54,8 @@
inline void overrideSearchTermForTesting(const String&);
private:
+ friend class ModalContainerPolicyDecisionScope;
+
void scheduleClickableElementCollection();
void collectClickableElementsTimerFired();
void revealModalContainer();
Modified: trunk/Tools/ChangeLog (287504 => 287505)
--- trunk/Tools/ChangeLog 2022-01-01 22:54:52 UTC (rev 287504)
+++ trunk/Tools/ChangeLog 2022-01-01 23:06:26 UTC (rev 287505)
@@ -1,5 +1,21 @@
2022-01-01 Wenson Hsieh <wenson_hs...@apple.com>
+ Modal containers are incorrectly detected in navigation elements and fixed document elements
+ https://bugs.webkit.org/show_bug.cgi?id=234669
+ rdar://87030613
+
+ Reviewed by Darin Adler.
+
+ Add several new API tests to exercise the changes.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm:
+ (-[ModalContainerWebView initWithFrame:configuration:]):
+ (-[ModalContainerWebView loadHTML:]):
+ (-[ModalContainerWebView _webView:decidePolicyForModalContainer:decisionHandler:]):
+ (TestWebKitAPI::TEST):
+
+2022-01-01 Wenson Hsieh <wenson_hs...@apple.com>
+
[Cocoa] Simplify some FontAttributes API tests
https://bugs.webkit.org/show_bug.cgi?id=234770
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm (287504 => 287505)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm 2022-01-01 22:54:52 UTC (rev 287504)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm 2022-01-01 23:06:26 UTC (rev 287505)
@@ -24,6 +24,7 @@
*/
#import "config.h"
+#import "Test.h"
#import "PlatformUtilities.h"
#import "TestProtocol.h"
@@ -86,6 +87,7 @@
} // namespace TestWebKitAPI
@interface ModalContainerWebView : TestWKWebView <WKUIDelegatePrivate>
+@property (nonatomic) _WKModalContainerDecision decision;
@property (nonatomic, readonly) _WKModalContainerInfo *lastModalContainerInfo;
@end
@@ -93,7 +95,6 @@
std::unique_ptr<TestWebKitAPI::ClassifierModelSwizzler> _classifierModelSwizzler;
RetainPtr<TestNavigationDelegate> _navigationDelegate;
RetainPtr<_WKModalContainerInfo> _lastModalContainerInfo;
- std::optional<_WKModalContainerDecision> _decision;
bool _doneWaitingForDecisionHandler;
}
@@ -107,6 +108,7 @@
[TestProtocol registerWithScheme:@"http"];
});
+ _decision = _WKModalContainerDecisionShow;
_classifierModelSwizzler = makeUnique<TestWebKitAPI::ClassifierModelSwizzler>();
_navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
_doneWaitingForDecisionHandler = true;
@@ -148,9 +150,26 @@
[_navigationDelegate waitForDidFinishNavigationWithPreferences:preferences.get()];
}
+- (void)loadHTML:(NSString *)html
+{
+ NSURLRequest *fakeRequest = [NSURLRequest requestWithURL:[NSURL URLWithString:@"http://webkit.org"]];
+ [self loadSimulatedRequest:fakeRequest responseHTMLString:html];
+
+ auto preferences = adoptNS([[WKWebpagePreferences alloc] init]);
+ [preferences _setModalContainerObservationPolicy:_WKWebsiteModalContainerObservationPolicyPrompt];
+ [_navigationDelegate waitForDidFinishNavigationWithPreferences:preferences.get()];
+}
+
+- (void)waitForText:(NSString *)text
+{
+ TestWebKitAPI::Util::waitForConditionWithLogging([&]() -> bool {
+ return [self.contentsAsString containsString:text];
+ }, 3, @"Timed out waiting for '%@'", text);
+}
+
- (void)_webView:(WKWebView *)webView decidePolicyForModalContainer:(_WKModalContainerInfo *)containerInfo decisionHandler:(void (^)(_WKModalContainerDecision))handler
{
- handler(_decision.value_or(_WKModalContainerDecisionShow));
+ handler(_decision);
_lastModalContainerInfo = containerInfo;
_doneWaitingForDecisionHandler = true;
}
@@ -232,4 +251,41 @@
EXPECT_EQ([webView lastModalContainerInfo].availableTypes, _WKModalContainerControlTypePositive | _WKModalContainerControlTypeNegative);
}
+TEST(ModalContainerObservation, IgnoreFixedDocumentElement)
+{
+ auto webView = createModalContainerWebView();
+ [webView setDecision:_WKModalContainerDecisionHideAndIgnore];
+ [webView loadHTML:@("<head>"
+ "<script>internals.overrideModalContainerSearchTermForTesting('foo')</script>"
+ "<style>html { position: fixed; width: 100%; height: 100%; top: 0; left: 0; }</style>"
+ "</head><body><h1>foo bar</h1></body>")];
+
+ EXPECT_TRUE([[webView contentsAsString] containsString:@"foo bar"]);
+ EXPECT_NULL([webView lastModalContainerInfo]);
+}
+
+TEST(ModalContainerObservation, IgnoreNavigationElements)
+{
+ auto webView = createModalContainerWebView();
+ [webView setDecision:_WKModalContainerDecisionHideAndIgnore];
+ [webView loadBundlePage:@"modal-container-custom"];
+ [webView objectByEvaluatingJavaScript:@"show(`<nav>hello world 1</nav><div role='navigation'>hello world 2</div>`)"];
+ [webView waitForNextPresentationUpdate];
+
+ EXPECT_TRUE([[webView contentsAsString] containsString:@"hello world 1"]);
+ EXPECT_TRUE([[webView contentsAsString] containsString:@"hello world 2"]);
+ EXPECT_NULL([webView lastModalContainerInfo]);
+}
+
+TEST(ModalContainerObservation, ShowModalContainerAfterFalsePositive)
+{
+ auto webView = createModalContainerWebView();
+ [webView setDecision:_WKModalContainerDecisionHideAndIgnore];
+ [webView loadBundlePage:@"modal-container-custom"];
+ [webView objectByEvaluatingJavaScript:@"show(`<div>hello world</div><a href=''>Click here</a>`)"];
+ [webView waitForNextPresentationUpdate];
+ [webView waitForText:@"hello world"];
+ EXPECT_NULL([webView lastModalContainerInfo]);
+}
+
} // namespace TestWebKitAPI
