Skip to content
Permalink
Browse files
Some cleanup in ScrollAnimator
https://bugs.webkit.org/show_bug.cgi?id=152649

Reviewed by Zalan Bujtas.
Source/WebCore:

Change ScrollAnimatorMac::adjustScrollPositionIfNecessary() and similar code in
ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary() to
constrain between minimumScrollPosition() and maximumScrollPosition(), rather than
rolling their own code.

This revealed several issues. First, RenderLayer::maximumScrollPosition() is
wrong when the layer has borders, because RenderLayer::visibleContentRectInternal()
seems to have incorrect logic. However, we can just remove it, and use the ScrollableArea
implementation.

Second, ScrollAnimatorMac::scrollToOffsetWithoutAnimation() was failing to do a
position/offset conversion, so do one. We're converting too much, and should probably
just change ScrollAnimator to do everything in terms of positions.

Third, ScrollAnimator::scroll() was clamping a scroll position as an offset
(detected by scrollbars/scroll-rtl-or-bt-layer.html), so fix that.

Remove ScrollController::absoluteScrollPosition() and overrides, since this was
confusingly named, and could just be removed.

Remove ScrollController::m_origOrigin which was assigned to, but never read.

Test: fast/scrolling/arrow-key-scroll-in-rtl-document.html: new
      fast/dom/horizontal-scrollbar-in-rtl.html: progressed with these changes.

* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:
* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
(WebCore::ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary):
(WebCore::ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition): Deleted.
* platform/ScrollAnimator.cpp:
(WebCore::ScrollAnimator::scroll):
(WebCore::ScrollAnimator::notifyPositionChanged):
* platform/ScrollableArea.h:
(WebCore::ScrollableArea::constrainScrollPosition):
* platform/cocoa/ScrollController.h:
* platform/cocoa/ScrollController.mm:
(WebCore::ScrollController::snapRubberBandTimerFired): Deleted.
(WebCore::ScrollController::snapRubberBand): Deleted.
* platform/mac/ScrollAnimatorMac.h:
* platform/mac/ScrollAnimatorMac.mm:
(-[WebScrollAnimationHelperDelegate _immediateScrollToPoint:]):
(WebCore::ScrollAnimatorMac::scroll):
(WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation):
(WebCore::ScrollAnimatorMac::adjustScrollPositionIfNecessary):
(WebCore::ScrollAnimatorMac::adjustScrollPositionToBoundsIfNecessary):
(WebCore::ScrollAnimatorMac::immediateScrollToPosition):
(WebCore::ScrollAnimatorMac::immediateScrollToPositionForScrollAnimation):
(WebCore::ScrollAnimatorMac::immediateScrollTo): Deleted.
(WebCore::ScrollAnimatorMac::immediateScrollToPointForScrollAnimation): Deleted.
(WebCore::ScrollAnimatorMac::absoluteScrollPosition): Deleted.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::visibleContentRectInternal):
(WebCore::RenderLayer::overhangAmount):
(WebCore::RenderLayer::maximumScrollPosition): Deleted.
* rendering/RenderLayer.h:
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::minimumScrollPosition):
(WebCore::RenderListBox::maximumScrollPosition): RenderListBox scrolls by lines,
so it needs a custom implementation of this.
* rendering/RenderListBox.h:

LayoutTests:

Added fast/scrolling/arrow-key-scroll-in-rtl-document.html to test for arrow
key scrolling in an RTL document, which an earlier version of the patch
regressed without detection.

* fast/dom/horizontal-scrollbar-in-rtl-expected.txt:
* fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt: Added.
* fast/scrolling/arrow-key-scroll-in-rtl-document.html: Added.

Canonical link: https://commits.webkit.org/170740@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@194502 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
smfr committed Jan 2, 2016
1 parent d8b65e3 commit 30551c2a7252334e9342953208468231df4bdf34
@@ -1,3 +1,18 @@
2016-01-02 Simon Fraser <simon.fraser@apple.com>

Some cleanup in ScrollAnimator
https://bugs.webkit.org/show_bug.cgi?id=152649

Reviewed by Zalan Bujtas.

Added fast/scrolling/arrow-key-scroll-in-rtl-document.html to test for arrow
key scrolling in an RTL document, which an earlier version of the patch
regressed without detection.

* fast/dom/horizontal-scrollbar-in-rtl-expected.txt:
* fast/scrolling/arrow-key-scroll-in-rtl-document-expected.txt: Added.
* fast/scrolling/arrow-key-scroll-in-rtl-document.html: Added.

