Skip to content
Permalink
Browse files
[css-scroll-snap] focus should not take into account scroll margin fo…
…r visibility check

https://bugs.webkit.org/show_bug.cgi?id=245100
<rdar://99840839>

Reviewed by Simon Fraser.

When an element is focused, we perform a visibility calculation to determine if we need to scroll
the element into view. This visibility calculation should not take into account the scroll margin of
the element. This prevents a bug where we wont scroll an element into view that is offscreen because
the rect including its scroll margnin is within the viewport.

* LayoutTests/imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-margin-visibility-check.html:
* Source/WebCore/page/FrameView.cpp:
(WebCore::FrameView::scrollToFocusedElementInternal):

Canonical link: https://commits.webkit.org/254732@main
  • Loading branch information
nmoucht committed Sep 21, 2022
1 parent c69e0a9 commit f121c13339f2b359b9a6aaf8e9186ec8c539f58e
Show file tree
Hide file tree
Showing 10 changed files with 76 additions and 44 deletions.
@@ -1,3 +1,3 @@

FAIL scroll-margin-visibility-check assert_true: Visibility check should not account for margin expected true got false
PASS scroll-margin-visibility-check

@@ -19,7 +19,7 @@
height: 100px;
background-color: blue;
scroll-margin: 100px;
margin-left: 450px;
margin-left: 510px;
}
</style>

@@ -32,9 +32,12 @@
<script>
let scroller = document.getElementById("scroller");
let target = document.getElementById("target");
test(t => {
promise_test(async function() {
scroller.scrollTo(0, 0);
target.focus();
await new Promise(resolve => {
scroller.addEventListener("scroll", () => step_timeout(resolve, 0));
document.getElementById("target").focus();
});
assert_true(scroller.scrollTop > 0, "Visibility check should not account for margin");
assert_true(scroller.scrollLeft > 0, "Visibility check should not account for margin");
});
@@ -1012,14 +1012,14 @@ void Element::scrollIntoView(std::optional<std::variant<bool, ScrollIntoViewOpti
renderer = listBoxScrollResult->first;
absoluteBounds = listBoxScrollResult->second;

auto listBoxAbsoluteBounds = renderer->absoluteAnchorRectWithScrollMargin(&insideFixed);
auto listBoxAbsoluteBounds = renderer->absoluteAnchorRectWithScrollMargin(&insideFixed).marginRect;
absoluteBounds.moveBy(listBoxAbsoluteBounds.location());
} else {
renderer = this->renderer();
if (!renderer)
return;

absoluteBounds = renderer->absoluteAnchorRectWithScrollMargin(&insideFixed);
absoluteBounds = renderer->absoluteAnchorRectWithScrollMargin(&insideFixed).marginRect;
}

ScrollIntoViewOptions options;
@@ -1055,7 +1055,7 @@ void Element::scrollIntoView(bool alignToTop)
return;

bool insideFixed;
LayoutRect absoluteBounds = renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed);
LayoutRect absoluteBounds = renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed).marginRect;

// Align to the top / bottom and to the closest edge.
auto alignY = alignToTop ? ScrollAlignment::alignTopAlways : ScrollAlignment::alignBottomAlways;
@@ -1073,7 +1073,7 @@ void Element::scrollIntoViewIfNeeded(bool centerIfNeeded)
return;

bool insideFixed;
LayoutRect absoluteBounds = renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed);
LayoutRect absoluteBounds = renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed).marginRect;

auto alignY = centerIfNeeded ? ScrollAlignment::alignCenterIfNeeded : ScrollAlignment::alignToEdgeIfNeeded;
auto alignX = centerIfNeeded ? ScrollAlignment::alignCenterIfNeeded : ScrollAlignment::alignToEdgeIfNeeded;
@@ -1090,7 +1090,7 @@ void Element::scrollIntoViewIfNotVisible(bool centerIfNotVisible)
return;

