Skip to content
Permalink
Browse files
scroll-padding should affect paging operations
https://bugs.webkit.org/show_bug.cgi?id=219074
<rdar://problem/71747786>

Patch by Martin Robinson <mrobinson@igalia.com> on 2021-01-25
Reviewed by Simon Fraser.

Source/WebCore:

Have scroll-padding affect the amount of the scrollable area that moves during
paging operations. This is the behavior specified in the scroll snap specification
and allows scrollable areas with partially obscured areas to properly page through
their content.

See https://drafts.csswg.org/css-scroll-snap-1/#propdef-scroll-padding.

Tests: css3/scroll-snap/scroll-padding-mainframe-paging.html
       css3/scroll-snap/scroll-padding-overflow-paging.html

* page/FrameView.cpp:
(WebCore::FrameView::updateScrollbarSteps): Added this override method which
properly sets page steps. Only FrameView has access to the document.
* page/FrameView.h:
* platform/ScrollView.cpp:
(WebCore::ScrollView::updateScrollbars): Added this helper method, which is
virtual so that FrameView can override it.
(WebCore::ScrollView::updateScrollbarSteps): Use the helper method to actually
set the scrollbar steps.
* platform/ScrollView.h: Add new method declarations.
* rendering/RenderBox.cpp:
(WebCore::RenderBox::scrollPaddingForViewportRect): Add this new helper which
gets the scroll-padding for a box.
* rendering/RenderBox.h: Add new helper.
* rendering/RenderLayer.cpp:
(WebCore::expandScrollRectToVisibleTargetRectToIncludeScrollPadding): Use the new
RenderBox helper.
(WebCore::RenderLayer::updateScrollbarsAfterLayout): Use the new updateScrollbarSteps helper.
(WebCore::RenderLayer::updateScrollbarSteps): Added this helper so that RenderLayerModelObject
can update steps when necessary.
* rendering/RenderLayer.h: Added new declarations.
* rendering/RenderLayerModelObject.cpp:
(WebCore::RenderLayerModelObject::styleDidChange): Update steps on FrameViews and RenderLayers
when the style change dictates it.

LayoutTests:

* css3/scroll-snap/scroll-padding-mainframe-paging-expected.txt: Added.
* css3/scroll-snap/scroll-padding-mainframe-paging.html: Added.
* css3/scroll-snap/scroll-padding-overflow-paging-expected.txt: Added.
* css3/scroll-snap/scroll-padding-overflow-paging.html: Added.
* platform/ios-wk2/TestExpectations: Skip failing tests.
* platform/mac-wk1/TestExpectations: Skip failing tests.

Canonical link: https://commits.webkit.org/233294@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271788 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
mrobinson authored and webkit-commit-queue committed Jan 25, 2021
1 parent 947c82b commit 81c33b1ef0c76cc91187f5bbb8d4d5ffab9e7149
Showing 19 changed files with 338 additions and 20 deletions.
@@ -1,3 +1,18 @@
2021-01-25 Martin Robinson <mrobinson@igalia.com>

scroll-padding should affect paging operations
https://bugs.webkit.org/show_bug.cgi?id=219074
<rdar://problem/71747786>

Reviewed by Simon Fraser.

* css3/scroll-snap/scroll-padding-mainframe-paging-expected.txt: Added.
* css3/scroll-snap/scroll-padding-mainframe-paging.html: Added.
* css3/scroll-snap/scroll-padding-overflow-paging-expected.txt: Added.
* css3/scroll-snap/scroll-padding-overflow-paging.html: Added.
* platform/ios-wk2/TestExpectations: Skip failing tests.
* platform/mac-wk1/TestExpectations: Skip failing tests.

2021-01-25 Carlos Garcia Campos <cgarcia@igalia.com>

Null Ptr Deref @ WebCore::ReplaceSelectionCommand::doApply
@@ -0,0 +1,11 @@
PASS paging moved body
PASS paging moved padded body
PASS document.scrollingElement.scrollTop is within 1 of 535.6837606837607
PASS paging moved padded body
PASS document.scrollingElement.scrollTop is within 1 of 535.6837606837607
PASS paging moved padded body
PASS document.scrollingElement.scrollTop is within 2 of 526.3675213675214
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,78 @@
<!DOCTYPE html>
<html>
<head>
<style>
body {
margin: 0;
}

.box {
width: 100vw;
height: 100vh;
}
</style>
<script src="../../resources/js-test.js"></script>
<script src="../../resources/ui-helper.js"></script>
<script>
window.jsTestIsAsync = true;

