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.
Created attachment 252422 [details] border example
Created attachment 252423 [details] padding example
<rdar://problem/20829473>
Created attachment 252492 [details] Patch
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?
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.
Created attachment 252530 [details] Patch
Created attachment 252531 [details] Patch v3 (Removed debugging code I accidentally uploaded)
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.
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.
Committed r183906: <http://trac.webkit.org/changeset/183906>