- Revision
- 287777
- Author
- wenson_hs...@apple.com
- Date
- 2022-01-07 12:28:04 -0800 (Fri, 07 Jan 2022)
Log Message
Teach modal container observer to make the body element scrollable if necessary
https://bugs.webkit.org/show_bug.cgi?id=234708
rdar://86960677
Reviewed by Tim Horton.
Source/WebCore:
Add a mechanism to allow ModalContainerObserver to force the body and/or document elements in the main document
to become vertically scrollable, in the case where a modal container has been detected.
In particular, if a modal container has already been detected and hidden away, the frame is non-scrollable, and
the body and/or document element satisfies both conditions:
1. Has a height that is taller than the visible height of the top FrameView
2. Has `overflow-y: hidden;`
...then we'll flag one or both of those elements and force them to be vertically scrollable during style
adjustment (i.e. return `true` from `shouldMakeVerticallyScrollable()`).
Covered by augmenting an existing API test:
ModalContainerObservation.HideUserInteractionBlockingElementAndMakeDocumentScrollable
* page/ModalContainerObserver.cpp:
(WebCore::ModalContainerObserver::setContainer):
(WebCore::ModalContainerObserver::collectClickableElementsTimerFired):
(WebCore::ModalContainerObserver::makeBodyAndDocumentElementScrollableIfNeeded):
Helper method that contains logic for overriding scrollability on the body or html element, if needed.
(WebCore::ModalContainerObserver::clearScrollabilityOverrides):
(WebCore::ModalContainerObserver::hideUserInteractionBlockingElementIfNeeded):
Drive-by fix: `target` is just a raw pointer here, so just assign it directly to `foundElement` instead of
trying to use move semantics.
(WebCore::ModalContainerObserver::revealModalContainer):
(WebCore::ModalContainerObserver::shouldMakeVerticallyScrollable const):
Add a helper method (similar to `shouldHide()`) that can be used to make elements vertically scrollable during
style adjustment time. See above for more details.
(WebCore::ModalContainerObserver::tryToMakeBodyAndDocumentElementScrollableThroughQuirks):
* page/ModalContainerObserver.h:
* style/StyleAdjuster.cpp:
(WebCore::Style::Adjuster::adjust const):
Consult `shouldMakeVerticallyScrollable` in addition to `shouldHide` if `ModalContainerObserver` is present.
Tools:
Adjust an existing API test to exercise the change.
* TestWebKitAPI/Tests/WebKit/modal-container-with-overlay.html:
* TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm:
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (287776 => 287777)
--- trunk/Source/WebCore/ChangeLog 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Source/WebCore/ChangeLog 2022-01-07 20:28:04 UTC (rev 287777)
@@ -1,3 +1,52 @@
+2022-01-07 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Teach modal container observer to make the body element scrollable if necessary
+ https://bugs.webkit.org/show_bug.cgi?id=234708
+ rdar://86960677
+
+ Reviewed by Tim Horton.
+
+ Add a mechanism to allow ModalContainerObserver to force the body and/or document elements in the main document
+ to become vertically scrollable, in the case where a modal container has been detected.
+
+ In particular, if a modal container has already been detected and hidden away, the frame is non-scrollable, and
+ the body and/or document element satisfies both conditions:
+
+ 1. Has a height that is taller than the visible height of the top FrameView
+ 2. Has `overflow-y: hidden;`
+
+ ...then we'll flag one or both of those elements and force them to be vertically scrollable during style
+ adjustment (i.e. return `true` from `shouldMakeVerticallyScrollable()`).
+
+ Covered by augmenting an existing API test:
+ ModalContainerObservation.HideUserInteractionBlockingElementAndMakeDocumentScrollable
+
+ * page/ModalContainerObserver.cpp:
+ (WebCore::ModalContainerObserver::setContainer):
+ (WebCore::ModalContainerObserver::collectClickableElementsTimerFired):
+ (WebCore::ModalContainerObserver::makeBodyAndDocumentElementScrollableIfNeeded):
+
+ Helper method that contains logic for overriding scrollability on the body or html element, if needed.
+
+ (WebCore::ModalContainerObserver::clearScrollabilityOverrides):
+ (WebCore::ModalContainerObserver::hideUserInteractionBlockingElementIfNeeded):
+
+ Drive-by fix: `target` is just a raw pointer here, so just assign it directly to `foundElement` instead of
+ trying to use move semantics.
+
+ (WebCore::ModalContainerObserver::revealModalContainer):
+ (WebCore::ModalContainerObserver::shouldMakeVerticallyScrollable const):
+
+ Add a helper method (similar to `shouldHide()`) that can be used to make elements vertically scrollable during
+ style adjustment time. See above for more details.
+
+ (WebCore::ModalContainerObserver::tryToMakeBodyAndDocumentElementScrollableThroughQuirks):
+ * page/ModalContainerObserver.h:
+ * style/StyleAdjuster.cpp:
+ (WebCore::Style::Adjuster::adjust const):
+
+ Consult `shouldMakeVerticallyScrollable` in addition to `shouldHide` if `ModalContainerObserver` is present.
+
2022-01-07 Antti Koivisto <an...@apple.com>
Make separate invalidation rulesets for negated selectors (inside :not())
Modified: trunk/Source/WebCore/page/ModalContainerObserver.cpp (287776 => 287777)
--- trunk/Source/WebCore/page/ModalContainerObserver.cpp 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Source/WebCore/page/ModalContainerObserver.cpp 2022-01-07 20:28:04 UTC (rev 287777)
@@ -262,8 +262,12 @@
if (!container)
return;
- if (auto observer = container->document().modalContainerObserverIfExists(); observer && container == observer->container())
- observer->hideUserInteractionBlockingElementIfNeeded();
+ auto observer = container->document().modalContainerObserverIfExists();
+ if (!observer || container != observer->container())
+ return;
+
+ observer->hideUserInteractionBlockingElementIfNeeded();
+ observer->makeBodyAndDocumentElementScrollableIfNeeded();
});
}
@@ -538,8 +542,10 @@
if (decision == ModalContainerDecision::Show)
return;
- if (RefPtr controlToClick = classifiedControls.controlToClick(decision))
+ if (RefPtr controlToClick = classifiedControls.controlToClick(decision)) {
+ observer->clearScrollabilityOverrides(*document);
controlToClick->dispatchSimulatedClick(nullptr, SendMouseUpDownEvents, DoNotShowPressedLook);
+ }
decisionScope.continueHidingModalContainerAfterScope();
});
@@ -547,13 +553,63 @@
});
}
-void ModalContainerObserver::hideUserInteractionBlockingElementIfNeeded()
+void ModalContainerObserver::makeBodyAndDocumentElementScrollableIfNeeded()
{
- if (m_userInteractionBlockingElement) {
- ASSERT_NOT_REACHED();
+ if (!container())
return;
+
+ Ref document = container()->document();
+ RefPtr view = document->view();
+ if (!view || view->isScrollable())
+ return;
+
+ document->updateLayoutIgnorePendingStylesheets();
+
+ auto visibleHeight = view->visibleSize().height();
+ auto shouldMakeElementScrollable = [visibleHeight] (Element* element) {
+ if (!element)
+ return false;
+
+ auto renderer = element->renderer();
+ if (!renderer || renderer->style().overflowY() != Overflow::Hidden)
+ return false;
+
+ return element->boundingClientRect().height() > visibleHeight;
+ };
+
+ if (!m_makeBodyElementScrollable) {
+ if (RefPtr body = document->body(); shouldMakeElementScrollable(body.get())) {
+ m_makeBodyElementScrollable = true;
+ body->invalidateStyle();
+ }
}
+ if (!m_makeDocumentElementScrollable) {
+ if (RefPtr documentElement = document->documentElement(); shouldMakeElementScrollable(documentElement.get())) {
+ m_makeDocumentElementScrollable = true;
+ documentElement->invalidateStyle();
+ }
+ }
+}
+
+void ModalContainerObserver::clearScrollabilityOverrides(Document& document)
+{
+ if (std::exchange(m_makeBodyElementScrollable, false)) {
+ if (auto element = document.body())
+ element->invalidateStyle();
+ }
+
+ if (std::exchange(m_makeDocumentElementScrollable, false)) {
+ if (auto element = document.documentElement())
+ element->invalidateStyle();
+ }
+}
+
+void ModalContainerObserver::hideUserInteractionBlockingElementIfNeeded()
+{
+ if (m_userInteractionBlockingElement)
+ return;
+
RefPtr container = this->container();
if (!container) {
ASSERT_NOT_REACHED();
@@ -593,7 +649,7 @@
return;
if (!foundElement)
- foundElement = WTFMove(target);
+ foundElement = target;
}
m_userInteractionBlockingElement = foundElement.get();
@@ -603,8 +659,10 @@
void ModalContainerObserver::revealModalContainer()
{
auto [container, frameOwner] = std::exchange(m_containerAndFrameOwnerForControls, { });
- if (container)
+ if (container) {
container->invalidateStyle();
+ clearScrollabilityOverrides(container->document());
+ }
if (auto element = std::exchange(m_userInteractionBlockingElement, { }))
element->invalidateStyle();
@@ -660,4 +718,15 @@
return { WTFMove(classifiableControls), WTFMove(controlTextsToClassify) };
}
+bool ModalContainerObserver::shouldMakeVerticallyScrollable(const Element& element) const
+{
+ if (m_makeBodyElementScrollable && element.document().body() == &element)
+ return true;
+
+ if (m_makeDocumentElementScrollable && element.document().documentElement() == &element)
+ return true;
+
+ return false;
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/page/ModalContainerObserver.h (287776 => 287777)
--- trunk/Source/WebCore/page/ModalContainerObserver.h 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Source/WebCore/page/ModalContainerObserver.h 2022-01-07 20:28:04 UTC (rev 287777)
@@ -50,6 +50,7 @@
ModalContainerObserver();
~ModalContainerObserver();
+ bool shouldMakeVerticallyScrollable(const Element&) const;
inline bool shouldHide(const Element&) const;
void updateModalContainerIfNeeded(const FrameView&);
@@ -64,6 +65,9 @@
void setContainer(Element&, HTMLFrameOwnerElement* = nullptr);
void searchForModalContainerOnBehalfOfFrameOwnerIfNeeded(HTMLFrameOwnerElement&);
+ void makeBodyAndDocumentElementScrollableIfNeeded();
+ void clearScrollabilityOverrides(Document&);
+
Element* container() const;
HTMLFrameOwnerElement* frameOwnerForControls() const;
@@ -78,6 +82,8 @@
Timer m_collectClickableElementsTimer;
bool m_collectingClickableElements { false };
bool m_hasAttemptedToFulfillPolicy { false };
+ bool m_makeBodyElementScrollable { false };
+ bool m_makeDocumentElementScrollable { false };
};
inline void ModalContainerObserver::overrideSearchTermForTesting(const String& searchTerm)
Modified: trunk/Source/WebCore/style/StyleAdjuster.cpp (287776 => 287777)
--- trunk/Source/WebCore/style/StyleAdjuster.cpp 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Source/WebCore/style/StyleAdjuster.cpp 2022-01-07 20:28:04 UTC (rev 287777)
@@ -553,8 +553,12 @@
#endif
if (m_element) {
- if (auto observer = m_element->document().modalContainerObserver(); observer && observer->shouldHide(*m_element))
- style.setDisplay(DisplayType::None);
+ if (auto observer = m_element->document().modalContainerObserverIfExists()) {
+ if (observer->shouldHide(*m_element))
+ style.setDisplay(DisplayType::None);
+ if (observer->shouldMakeVerticallyScrollable(*m_element))
+ style.setOverflowY(Overflow::Auto);
+ }
}
adjustForSiteSpecificQuirks(style);
Modified: trunk/Tools/ChangeLog (287776 => 287777)
--- trunk/Tools/ChangeLog 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Tools/ChangeLog 2022-01-07 20:28:04 UTC (rev 287777)
@@ -1,3 +1,17 @@
+2022-01-07 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Teach modal container observer to make the body element scrollable if necessary
+ https://bugs.webkit.org/show_bug.cgi?id=234708
+ rdar://86960677
+
+ Reviewed by Tim Horton.
+
+ Adjust an existing API test to exercise the change.
+
+ * TestWebKitAPI/Tests/WebKit/modal-container-with-overlay.html:
+ * TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm:
+ (TestWebKitAPI::TEST):
+
2022-01-07 Ryan Haddad <ryanhad...@apple.com>
REGRESSION: TestWebKitAPI.PublicSuffix.IsPublicSuffix is failing on some bots
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKit/modal-container-with-overlay.html (287776 => 287777)
--- trunk/Tools/TestWebKitAPI/Tests/WebKit/modal-container-with-overlay.html 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit/modal-container-with-overlay.html 2022-01-07 20:28:04 UTC (rev 287777)
@@ -1,5 +1,5 @@
<!DOCTYPE html>
-<html>
+<html style="overflow: hidden;">
<head>
<meta name="viewport" content="width=device-width, initial-scale=1">
<style>
@@ -9,6 +9,10 @@
font-family: system-ui;
}
+body {
+ height: 3000px;
+}
+
button {
padding: 6px;
border-radius: 6px;
@@ -58,7 +62,7 @@
internals.overrideModalContainerSearchTermForTesting("hello world");
</script>
</head>
-<body>
+<body style="overflow: hidden;">
<p id="content">Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.</p>
<div id="overlay"></div>
<div id="fixed">
@@ -67,6 +71,8 @@
</div>
<script>
button.addEventListener("click", () => {
+ document.body.style.removeProperty("overflow");
+ document.documentElement.style.removeProperty("overflow");
fixed.remove();
overlay.remove();
});
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm (287776 => 287777)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm 2022-01-07 20:20:58 UTC (rev 287776)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ModalContainerObservation.mm 2022-01-07 20:28:04 UTC (rev 287777)
@@ -256,7 +256,7 @@
EXPECT_EQ([webView lastModalContainerInfo].availableTypes, _WKModalContainerControlTypePositive | _WKModalContainerControlTypeNegative);
}
-TEST(ModalContainerObservation, HideUserInteractionBlockingElement)
+TEST(ModalContainerObservation, HideUserInteractionBlockingElementAndMakeDocumentScrollable)
{
auto webView = createModalContainerWebView();
[webView loadBundlePage:@"modal-container-with-overlay" andDecidePolicy:_WKModalContainerDecisionHideAndIgnore];
@@ -266,6 +266,8 @@
NSString *hitTestedText = [webView stringByEvaluatingJavaScript:@"document.elementFromPoint(50, 50).textContent"];
EXPECT_TRUE([hitTestedText containsString:@"Lorem"]);
+ EXPECT_WK_STREQ("auto", [webView stringByEvaluatingJavaScript:@"getComputedStyle(document.documentElement).overflowY"]);
+ EXPECT_WK_STREQ("auto", [webView stringByEvaluatingJavaScript:@"getComputedStyle(document.body).overflowY"]);
}
TEST(ModalContainerObservation, IgnoreFixedDocumentElement)