async function runTests()
{
try {
await UIHelper.keyDown("pageDown");

expectTrue(document.scrollingElement.scrollTop != 0, "paging moved body");
let defaultPagePosition = document.scrollingElement.scrollTop;
let viewportHeight = document.scrollingElement.clientHeight;
let pageProportion = document.scrollingElement.scrollTop / viewportHeight;

document.scrollingElement.scrollTop = 0;
document.documentElement.style.scrollPaddingTop = "10px";
await UIHelper.keyDown("pageDown");

let expected = (viewportHeight - 10) * pageProportion;
expectTrue(document.scrollingElement.scrollTop != 0, "paging moved padded body");
shouldBeCloseTo("document.scrollingElement.scrollTop", expected, 1);

document.scrollingElement.scrollTop = 0;
document.documentElement.style.scrollPaddingTop = "0px";
document.documentElement.style.scrollPaddingBottom = "10px";
await UIHelper.keyDown("pageDown");

expectTrue(document.scrollingElement.scrollTop != 0, "paging moved padded body");
shouldBeCloseTo("document.scrollingElement.scrollTop", expected, 1);

document.scrollingElement.scrollTop = 0;
document.documentElement.style.scrollPaddingTop = "10px";
document.documentElement.style.scrollPaddingBottom = "10px";
await UIHelper.keyDown("pageDown");

expected = (viewportHeight - 20) * pageProportion;
expectTrue(document.scrollingElement.scrollTop != 0, "paging moved padded body");
shouldBeCloseTo("document.scrollingElement.scrollTop", expected, 2);
} catch (e) {
console.log(e);
} finally {
finishJSTest();
}
}

function onLoad()
{
if (window.eventSender) {
runTests();
} else {
var console = document.getElementById('console');
console.innerText = "This test cannot be run manually."
}
}
</script>
</head>
<body onload="onLoad();">
<div class="box" style="background: green;"><div id="console"></div></div>
<div class="box" style="background: red;"></div>
<div class="box" style="background: pink;"></div>
<div class="box" style="background: yellow;"></div>
<div class="box" style="background: orange;"></div>
</body>
</html>
@@ -0,0 +1,11 @@
PASS paging moved container
PASS paging moved padded container
PASS container.scrollTop is within 1 of 140
PASS paging moved padded container
PASS container.scrollTop is within 1 of 140
PASS paging moved padded container
PASS container.scrollTop is within 1 of 132
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,89 @@
<!DOCTYPE html>
<html>
<head>
<style>
.box {
width: 200px;
height: 200px;
}

.gallery {
overflow: scroll;
}
</style>
<script src="../../resources/js-test.js"></script>
<script src="../../resources/ui-helper.js"></script>
<script>
window.jsTestIsAsync = true;

function clickOnElement(targetElement) {
var clientRect = targetElement.getBoundingClientRect();
eventSender.mouseMoveTo(clientRect.x + 5, clientRect.y + 5);
eventSender.mouseDown();
}

async function runTests()
{
try {
var container = document.getElementById("container");

clickOnElement(container);
await UIHelper.keyDown("pageDown");

expectTrue(container.scrollTop != 0, "paging moved container");
let defaultPagePosition = container.scrollTop;
let pageProportion = container.scrollTop / container.clientHeight;

container.scrollTop = 0;
container.style.scrollPaddingTop = "10px";
await UIHelper.keyDown("pageDown");

let expected = (container.clientHeight - 10) * pageProportion;
expectTrue(container.scrollTop != 0, "paging moved padded container");
shouldBeCloseTo("container.scrollTop", expected, 1);

container.scrollTop = 0;
container.style.scrollPaddingTop = "0px";
container.style.scrollPaddingBottom = "10px";
await UIHelper.keyDown("pageDown");

expectTrue(container.scrollTop != 0, "paging moved padded container");
shouldBeCloseTo("container.scrollTop", expected, 1);

container.scrollTop = 0;
container.style.scrollPaddingTop = "10px";
container.style.scrollPaddingBottom = "10px";
await UIHelper.keyDown("pageDown");

expected = (container.clientHeight - 20) * pageProportion;
expectTrue(container.scrollTop != 0, "paging moved padded container");
shouldBeCloseTo("container.scrollTop", expected, 1);
} catch (e) {
console.log(e);
} finally {
finishJSTest();
}
}