bool insideFixed;
LayoutRect absoluteBounds = renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed);
LayoutRect absoluteBounds = renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed).marginRect;
auto align = centerIfNotVisible ? ScrollAlignment::alignCenterIfNotVisible : ScrollAlignment::alignToEdgeIfNotVisible;
FrameView::scrollRectToVisible(absoluteBounds, *renderer(), insideFixed, { SelectionRevealMode::Reveal, align, align, ShouldAllowCrossOriginScrolling::No });
}
@@ -2458,8 +2458,10 @@ void FrameView::scrollToFocusedElementInternal()
return;

bool insideFixed;
LayoutRect absoluteBounds = renderer->absoluteAnchorRectWithScrollMargin(&insideFixed);
FrameView::scrollRectToVisible(absoluteBounds, *renderer, insideFixed, { m_selectionRevealModeForFocusedElement, ScrollAlignment::alignCenterIfNeeded, ScrollAlignment::alignCenterIfNeeded, ShouldAllowCrossOriginScrolling::No });
auto absoluteBounds = renderer->absoluteAnchorRectWithScrollMargin(&insideFixed);
auto anchorRectWithScrollMargin = absoluteBounds.marginRect;
auto anchorRect = absoluteBounds.anchorRect;
FrameView::scrollRectToVisible(anchorRectWithScrollMargin, *renderer, insideFixed, { m_selectionRevealModeForFocusedElement, ScrollAlignment::alignCenterIfNeeded, ScrollAlignment::alignCenterIfNeeded, ShouldAllowCrossOriginScrolling::No, ScrollBehavior::Auto, anchorRect });
}

void FrameView::textFragmentIndicatorTimerFired()
@@ -3526,7 +3528,7 @@ void FrameView::scrollToAnchor()
LayoutRect rect;
bool insideFixed = false;
if (anchorNode != frame().document() && anchorNode->renderer())
rect = anchorNode->renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed);
rect = anchorNode->renderer()->absoluteAnchorRectWithScrollMargin(&insideFixed).marginRect;

LOG_WITH_STREAM(Scrolling, stream << " anchor node rect " << rect);

@@ -808,23 +808,28 @@ std::optional<BoxSide> ScrollableArea::targetSideForScrollDelta(FloatSize delta,
return { };
}

