Bug 126054 – REGRESSION: cnn.com will continue to reveal 1 px of overhang after rubber-banding at the top

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

http://trac.webkit.org/changeset/160791 caused this regression. Patch forthcoming.

[Radar WebKit Bug Importer]

<rdar://problem/15706328>

[Beth Dakin]

Created attachment 219731 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 219731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219731&action=review

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:287
> +    float nearestXWithinBounds = std::max<float>(std::min<float>(currentScrollPosition.x(), maxPosition.x()), minPosition.x());
> +    float nearestYWithinBounds = std::max<float>(std::min<float>(currentScrollPosition.y(), maxPosition.y()), minPosition.y());

Not sure why you're doing this in floats (yet).

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:739
> +    bool currentlyConstrainsToContentEdge = m_scrollableArea->constrainsScrollingToContentEdge();
> +    m_scrollableArea->setConstrainsScrollingToContentEdge(true);

We can't use TemporaryChange<> here?

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 219731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219731&action=review
> 
> > Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:287
> > +    float nearestXWithinBounds = std::max<float>(std::min<float>(currentScrollPosition.x(), maxPosition.x()), minPosition.x());
> > +    float nearestYWithinBounds = std::max<float>(std::min<float>(currentScrollPosition.y(), maxPosition.y()), minPosition.y());
> 
> Not sure why you're doing this in floats (yet).
> 

Will change to ints.

> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:739
> > +    bool currentlyConstrainsToContentEdge = m_scrollableArea->constrainsScrollingToContentEdge();
> > +    m_scrollableArea->setConstrainsScrollingToContentEdge(true);
> 
> We can't use TemporaryChange<> here?

I don't think we can because the bool belongs to ScrollableArea and this is ScrollAnimator. I think that ScrollAnimator would need a direct reference to the variable to use a TemporaryChange<>. Let me know if I am wrong though! I would be happy to fix it in a follow-up patch.

[Beth Dakin]

http://trac.webkit.org/changeset/160898