2016-01-02 Zalan Bujtas <zalan@apple.com>

Simple line layout:: Add text-decoration support.
@@ -3,6 +3,6 @@ horizontal scroll: : Success
continuously call window.scrollX : Success
zoom in and out preserve scroll position: Success
resize preserves scroll position: Success
KeyDown HOME move x-scroll position to right for RTL page: -1000
KeyDown END move x-scroll position to right for RTL page: -1000
KeyDown HOME move x-scroll position to right for RTL page: 0
KeyDown END move x-scroll position to right for RTL page: 0
selectAll selects all document: Success
@@ -0,0 +1 @@
PASS: scrollLeft is -120
@@ -0,0 +1,58 @@
<!DOCTYPE html>

<html dir="rtl">
<head>
<style>
.wide {
width: 2000px;
height: 10px;
background-color: silver;
}
.origin {
position: absolute;
top: 0;
left: 0;
width: 10px;
height: 10px;
background-color: blue;
}
</style>
<script>
if (window.testRunner) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
}

function checkForScroll()
{
var expectedScrollLeft = -120;
var scrollLeft = document.scrollingElement.scrollLeft;
if (scrollLeft != expectedScrollLeft)
document.getElementById('result').textContent = "FAIL: scrollLeft is " + scrollLeft + ", expected " + expectedScrollLeft;
else
document.getElementById('result').textContent = "PASS: scrollLeft is " + scrollLeft;

if (window.testRunner)
testRunner.notifyDone();
}

function doTest()
{
if (window.eventSender) {
eventSender.monitorWheelEvents();
eventSender.keyDown("leftArrow");
eventSender.keyDown("leftArrow");
eventSender.keyDown("leftArrow");
eventSender.callAfterScrollingCompletes(checkForScroll);
}
}

window.addEventListener('load', doTest, false);
</script>
</head>
<body>
<div class="wide"></div>
<div class="origin"></div>
<div id="result"></div>
</body>
</html>
@@ -1,3 +1,71 @@
2016-01-02 Simon Fraser <simon.fraser@apple.com>

Some cleanup in ScrollAnimator
https://bugs.webkit.org/show_bug.cgi?id=152649

Reviewed by Zalan Bujtas.

Change ScrollAnimatorMac::adjustScrollPositionIfNecessary() and similar code in
ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary() to
constrain between minimumScrollPosition() and maximumScrollPosition(), rather than
rolling their own code.

This revealed several issues. First, RenderLayer::maximumScrollPosition() is
wrong when the layer has borders, because RenderLayer::visibleContentRectInternal()
seems to have incorrect logic. However, we can just remove it, and use the ScrollableArea
implementation.

Second, ScrollAnimatorMac::scrollToOffsetWithoutAnimation() was failing to do a
position/offset conversion, so do one. We're converting too much, and should probably
just change ScrollAnimator to do everything in terms of positions.

Third, ScrollAnimator::scroll() was clamping a scroll position as an offset
(detected by scrollbars/scroll-rtl-or-bt-layer.html), so fix that.

Remove ScrollController::absoluteScrollPosition() and overrides, since this was
confusingly named, and could just be removed.

Remove ScrollController::m_origOrigin which was assigned to, but never read.

Test: fast/scrolling/arrow-key-scroll-in-rtl-document.html: new
fast/dom/horizontal-scrollbar-in-rtl.html: progressed with these changes.

* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.h:
* page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:
(WebCore::ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary):
(WebCore::ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition): Deleted.
* platform/ScrollAnimator.cpp:
(WebCore::ScrollAnimator::scroll):
(WebCore::ScrollAnimator::notifyPositionChanged):
* platform/ScrollableArea.h:
(WebCore::ScrollableArea::constrainScrollPosition):
* platform/cocoa/ScrollController.h:
* platform/cocoa/ScrollController.mm:
(WebCore::ScrollController::snapRubberBandTimerFired): Deleted.
(WebCore::ScrollController::snapRubberBand): Deleted.
* platform/mac/ScrollAnimatorMac.h:
* platform/mac/ScrollAnimatorMac.mm:
(-[WebScrollAnimationHelperDelegate _immediateScrollToPoint:]):
(WebCore::ScrollAnimatorMac::scroll):
(WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation):
(WebCore::ScrollAnimatorMac::adjustScrollPositionIfNecessary):
(WebCore::ScrollAnimatorMac::adjustScrollPositionToBoundsIfNecessary):
(WebCore::ScrollAnimatorMac::immediateScrollToPosition):
(WebCore::ScrollAnimatorMac::immediateScrollToPositionForScrollAnimation):
(WebCore::ScrollAnimatorMac::immediateScrollTo): Deleted.
(WebCore::ScrollAnimatorMac::immediateScrollToPointForScrollAnimation): Deleted.
(WebCore::ScrollAnimatorMac::absoluteScrollPosition): Deleted.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::visibleContentRectInternal):
(WebCore::RenderLayer::overhangAmount):
(WebCore::RenderLayer::maximumScrollPosition): Deleted.
* rendering/RenderLayer.h:
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::minimumScrollPosition):
(WebCore::RenderListBox::maximumScrollPosition): RenderListBox scrolls by lines,
so it needs a custom implementation of this.
* rendering/RenderListBox.h:

2016-01-02 Zalan Bujtas <zalan@apple.com>

Simple line layout:: Add text-decoration support.
@@ -58,7 +58,6 @@ class ScrollingTreeFrameScrollingNodeMac : public ScrollingTreeFrameScrollingNod
bool canScrollHorizontally() override;
bool canScrollVertically() override;
bool shouldRubberBandInDirection(ScrollDirection) override;
IntPoint absoluteScrollPosition() override;
void immediateScrollBy(const FloatSize&) override;
void immediateScrollByWithoutContentEdgeConstraints(const FloatSize&) override;
void stopSnapRubberbandTimer() override;
@@ -325,11 +325,6 @@ static bool newGestureIsStarting(const PlatformWheelEvent& wheelEvent)
return true;
}

IntPoint ScrollingTreeFrameScrollingNodeMac::absoluteScrollPosition()
{
return roundedIntPoint(scrollPosition());
}

void ScrollingTreeFrameScrollingNodeMac::immediateScrollBy(const FloatSize& delta)
{
scrollBy(delta);
@@ -350,15 +345,9 @@ static bool newGestureIsStarting(const PlatformWheelEvent& wheelEvent)

void ScrollingTreeFrameScrollingNodeMac::adjustScrollPositionToBoundsIfNecessary()
{
FloatPoint currentScrollPosition = absoluteScrollPosition();
FloatPoint minPosition = minimumScrollPosition();
FloatPoint maxPosition = maximumScrollPosition();

float nearestXWithinBounds = std::max(std::min(currentScrollPosition.x(), maxPosition.x()), minPosition.x());
float nearestYWithinBounds = std::max(std::min(currentScrollPosition.y(), maxPosition.y()), minPosition.y());

FloatPoint nearestPointWithinBounds(nearestXWithinBounds, nearestYWithinBounds);
immediateScrollBy(nearestPointWithinBounds - currentScrollPosition);
FloatPoint currentScrollPosition = scrollPosition();
FloatPoint constainedPosition = currentScrollPosition.constrainedBetween(minimumScrollPosition(), maximumScrollPosition());
immediateScrollBy(constainedPosition - currentScrollPosition);
}

FloatPoint ScrollingTreeFrameScrollingNodeMac::scrollPosition() const
@@ -63,15 +63,22 @@ ScrollAnimator::~ScrollAnimator()

bool ScrollAnimator::scroll(ScrollbarOrientation orientation, ScrollGranularity, float step, float multiplier)
{
float* currentPos = (orientation == HorizontalScrollbar) ? &m_currentPosX : &m_currentPosY;
float newPos = std::max(std::min(*currentPos + (step * multiplier), static_cast<float>(m_scrollableArea.scrollSize(orientation))), 0.0f);
float delta = *currentPos - newPos;
if (*currentPos == newPos)
FloatPoint currentPosition(m_currentPosX, m_currentPosY);
FloatSize delta;
if (orientation == HorizontalScrollbar)
delta.setWidth(step * multiplier);
else
delta.setHeight(step * multiplier);

FloatPoint newPosition = FloatPoint(currentPosition + delta).constrainedBetween(m_scrollableArea.minimumScrollPosition(), m_scrollableArea.maximumScrollPosition());
if (currentPosition == newPosition)
return false;
*currentPos = newPos;

notifyPositionChanged(orientation == HorizontalScrollbar ? FloatSize(delta, 0) : FloatSize(0, delta));
// FIXME: m_currentPosX/Y should just be a FloatPoint.
m_currentPosX = newPosition.x();
m_currentPosY = newPosition.y();

notifyPositionChanged(newPosition - currentPosition);
return true;
}

@@ -194,7 +201,8 @@ void ScrollAnimator::updateActiveScrollSnapIndexForOffset()
void ScrollAnimator::notifyPositionChanged(const FloatSize& delta)
{
UNUSED_PARAM(delta);
m_scrollableArea.setScrollOffsetFromAnimation(IntPoint(m_currentPosX, m_currentPosY));
// FIXME: need to not map back and forth all the time.
m_scrollableArea.setScrollOffsetFromAnimation(m_scrollableArea.scrollOffsetFromPosition(IntPoint(m_currentPosX, m_currentPosY)));
}

#if ENABLE(CSS_SCROLL_SNAP)
@@ -188,6 +188,11 @@ class ScrollableArea {
virtual ScrollPosition minimumScrollPosition() const;
virtual ScrollPosition maximumScrollPosition() const;

ScrollPosition constrainScrollPosition(const ScrollPosition& position) const
{
return position.constrainedBetween(minimumScrollPosition(), maximumScrollPosition());
}

ScrollOffset maximumScrollOffset() const;

WEBCORE_EXPORT ScrollPosition scrollPositionFromOffset(ScrollOffset) const;
@@ -60,9 +60,6 @@ class ScrollControllerClient {
virtual bool canScrollVertically() = 0;
virtual bool shouldRubberBandInDirection(ScrollDirection) = 0;

// Return the absolute scroll position, not relative to the scroll origin.
virtual WebCore::IntPoint absoluteScrollPosition() = 0;

virtual void immediateScrollBy(const FloatSize&) = 0;
virtual void immediateScrollByWithoutContentEdgeConstraints(const FloatSize&) = 0;
virtual void startSnapRubberbandTimer()
@@ -179,7 +176,6 @@ class ScrollController {
// Rubber band state.
CFTimeInterval m_startTime { 0 };
FloatSize m_startStretch;
FloatPoint m_origOrigin;
FloatSize m_origVelocity;
RunLoop::Timer<ScrollController> m_snapRubberbandTimer;
#endif
@@ -349,12 +349,10 @@ static inline float roundToDevicePixelTowardZero(float num)
m_stretchScrollForce = FloatSize();
m_startTime = 0;
m_startStretch = FloatSize();
m_origOrigin = FloatPoint();
m_origVelocity = FloatSize();
return;
}

m_origOrigin = m_client.absoluteScrollPosition() - m_startStretch;
m_origVelocity = m_momentumVelocity;

// Just like normal scrolling, prefer vertical rubberbanding
@@ -387,7 +385,6 @@ FloatPoint delta(roundToDevicePixelTowardZero(elasticDeltaForTimeDelta(m_startSt
m_stretchScrollForce = FloatSize();
m_startTime = 0;
m_startStretch = FloatSize();
m_origOrigin = FloatPoint();
m_origVelocity = FloatSize();
}
} else {
@@ -451,7 +448,6 @@ FloatPoint delta(roundToDevicePixelTowardZero(elasticDeltaForTimeDelta(m_startSt

m_startTime = [NSDate timeIntervalSinceReferenceDate];
m_startStretch = FloatSize();
m_origOrigin = FloatPoint();
m_origVelocity = FloatSize();

startSnapRubberbandTimer();
@@ -52,7 +52,7 @@ class ScrollAnimatorMac : public ScrollAnimator {
ScrollAnimatorMac(ScrollableArea&);
virtual ~ScrollAnimatorMac();

void immediateScrollToPointForScrollAnimation(const FloatPoint& newPosition);
void immediateScrollToPositionForScrollAnimation(const FloatPoint& newPosition);
bool haveScrolledSincePageLoad() const { return m_haveScrolledSincePageLoad; }

void updateScrollerStyle();
@@ -134,7 +134,7 @@ class ScrollAnimatorMac : public ScrollAnimator {

FloatPoint adjustScrollPositionIfNecessary(const FloatPoint&) const;

void immediateScrollTo(const FloatPoint&);
void immediateScrollToPosition(const FloatPoint&);

bool isRubberBandInProgress() const override;
bool isScrollSnapInProgress() const override;
@@ -148,7 +148,6 @@ class ScrollAnimatorMac : public ScrollAnimator {
virtual bool canScrollHorizontally() override;
virtual bool canScrollVertically() override;
virtual bool shouldRubberBandInDirection(ScrollDirection) override;
virtual WebCore::IntPoint absoluteScrollPosition() override;
virtual void immediateScrollByWithoutContentEdgeConstraints(const FloatSize&) override;
virtual void immediateScrollBy(const FloatSize&) override;
virtual void adjustScrollPositionToBoundsIfNecessary() override;

0 comments on commit 30551c2

Please sign in to comment.