LayoutRect ScrollableArea::getRectToExposeForScrollIntoView(const LayoutRect& visibleRect, const LayoutRect& exposeRect, const ScrollAlignment& alignX, const ScrollAlignment& alignY) const
LayoutRect ScrollableArea::getRectToExposeForScrollIntoView(const LayoutRect& visibleBounds, const LayoutRect& exposeRect, const ScrollAlignment& alignX, const ScrollAlignment& alignY, const std::optional<LayoutRect> visibilityCheckRect) const
{
static const int minIntersectForReveal = 32;

ScrollAlignment::Behavior scrollX = alignX.getHiddenBehavior();
bool intersectsInX = exposeRect.maxX() >= visibleRect.x() && exposeRect.x() <= visibleRect.maxX();


bool intersectsInX = false;
if (visibilityCheckRect)
intersectsInX = visibilityCheckRect->maxX() >= visibleBounds.x() && visibilityCheckRect->x() <= visibleBounds.maxX();
else
intersectsInX = exposeRect.maxX() >= visibleBounds.x() && exposeRect.x() <= visibleBounds.maxX();

// Determine the appropriate X behavior.
if (intersectsInX) {
LayoutUnit intersectWidth = std::max(LayoutUnit(), std::min(visibleRect.maxX(), exposeRect.maxX()) - std::max(visibleRect.x(), exposeRect.x()));
LayoutUnit intersectWidth = std::max(LayoutUnit(), std::min(visibleBounds.maxX(), exposeRect.maxX()) - std::max(visibleBounds.x(), exposeRect.x()));

if (intersectWidth == exposeRect.width() || (alignX.legacyHorizontalVisibilityThresholdEnabled() && intersectWidth >= minIntersectForReveal)) {
// If the rectangle is fully visible, use the specified visible behavior.
// If the rectangle is partially visible, but over a certain threshold,
// then treat it as fully visible to avoid unnecessary horizontal scrolling
scrollX = alignX.getVisibleBehavior();
} else if (intersectWidth == visibleRect.width()) {
} else if (intersectWidth == visibleBounds.width()) {
// If the rect is bigger than the visible area, don't bother trying to center. Other alignments will work.
scrollX = alignX.getVisibleBehavior();
if (scrollX == ScrollAlignment::Behavior::AlignCenter)
@@ -837,31 +842,36 @@ LayoutRect ScrollableArea::getRectToExposeForScrollIntoView(const LayoutRect& vi
}

// If we're trying to align to the closest edge, and the exposeRect is further right
// than the visibleRect, and not bigger than the visible area, then align with the right.
if (scrollX == ScrollAlignment::Behavior::AlignToClosestEdge && exposeRect.maxX() > visibleRect.maxX() && exposeRect.width() < visibleRect.width())
// than the visibleBounds, and not bigger than the visible area, then align with the right.
if (scrollX == ScrollAlignment::Behavior::AlignToClosestEdge && exposeRect.maxX() > visibleBounds.maxX() && exposeRect.width() < visibleBounds.width())
scrollX = ScrollAlignment::Behavior::AlignRight;

// Given the X behavior, compute the X coordinate.
LayoutUnit x;
if (scrollX == ScrollAlignment::Behavior::NoScroll)
x = visibleRect.x();
x = visibleBounds.x();
else if (scrollX == ScrollAlignment::Behavior::AlignRight)
x = exposeRect.maxX() - visibleRect.width();
x = exposeRect.maxX() - visibleBounds.width();
else if (scrollX == ScrollAlignment::Behavior::AlignCenter)
x = exposeRect.x() + (exposeRect.width() - visibleRect.width()) / 2;
x = exposeRect.x() + (exposeRect.width() - visibleBounds.width()) / 2;
else
x = exposeRect.x();

ScrollAlignment::Behavior scrollY = alignY.getHiddenBehavior();
bool intersectsInY = exposeRect.maxY() >= visibleRect.y() && exposeRect.y() <= visibleRect.maxY();

bool intersectsInY = false;
if (visibilityCheckRect)
intersectsInY = visibilityCheckRect->maxY() >= visibleBounds.y() && visibilityCheckRect->y() <= visibleBounds.maxY();
else
intersectsInY = exposeRect.maxY() >= visibleBounds.y() && exposeRect.y() <= visibleBounds.maxY();

// Determine the appropriate Y behavior.
if (intersectsInY) {
LayoutUnit intersectHeight = std::max(LayoutUnit(), std::min(visibleRect.maxY(), exposeRect.maxY()) - std::max(visibleRect.y(), exposeRect.y()));
LayoutUnit intersectHeight = std::max(LayoutUnit(), std::min(visibleBounds.maxY(), exposeRect.maxY()) - std::max(visibleBounds.y(), exposeRect.y()));
if (intersectHeight == exposeRect.height()) {
// If the rectangle is fully visible, use the specified visible behavior.
scrollY = alignY.getVisibleBehavior();
} else if (intersectHeight == visibleRect.height()) {
} else if (intersectHeight == visibleBounds.height()) {
// If the rect is bigger than the visible area, don't bother trying to center. Other alignments will work.
scrollY = alignY.getVisibleBehavior();
if (scrollY == ScrollAlignment::Behavior::AlignCenter)
@@ -874,22 +884,22 @@ LayoutRect ScrollableArea::getRectToExposeForScrollIntoView(const LayoutRect& vi
}

// If we're trying to align to the closest edge, and the exposeRect is further down
// than the visibleRect, and not bigger than the visible area, then align with the bottom.
if (scrollY == ScrollAlignment::Behavior::AlignToClosestEdge && exposeRect.maxY() > visibleRect.maxY() && exposeRect.height() < visibleRect.height())
// than the visibleBounds, and not bigger than the visible area, then align with the bottom.
if (scrollY == ScrollAlignment::Behavior::AlignToClosestEdge && exposeRect.maxY() > visibleBounds.maxY() && exposeRect.height() < visibleBounds.height())
scrollY = ScrollAlignment::Behavior::AlignBottom;

// Given the Y behavior, compute the Y coordinate.
LayoutUnit y;
if (scrollY == ScrollAlignment::Behavior::NoScroll)
y = visibleRect.y();
y = visibleBounds.y();
else if (scrollY == ScrollAlignment::Behavior::AlignBottom)
y = exposeRect.maxY() - visibleRect.height();
y = exposeRect.maxY() - visibleBounds.height();
else if (scrollY == ScrollAlignment::Behavior::AlignCenter)
y = exposeRect.y() + (exposeRect.height() - visibleRect.height()) / 2;
y = exposeRect.y() + (exposeRect.height() - visibleBounds.height()) / 2;
else
y = exposeRect.y();

return LayoutRect(LayoutPoint(x, y), visibleRect.size());
return LayoutRect(LayoutPoint(x, y), visibleBounds.size());
}

TextStream& operator<<(TextStream& ts, const ScrollableArea& scrollableArea)
@@ -406,7 +406,7 @@ class ScrollableArea : public CanMakeWeakPtr<ScrollableArea> {

bool hasLayerForScrollCorner() const;

LayoutRect getRectToExposeForScrollIntoView(const LayoutRect& visibleRect, const LayoutRect& exposeRect, const ScrollAlignment& alignX, const ScrollAlignment& alignY) const;
LayoutRect getRectToExposeForScrollIntoView(const LayoutRect& visibleBounds, const LayoutRect& exposeRect, const ScrollAlignment& alignX, const ScrollAlignment& alignY, const std::optional<LayoutRect> = std::nullopt) const;

private:
WEBCORE_EXPORT virtual IntRect visibleContentRectInternal(VisibleContentRectIncludesScrollbars, VisibleContentRectBehavior) const;
@@ -1724,12 +1724,12 @@ LayoutRect RenderElement::absoluteAnchorRect(bool* insideFixed) const
return enclosingLayoutRect(FloatRect(upperLeft, lowerRight.expandedTo(upperLeft) - upperLeft));
}

LayoutRect RenderElement::absoluteAnchorRectWithScrollMargin(bool* insideFixed) const
MarginRect RenderElement::absoluteAnchorRectWithScrollMargin(bool* insideFixed) const
{
LayoutRect anchorRect = absoluteAnchorRect(insideFixed);
const LengthBox& scrollMargin = style().scrollMargin();
if (scrollMargin.isZero())
return anchorRect;
return { anchorRect, anchorRect };

// The scroll snap specification says that the scroll-margin should be applied in the
// coordinate system of the scroll container and applied to the rectangular bounding
@@ -1740,8 +1740,9 @@ LayoutRect RenderElement::absoluteAnchorRectWithScrollMargin(bool* insideFixed)
valueForLength(scrollMargin.right(), anchorRect.width()),
valueForLength(scrollMargin.bottom(), anchorRect.height()),
valueForLength(scrollMargin.left(), anchorRect.width()));
anchorRect.expand(margin);
return anchorRect;
auto marginRect = anchorRect;
marginRect.expand(margin);
return { marginRect, anchorRect };
}

static bool usePlatformFocusRingColorForOutlineStyleAuto()
@@ -36,6 +36,11 @@ class RenderBlock;
class RenderStyle;
class RenderTreeBuilder;

struct MarginRect {
LayoutRect marginRect;
LayoutRect anchorRect;
};

class RenderElement : public RenderObject {
WTF_MAKE_ISO_ALLOCATED(RenderElement);
public:
@@ -195,7 +200,7 @@ class RenderElement : public RenderObject {

// absoluteAnchorRectWithScrollMargin() is similar to absoluteAnchorRect, but it also takes into account any
// CSS scroll-margin that is set in the style of this RenderElement.
LayoutRect absoluteAnchorRectWithScrollMargin(bool* insideFixed = nullptr) const;
MarginRect absoluteAnchorRectWithScrollMargin(bool* insideFixed = nullptr) const;

bool hasFilter() const { return style().hasFilter(); }
bool hasBackdropFilter() const
@@ -47,6 +47,7 @@
#include "ClipRect.h"
#include "GraphicsLayer.h"
#include "LayerFragment.h"
#include "LayoutRect.h"
#include "PaintFrequencyTracker.h"
#include "PaintInfo.h"
#include "RenderBox.h"
@@ -142,6 +143,7 @@ struct ScrollRectToVisibleOptions {
const ScrollAlignment& alignY { ScrollAlignment::alignCenterIfNeeded };
ShouldAllowCrossOriginScrolling shouldAllowCrossOriginScrolling { ShouldAllowCrossOriginScrolling::No };
ScrollBehavior behavior { ScrollBehavior::Auto };
std::optional<LayoutRect> visibilityCheckRect { std::nullopt };
};

using ScrollingScope = uint64_t;
@@ -1806,11 +1806,8 @@ void RenderLayerScrollableArea::panScrollFromPoint(const IntPoint& sourcePoint)

scrollByRecursively(adjustedScrollDelta(delta));
}

LayoutRect RenderLayerScrollableArea::scrollRectToVisible(const LayoutRect& absoluteRect, const ScrollRectToVisibleOptions& options)
static LayoutRect getLocalExposeRect(const LayoutRect& absoluteRect, RenderBox* box, int verticalScrollbarWidth, const LayoutRect& layerBounds)
{
RenderBox* box = layer().renderBox();
ASSERT(box);
LayoutRect localExposeRect(box->absoluteToLocalQuad(FloatQuad(FloatRect(absoluteRect))).boundingBox());

// localExposedRect is now the absolute rect in local coordinates, but relative to the
@@ -1821,17 +1818,29 @@ LayoutRect RenderLayerScrollableArea::scrollRectToVisible(const LayoutRect& abso
// For `direction: rtl; writing-mode: horizontal-{tb,bt}` and `writing-mode: vertical-rl`
// boxes, the scroll bar is on the left side. The visible rect starts from the right side
// of the scroll bar. So the x of localExposeRect should start from the same position too.
localExposeRect.moveBy(LayoutPoint(-verticalScrollbarWidth(), 0));
localExposeRect.moveBy(LayoutPoint(-verticalScrollbarWidth, 0));
}

// scroll-padding applies to the scroll container, but expand the rectangle that we want to expose in order
// simulate padding the scroll container. This rectangle is passed up the tree of scrolling elements to
// ensure that the padding on this scroll container is maintained.
LayoutRect layerBounds(0_lu, 0_lu, box->clientWidth(), box->clientHeight());
localExposeRect.expand(box->scrollPaddingForViewportRect(layerBounds));
return localExposeRect;
}

LayoutRect RenderLayerScrollableArea::scrollRectToVisible(const LayoutRect& absoluteRect, const ScrollRectToVisibleOptions& options)
{
RenderBox* box = layer().renderBox();
ASSERT(box);

LayoutRect layerBounds(0_lu, 0_lu, box->clientWidth(), box->clientHeight());

auto revealRect = getRectToExposeForScrollIntoView(layerBounds, localExposeRect, options.alignX, options.alignY);
LayoutRect localExposeRect = getLocalExposeRect(absoluteRect, box, verticalScrollbarWidth(), layerBounds);
std::optional<LayoutRect> localVisiblityRect;
if (options.visibilityCheckRect)
localVisiblityRect = getLocalExposeRect(*options.visibilityCheckRect, box, verticalScrollbarWidth(), layerBounds);

auto revealRect = getRectToExposeForScrollIntoView(layerBounds, localExposeRect, options.alignX, options.alignY, localVisiblityRect);
auto scrollPositionOptions = ScrollPositionChangeOptions::createProgrammatic();
if (!box->frame().eventHandler().autoscrollInProgress() && box->element() && useSmoothScrolling(options.behavior, box->element()))
scrollPositionOptions.animated = ScrollIsAnimated::Yes;

0 comments on commit f121c13

Please sign in to comment.