function onLoad()
{
if (window.eventSender) {
runTests();
} else {
var console = document.getElementById('console');
console.innerText = "This test cannot be run manually."
}
}
</script>
</head>
<body onload="onLoad();">
<div id="console"></div>
<div id="container" class="box" style="overflow: scroll;">
<div class="box" style="background: green;"></div>
<div class="box" style="background: red;"></div>
<div class="box" style="background: pink;"></div>
<div class="box" style="background: yellow;"></div>
<div class="box" style="background: orange;"></div>
</div>
</body>
</html>
@@ -134,6 +134,8 @@ js/dom/dfg-inline-resolve.html
scrollbars/corner-resizer-window-inactive.html [ ImageOnlyFailure ]
scrollbars/scrolling-by-page-on-keyboard-spacebar.html [ Failure ]
scrollbars/scrollbars-on-positioned-content.html [ Failure ]
css3/scroll-snap/scroll-padding-mainframe-paging.html [ Failure ]
css3/scroll-snap/scroll-padding-overflow-paging.html [ Failure ]

# SVG tests that time out (these require EventSender)
svg/animations/animVal-basics.html
@@ -448,8 +448,9 @@ storage/indexeddb/modern/exceed-open-file-limit.html
imported/w3c/web-platform-tests/cookies/secure/set-from-wss.https.sub.html [ Failure ]
imported/w3c/web-platform-tests/cookies/secure/set-from-ws.sub.html [ Failure ]

# WebKit1 frames use native scrollbars causing this reference test to fail.
# WebKit1 frames use native scrollbars causing these tests to fail.
imported/w3c/web-platform-tests/css/css-scroll-snap/scroll-target-padding-001.html [ ImageOnlyFailure ]
css3/scroll-snap/scroll-padding-mainframe-paging.html [ Failure ]

### END OF (2) Failures without bug reports
########################################
@@ -1,3 +1,46 @@
2021-01-25 Martin Robinson <mrobinson@igalia.com>

scroll-padding should affect paging operations
https://bugs.webkit.org/show_bug.cgi?id=219074
<rdar://problem/71747786>

Reviewed by Simon Fraser.

Have scroll-padding affect the amount of the scrollable area that moves during
paging operations. This is the behavior specified in the scroll snap specification
and allows scrollable areas with partially obscured areas to properly page through
their content.

See https://drafts.csswg.org/css-scroll-snap-1/#propdef-scroll-padding.

Tests: css3/scroll-snap/scroll-padding-mainframe-paging.html
css3/scroll-snap/scroll-padding-overflow-paging.html

* page/FrameView.cpp:
(WebCore::FrameView::updateScrollbarSteps): Added this override method which
properly sets page steps. Only FrameView has access to the document.
* page/FrameView.h:
* platform/ScrollView.cpp:
(WebCore::ScrollView::updateScrollbars): Added this helper method, which is
virtual so that FrameView can override it.
(WebCore::ScrollView::updateScrollbarSteps): Use the helper method to actually
set the scrollbar steps.
* platform/ScrollView.h: Add new method declarations.
* rendering/RenderBox.cpp:
(WebCore::RenderBox::scrollPaddingForViewportRect): Add this new helper which
gets the scroll-padding for a box.
* rendering/RenderBox.h: Add new helper.
* rendering/RenderLayer.cpp:
(WebCore::expandScrollRectToVisibleTargetRectToIncludeScrollPadding): Use the new
RenderBox helper.
(WebCore::RenderLayer::updateScrollbarsAfterLayout): Use the new updateScrollbarSteps helper.
(WebCore::RenderLayer::updateScrollbarSteps): Added this helper so that RenderLayerModelObject
can update steps when necessary.
* rendering/RenderLayer.h: Added new declarations.
* rendering/RenderLayerModelObject.cpp:
(WebCore::RenderLayerModelObject::styleDidChange): Update steps on FrameViews and RenderLayers
when the style change dictates it.

2021-01-25 Carlos Garcia Campos <cgarcia@igalia.com>

Null Ptr Deref @ WebCore::ReplaceSelectionCommand::doApply
@@ -5544,6 +5544,28 @@ float FrameView::pageScaleFactor() const
return frame().frameScaleFactor();
}

void FrameView::updateScrollbarSteps()
{
auto* documentElement = frame().document() ? frame().document()->documentElement() : nullptr;
auto* renderer = documentElement ? documentElement->renderBox() : nullptr;
if (!renderer) {
ScrollView::updateScrollbarSteps();
return;
}

LayoutRect paddedViewRect(LayoutPoint(), visibleSize());
paddedViewRect.contract(renderer->scrollPaddingForViewportRect(paddedViewRect));

if (horizontalScrollbar()) {
int pageStep = Scrollbar::pageStep(paddedViewRect.width());
horizontalScrollbar()->setSteps(Scrollbar::pixelsPerLineStep(), pageStep);

}
if (verticalScrollbar()) {
int pageStep = Scrollbar::pageStep(paddedViewRect.height());
verticalScrollbar()->setSteps(Scrollbar::pixelsPerLineStep(), pageStep);
}
}
} // namespace WebCore

