- Revision
- 259248
- Author
- simon.fra...@apple.com
- Date
- 2020-03-30 17:03:24 -0700 (Mon, 30 Mar 2020)
Log Message
scrollIntoView() erroneously scrolls non-containing block scrollers
https://bugs.webkit.org/show_bug.cgi?id=209715
Reviewed by Zalan Bujtas.
Source/WebCore:
RenderLayer::scrollByRecursively() would just walk up the parent RenderLayer chain
trying to scroll each one. However, this would hit overflow:scroll which wasn't in the
containing block chain, erroneously scrolling unrelated scrollers.
Fix by adding RenderLayer::enclosingLayerInContainingBlockOrder() and calling
it from RenderLayer::enclosingScrollableLayer(). Also extend enclosingScrollableLayer()
to make explicit 'include self' and frame-boundary crossing.
In future, scrollByRecursively() should really be virtual on ScrollableArea, and implemented
on its subclasses.
Test: http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html
* page/mac/EventHandlerMac.mm:
(WebCore::EventHandler::platformPrepareForWheelEvents):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::enclosingLayerInContainingBlockOrder const):
(WebCore::enclosingContainingBlockLayer):
(WebCore::RenderLayer::enclosingScrollableLayer const):
(WebCore::RenderLayer::scrollByRecursively):
(WebCore::RenderLayer::scrollRectToVisible):
(WebCore::RenderLayer::enclosingScrollableArea const):
(WebCore::RenderLayer::hasScrollableOrRubberbandableAncestor):
(WebCore::RenderLayer::calculateClipRects const):
(WebCore::parentLayerCrossFrame): Deleted.
* rendering/RenderLayer.h:
Source/WebKitLegacy/win:
* WebView.cpp:
(WebView::gesture):
LayoutTests:
* http/wpt/css/cssom-view/scrollintoview-containingblock-chain-expected.txt: Added.
* http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (259247 => 259248)
--- trunk/LayoutTests/ChangeLog 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/LayoutTests/ChangeLog 2020-03-31 00:03:24 UTC (rev 259248)
@@ -1,3 +1,13 @@
+2020-03-30 Simon Fraser <simon.fra...@apple.com>
+
+ scrollIntoView() erroneously scrolls non-containing block scrollers
+ https://bugs.webkit.org/show_bug.cgi?id=209715
+
+ Reviewed by Zalan Bujtas.
+
+ * http/wpt/css/cssom-view/scrollintoview-containingblock-chain-expected.txt: Added.
+ * http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html: Added.
+
2020-03-30 Ryan Haddad <ryanhad...@apple.com>
Unreviewed test gardening, mark webrtc/datachannel/multiple-connections.html as slow.
Added: trunk/LayoutTests/http/wpt/css/cssom-view/scrollintoview-containingblock-chain-expected.txt (0 => 259248)
--- trunk/LayoutTests/http/wpt/css/cssom-view/scrollintoview-containingblock-chain-expected.txt (rev 0)
+++ trunk/LayoutTests/http/wpt/css/cssom-view/scrollintoview-containingblock-chain-expected.txt 2020-03-31 00:03:24 UTC (rev 259248)
@@ -0,0 +1,3 @@
+
+PASS scrollIntoView should not scroll ancestor overflow:scroll elements that are not containing block ancestors
+
Added: trunk/LayoutTests/http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html (0 => 259248)
--- trunk/LayoutTests/http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html (rev 0)
+++ trunk/LayoutTests/http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html 2020-03-31 00:03:24 UTC (rev 259248)
@@ -0,0 +1,63 @@
+<!DOCTYPE HTML>
+<script src=""
+<script src=""
+<title>scrollIntoView should only adjust scrollers in the containing block chain</title>
+<style>
+ .scroller {
+ width: 300px;
+ height: 300px;
+ overflow: scroll;
+ }
+
+ .contents {
+ width: 200%;
+ height: 300%;
+ }
+
+ #inner.scroller {
+ position: absolute;
+ margin-top: 250px;
+ margin-left: 100px;
+ width: 400px
+ }
+
+ #reveal {
+ margin-top: 400px;
+ background-color: blue;
+ }
+</style>
+<div id="container">
+ <div id="outer" class="scroller">
+ <div class="contents">
+ This should not scroll
+ <div id="inner" class="inner scroller">
+ <div class="contents">
+ contents
+ <div id="reveal">
+ Reveal me
+ </div>
+ </div>
+ </div>
+ </div>
+</div>
+</div>
+<script>
+add_completion_callback(() => document.getElementById("container").remove());
+
+test(t => {
+ var reveal = document.getElementById('reveal');
+
+ var outerScroller = document.getElementById('outer');
+ var innerScroller = document.getElementById('inner');
+ var initialOuterTop = outerScroller.scrollTop;
+ var initialInnerTop = innerScroller.scrollTop;
+
+ assert_equals(initialOuterTop, 0);
+ assert_equals(initialInnerTop, 0);
+
+ reveal.scrollIntoView({block: "start", inline: "start"});
+
+ assert_approx_equals(innerScroller.scrollTop, 418, 4);
+ assert_equals(outerScroller.scrollTop, 0);
+}, "scrollIntoView should not scroll ancestor overflow:scroll elements that are not containing block ancestors");
+</script>
Modified: trunk/Source/WebCore/ChangeLog (259247 => 259248)
--- trunk/Source/WebCore/ChangeLog 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/Source/WebCore/ChangeLog 2020-03-31 00:03:24 UTC (rev 259248)
@@ -1,3 +1,37 @@
+2020-03-30 Simon Fraser <simon.fra...@apple.com>
+
+ scrollIntoView() erroneously scrolls non-containing block scrollers
+ https://bugs.webkit.org/show_bug.cgi?id=209715
+
+ Reviewed by Zalan Bujtas.
+
+ RenderLayer::scrollByRecursively() would just walk up the parent RenderLayer chain
+ trying to scroll each one. However, this would hit overflow:scroll which wasn't in the
+ containing block chain, erroneously scrolling unrelated scrollers.
+
+ Fix by adding RenderLayer::enclosingLayerInContainingBlockOrder() and calling
+ it from RenderLayer::enclosingScrollableLayer(). Also extend enclosingScrollableLayer()
+ to make explicit 'include self' and frame-boundary crossing.
+
+ In future, scrollByRecursively() should really be virtual on ScrollableArea, and implemented
+ on its subclasses.
+
+ Test: http/wpt/css/cssom-view/scrollintoview-containingblock-chain.html
+
+ * page/mac/EventHandlerMac.mm:
+ (WebCore::EventHandler::platformPrepareForWheelEvents):
+ * rendering/RenderLayer.cpp:
+ (WebCore::RenderLayer::enclosingLayerInContainingBlockOrder const):
+ (WebCore::enclosingContainingBlockLayer):
+ (WebCore::RenderLayer::enclosingScrollableLayer const):
+ (WebCore::RenderLayer::scrollByRecursively):
+ (WebCore::RenderLayer::scrollRectToVisible):
+ (WebCore::RenderLayer::enclosingScrollableArea const):
+ (WebCore::RenderLayer::hasScrollableOrRubberbandableAncestor):
+ (WebCore::RenderLayer::calculateClipRects const):
+ (WebCore::parentLayerCrossFrame): Deleted.
+ * rendering/RenderLayer.h:
+
2020-03-30 Wenson Hsieh <wenson_hs...@apple.com>
WebPasteboardProxy::SetPasteboardURL should fail gracefully when the copied NSURL is nil
Modified: trunk/Source/WebCore/page/mac/EventHandlerMac.mm (259247 => 259248)
--- trunk/Source/WebCore/page/mac/EventHandlerMac.mm 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/Source/WebCore/page/mac/EventHandlerMac.mm 2020-03-31 00:03:24 UTC (rev 259248)
@@ -970,7 +970,7 @@
LOG_WITH_STREAM(ScrollLatching, stream << "EventHandler::platformPrepareForWheelEvents() - event" << wheelEvent << " found scrollableContainer" << ValueOrNull(scrollableContainer.get()) << " scrollableArea " << (scrollableArea ? scrollableArea.get() : nullptr));
}
}
-
+
Page* page = m_frame.page();
if (scrollableArea && page && page->isMonitoringWheelEvents())
scrollableArea->scrollAnimator().setWheelEventTestMonitor(page->wheelEventTestMonitor());
Modified: trunk/Source/WebCore/rendering/RenderLayer.cpp (259247 => 259248)
--- trunk/Source/WebCore/rendering/RenderLayer.cpp 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/Source/WebCore/rendering/RenderLayer.cpp 2020-03-31 00:03:24 UTC (rev 259248)
@@ -1876,6 +1876,16 @@
return curr;
}
+RenderLayer* RenderLayer::enclosingLayerInContainingBlockOrder() const
+{
+ for (const auto* currentBlock = renderer().containingBlock(); currentBlock; currentBlock = currentBlock->containingBlock()) {
+ if (auto* layer = currentBlock->layer())
+ return layer;
+ }
+
+ return nullptr;
+}
+
static RenderLayer* enclosingFrameRenderLayer(const RenderLayer& layer)
{
auto* ownerElement = layer.renderer().document().ownerElement();
@@ -1889,18 +1899,28 @@
return ownerRenderer->enclosingLayer();
}
-static RenderLayer* parentLayerCrossFrame(const RenderLayer& layer)
+static RenderLayer* enclosingContainingBlockLayer(const RenderLayer& layer, CrossFrameBoundaries crossFrameBoundaries)
{
- if (auto* parent = layer.parent())
- return parent;
+ if (auto* ancestor = layer.enclosingLayerInContainingBlockOrder())
+ return ancestor;
+ if (crossFrameBoundaries == CrossFrameBoundaries::No)
+ return nullptr;
+
return enclosingFrameRenderLayer(layer);
}
-RenderLayer* RenderLayer::enclosingScrollableLayer() const
+RenderLayer* RenderLayer::enclosingScrollableLayer(IncludeSelfOrNot includeSelf, CrossFrameBoundaries crossFrameBoundaries) const
{
- for (RenderLayer* nextLayer = parentLayerCrossFrame(*this); nextLayer; nextLayer = parentLayerCrossFrame(*nextLayer)) {
- if (is<RenderBox>(nextLayer->renderer()) && downcast<RenderBox>(nextLayer->renderer()).canBeScrolledAndHasScrollableArea())
+ auto isConsideredScrollable = [](const RenderLayer& layer) {
+ return is<RenderBox>(layer.renderer()) && downcast<RenderBox>(layer.renderer()).canBeScrolledAndHasScrollableArea();
+ };
+
+ if (includeSelf == IncludeSelfOrNot::IncludeSelf && isConsideredScrollable(*this))
+ return const_cast<RenderLayer*>(this);
+
+ for (auto* nextLayer = enclosingContainingBlockLayer(*this, crossFrameBoundaries); nextLayer; nextLayer = enclosingContainingBlockLayer(*nextLayer, crossFrameBoundaries)) {
+ if (isConsideredScrollable(*nextLayer))
return nextLayer;
}
@@ -2569,7 +2589,8 @@
// If this layer can't do the scroll we ask the next layer up that can scroll to try
IntSize remainingScrollOffset = newScrollOffset - scrollOffset();
if (!remainingScrollOffset.isZero() && renderer().parent()) {
- if (RenderLayer* scrollableLayer = enclosingScrollableLayer())
+ // FIXME: This skips scrollable frames.
+ if (auto* scrollableLayer = enclosingScrollableLayer(IncludeSelfOrNot::ExcludeSelf, CrossFrameBoundaries::Yes))
scrollableLayer->scrollByRecursively(remainingScrollOffset, scrolledArea);
renderer().frame().eventHandler().updateAutoscrollRenderer();
@@ -2819,16 +2840,10 @@
{
LOG_WITH_STREAM(Scrolling, stream << "Layer " << this << " scrollRectToVisible " << absoluteRect);
- RenderLayer* parentLayer = nullptr;
LayoutRect newRect = absoluteRect;
-
- // We may end up propagating a scroll event. It is important that we suspend events until
- // the end of the function since they could delete the layer or the layer's renderer().
FrameView& frameView = renderer().view().frameView();
+ auto* parentLayer = enclosingContainingBlockLayer(*this, CrossFrameBoundaries::No);
- if (renderer().parent())
- parentLayer = renderer().parent()->enclosingLayer();
-
if (allowsCurrentScroll()) {
// Don't scroll to reveal an overflow layer that is restricted by the -webkit-line-clamp property.
// This will prevent us from revealing text hidden by the slider in Safari RSS.
@@ -2852,7 +2867,7 @@
localExposeRect.move(-scrollOffsetDifference);
newRect = LayoutRect(box->localToAbsoluteQuad(FloatQuad(FloatRect(localExposeRect)), UseTransforms).boundingBox());
}
- } else if (!parentLayer && renderer().isRenderView()) {
+ } else if (!parentLayer) {
HTMLFrameOwnerElement* ownerElement = renderer().document().ownerElement();
if (ownerElement && ownerElement->renderer()) {
@@ -2877,7 +2892,8 @@
frameView.setScrollPosition(scrollPosition, ScrollClamping::Clamped, animated);
if (options.shouldAllowCrossOriginScrolling == ShouldAllowCrossOriginScrolling::Yes || frameView.safeToPropagateScrollToParent()) {
- parentLayer = ownerElement->renderer()->enclosingLayer();
+ if (auto* enclosingLayer = ownerElement->renderer()->enclosingLayer())
+ parentLayer = enclosingLayer->enclosingScrollableLayer(IncludeSelfOrNot::IncludeSelf, CrossFrameBoundaries::No);
// Convert the rect into the coordinate space of the parent frame's document.
newRect = frameView.contentsToContainingViewContents(enclosingIntRect(newRect));
insideFixed = false; // FIXME: ideally need to determine if this <iframe> is inside position:fixed.
@@ -3582,7 +3598,7 @@
ScrollableArea* RenderLayer::enclosingScrollableArea() const
{
- if (RenderLayer* scrollableLayer = enclosingScrollableLayer())
+ if (RenderLayer* scrollableLayer = enclosingScrollableLayer(IncludeSelfOrNot::ExcludeSelf, CrossFrameBoundaries::Yes))
return scrollableLayer;
// FIXME: We should return the frame view here (or possibly an ancestor frame view,
@@ -3597,7 +3613,7 @@
bool RenderLayer::hasScrollableOrRubberbandableAncestor()
{
- for (RenderLayer* nextLayer = parentLayerCrossFrame(*this); nextLayer; nextLayer = parentLayerCrossFrame(*nextLayer)) {
+ for (auto* nextLayer = enclosingContainingBlockLayer(*this, CrossFrameBoundaries::Yes); nextLayer; nextLayer = enclosingContainingBlockLayer(*nextLayer, CrossFrameBoundaries::Yes)) {
if (nextLayer->isScrollableOrRubberbandable())
return true;
}
@@ -6966,6 +6982,14 @@
return renderer().style().filterOutsets();
}
+static RenderLayer* parentLayerCrossFrame(const RenderLayer& layer)
+{
+ if (auto* parent = layer.parent())
+ return parent;
+
+ return enclosingFrameRenderLayer(layer);
+}
+
bool RenderLayer::isTransparentRespectingParentFrames() const
{
static const double minimumVisibleOpacity = 0.01;
Modified: trunk/Source/WebCore/rendering/RenderLayer.h (259247 => 259248)
--- trunk/Source/WebCore/rendering/RenderLayer.h 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/Source/WebCore/rendering/RenderLayer.h 2020-03-31 00:03:24 UTC (rev 259248)
@@ -85,6 +85,7 @@
enum BorderRadiusClippingRule { IncludeSelfForBorderRadius, DoNotIncludeSelfForBorderRadius };
enum IncludeSelfOrNot { IncludeSelf, ExcludeSelf };
+enum CrossFrameBoundaries { No, Yes };
enum RepaintStatus {
NeedsNormalRepaint,
@@ -460,6 +461,9 @@
void setPostLayoutScrollPosition(Optional<ScrollPosition>);
void applyPostLayoutScrollPositionIfNeeded();
+ // Returns the nearest enclosing layer that is scrollable.
+ RenderLayer* enclosingScrollableLayer(IncludeSelfOrNot, CrossFrameBoundaries) const;
+
void availableContentSizeChanged(AvailableSizeChangeReason) override;
enum AutoscrollStatus { NotInProgress, InProgress };
@@ -631,10 +635,9 @@
// Gets the nearest enclosing positioned ancestor layer (also includes
// the <html> layer and the root layer).
RenderLayer* enclosingAncestorForPosition(PositionType) const;
+
+ RenderLayer* enclosingLayerInContainingBlockOrder() const;
- // Returns the nearest enclosing layer that is scrollable.
- RenderLayer* enclosingScrollableLayer() const;
-
// The layer relative to which clipping rects for this layer are computed.
RenderLayer* clippingRootForPainting() const;
Modified: trunk/Source/WebKitLegacy/win/ChangeLog (259247 => 259248)
--- trunk/Source/WebKitLegacy/win/ChangeLog 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/Source/WebKitLegacy/win/ChangeLog 2020-03-31 00:03:24 UTC (rev 259248)
@@ -1,3 +1,13 @@
+2020-03-30 Simon Fraser <simon.fra...@apple.com>
+
+ scrollIntoView() erroneously scrolls non-containing block scrollers
+ https://bugs.webkit.org/show_bug.cgi?id=209715
+
+ Reviewed by Zalan Bujtas.
+
+ * WebView.cpp:
+ (WebView::gesture):
+
2020-03-29 Darin Adler <da...@apple.com>
Move TextIterator::rangeFromLocationAndLength off of live ranges
Modified: trunk/Source/WebKitLegacy/win/WebView.cpp (259247 => 259248)
--- trunk/Source/WebKitLegacy/win/WebView.cpp 2020-03-31 00:02:50 UTC (rev 259247)
+++ trunk/Source/WebKitLegacy/win/WebView.cpp 2020-03-31 00:03:24 UTC (rev 259248)
@@ -2037,8 +2037,10 @@
IntSize logicalScrollDelta(-deltaX * scaleFactor, -deltaY * scaleFactor);
RenderLayer* scrollableLayer = nullptr;
- if (m_gestureTargetNode && m_gestureTargetNode->renderer() && m_gestureTargetNode->renderer()->enclosingLayer())
- scrollableLayer = m_gestureTargetNode->renderer()->enclosingLayer()->enclosingScrollableLayer();
+ if (m_gestureTargetNode && m_gestureTargetNode->renderer()) {
+ if (auto* layer = m_gestureTargetNode->renderer()->enclosingLayer())
+ scrollableLayer = layer->enclosingScrollableLayer(IncludeSelfOrNot::ExcludeSelf, CrossFrameBoundaries::Yes);
+ }
if (!scrollableLayer) {
// We might directly hit the document without hitting any nodes
