Bug 144647 – Scroll-snap points do not handle margins and padding properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

If we decorate the container holding a scroll-snap region with padding or borders, the calculation of the snap points is not correct. See the attached example for details:

1. The initial color block fills the region and looks correct.
2. After performing a single swipe (which should move 100% to the next color block), we are off by about 2x the border width, resulting in display of the third color at the edge of the display region.

[Brent Fulgham]

Created attachment 252422 [details]
border example

[Brent Fulgham]

Created attachment 252423 [details]
padding example

[Radar WebKit Bug Importer]

<rdar://problem/20829473>

[Brent Fulgham]

Created attachment 252492 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 252492 [details]
Patch

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

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:43
> +    LayoutUnit adjustedSize = viewSize;

"size" sounds like a LayoutSize. Why not just pass a LayoutSize and pick out the right axis?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:50
> +    if (axis == ScrollEventAxis::Horizontal) {
> +        adjustedSize -= valueForLength(style.paddingLeft(), viewSize);
> +        adjustedSize -= valueForLength(style.paddingRight(), viewSize);
> +    } else {
> +        adjustedSize -= valueForLength(style.paddingTop(), viewSize);
> +        adjustedSize -= valueForLength(style.paddingBottom(), viewSize);

Isn't this just what contentBoxRect() would give you?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:59
>      for (auto& child : childrenOfType<Element>(parent)) {

Wait, why are we iterating through elements here?

[Brent Fulgham]

Comment on attachment 252492 [details]
Patch

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

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:50
>> +        adjustedSize -= valueForLength(style.paddingBottom(), viewSize);
> 
> Isn't this just what contentBoxRect() would give you?

Yes! I finally found that after writing this silly code. :-(

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:59
>>      for (auto& child : childrenOfType<Element>(parent)) {
> 
> Wait, why are we iterating through elements here?

Bad code. I'll do a drive-by fix here.

[Brent Fulgham]

Created attachment 252530 [details]
Patch

[Brent Fulgham]

Created attachment 252531 [details]
Patch v3 (Removed debugging code I accidentally uploaded)

[Simon Fraser (smfr)]

Comment on attachment 252531 [details]
Patch v3 (Removed debugging code I accidentally uploaded)

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

> Source/WebCore/testing/Internals.cpp:2814
> +    RenderBox& box = *element->renderBox();
> +    const auto& scrollSnapCoordinates = box.style().scrollSnapCoordinates();
> +    if (scrollSnapCoordinates.isEmpty())
> +        return "none";
> +
> +    LayoutRect viewSize = box.contentBoxRect();
> +    LayoutUnit viewWidth = viewSize.width();
> +    LayoutUnit viewHeight = viewSize.height();
> +    
> +    StringBuilder result;
> +    result.append("{ ");
> +    for (auto& coordinate : scrollSnapCoordinates) {
> +        if (result.length() > 2)
> +            result.append(", ");
> +
> +        LayoutUnit x = valueForLength(coordinate.width(), viewWidth);
> +        LayoutUnit y = valueForLength(coordinate.height(), viewHeight);
> +        result.append("(");
> +        result.append(String::number(x.toUnsigned()));
> +        result.append(", ");
> +        result.append(String::number(y.toUnsigned()));
> +        result.append(")");
> +    }
> +    result.append(" }");

Is this code telling us anything that we don't get from computed style? Use of box.contentBoxRect() here isn't interesting, because this is test-only code.

[Brent Fulgham]

Comment on attachment 252531 [details]
Patch v3 (Removed debugging code I accidentally uploaded)

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

>> Source/WebCore/testing/Internals.cpp:2814
>> +    result.append(" }");
> 
> Is this code telling us anything that we don't get from computed style? Use of box.contentBoxRect() here isn't interesting, because this is test-only code.

OK. I'll get rid of this for now. If I discover a need for it when delving into the corrdinate/destination stuff more thoroughly I'll put it back.

[Brent Fulgham]

Committed r183906: <http://trac.webkit.org/changeset/183906>