#undef PAGE_ID
@@ -680,6 +680,9 @@ class FrameView final : public ScrollView {

String debugDescription() const final;

// ScrollView
void updateScrollbarSteps() override;

private:
explicit FrameView(Frame&);

@@ -731,7 +731,6 @@ void ScrollView::updateScrollbars(const ScrollPosition& desiredPosition)

if (m_horizontalScrollbar) {
int clientWidth = visibleWidth();
int pageStep = Scrollbar::pageStep(clientWidth);
IntRect oldRect(m_horizontalScrollbar->frameRect());
IntRect hBarRect(shouldPlaceBlockDirectionScrollbarOnLeft() && m_verticalScrollbar ? m_verticalScrollbar->occupiedWidth() : 0,
height() - m_horizontalScrollbar->height(),
@@ -744,15 +743,13 @@ void ScrollView::updateScrollbars(const ScrollPosition& desiredPosition)
if (m_scrollbarsSuppressed)
m_horizontalScrollbar->setSuppressInvalidation(true);
m_horizontalScrollbar->setEnabled(contentsWidth() > clientWidth);
m_horizontalScrollbar->setSteps(Scrollbar::pixelsPerLineStep(), pageStep);
m_horizontalScrollbar->setProportion(clientWidth, contentsWidth());
if (m_scrollbarsSuppressed)
m_horizontalScrollbar->setSuppressInvalidation(false);
}

if (m_verticalScrollbar) {
int clientHeight = visibleHeight();
int pageStep = Scrollbar::pageStep(clientHeight);
IntRect oldRect(m_verticalScrollbar->frameRect());
IntRect vBarRect(shouldPlaceBlockDirectionScrollbarOnLeft() ? 0 : width() - m_verticalScrollbar->width(),
topContentInset(),
@@ -765,12 +762,13 @@ void ScrollView::updateScrollbars(const ScrollPosition& desiredPosition)
if (m_scrollbarsSuppressed)
m_verticalScrollbar->setSuppressInvalidation(true);
m_verticalScrollbar->setEnabled(totalContentsSize().height() > clientHeight);
m_verticalScrollbar->setSteps(Scrollbar::pixelsPerLineStep(), pageStep);
m_verticalScrollbar->setProportion(clientHeight, totalContentsSize().height());
if (m_scrollbarsSuppressed)
m_verticalScrollbar->setSuppressInvalidation(false);
}

updateScrollbarSteps();

if (hasHorizontalScrollbar != newHasHorizontalScrollbar || hasVerticalScrollbar != newHasVerticalScrollbar) {
// FIXME: Is frameRectsChanged really necessary here? Have any frame rects changed?
frameRectsChanged();
@@ -791,6 +789,14 @@ void ScrollView::updateScrollbars(const ScrollPosition& desiredPosition)
m_inUpdateScrollbars = false;
}

void ScrollView::updateScrollbarSteps()
{
if (m_horizontalScrollbar)
m_horizontalScrollbar->setSteps(Scrollbar::pixelsPerLineStep(), Scrollbar::pageStep(visibleWidth()));
if (m_verticalScrollbar)
m_verticalScrollbar->setSteps(Scrollbar::pixelsPerLineStep(), Scrollbar::pageStep(visibleHeight()));
}

const int panIconSizeLength = 16;

IntRect ScrollView::rectToCopyOnScroll() const
@@ -397,6 +397,7 @@ class ScrollView : public Widget, public ScrollableArea {
bool allowsUnclampedScrollPosition() const { return m_allowsUnclampedScrollPosition; }

bool managesScrollbars() const;
virtual void updateScrollbarSteps();

protected:
ScrollView();
@@ -5162,4 +5162,12 @@ LayoutRect RenderBox::absoluteAnchorRectWithScrollMargin(bool* insideFixed) cons
return anchorRect;
}

LayoutBoxExtent RenderBox::scrollPaddingForViewportRect(const LayoutRect& viewportRect)
{
const auto& padding = style().scrollPadding();
return LayoutBoxExtent(
valueForLength(padding.top(), viewportRect.height()), valueForLength(padding.right(), viewportRect.width()),
valueForLength(padding.bottom(), viewportRect.height()), valueForLength(padding.left(), viewportRect.width()));
}

} // namespace WebCore

0 comments on commit 81c33b1

Please sign in to comment.