Modified: trunk/Source/WebCore/ChangeLog (181854 => 181855)
--- trunk/Source/WebCore/ChangeLog 2015-03-23 11:47:11 UTC (rev 181854)
+++ trunk/Source/WebCore/ChangeLog 2015-03-23 16:57:06 UTC (rev 181855)
@@ -1,3 +1,30 @@
+2015-03-23 Brent Fulgham <[email protected]>
+
+ scroll-snap-destination and scroll-snap-coordinate do not seem to work together properly
+ https://bugs.webkit.org/show_bug.cgi?id=142552
+ <rdar://problem/20114743>
+
+ Reviewed by Dean Jackson.
+
+ Revise the snap point logic as follows:
+ (1) Put the snap point destination handling in a helper function to make the rest of the code
+ easier to read.
+ (2) Make sure we always have a left-hand snap point (i.e., position 0), but don't add multiple
+ left-hand snap points.
+ (3) Create a helper function to determine if we should be working with the scroll snap 'elements'
+ behavior. We want to use this for scroll-snap-destination/scroll-snap-coordinate markup.
+ (4) Create per-element snap point offsets when using scroll-snap-destination/scroll-snap-coordinate.
+
+ * css/CSSParser.cpp:
+ (WebCore::CSSParser::parseScrollSnapDestination): Add assertion to try to catch bad parser state.
+ * page/scrolling/AxisScrollSnapOffsets.cpp:
+ (WebCore::destinationOffsetForViewSize): Added helper function to consolidate logic for handling
+ destination coordinates.
+ (WebCore::updateFromStyle): Make sure a left-hand snap point is always provided.
+ (WebCore::styleUsesElements): Added helper function.
+ (WebCore::updateSnapOffsetsForScrollableArea): Revise logic to generate 'per-element' snap point
+ offsets.
+
2015-03-23 Yoav Weiss <[email protected]>
Refactor ImageLoader's setting of CachedImage
Modified: trunk/Source/WebCore/css/CSSParser.cpp (181854 => 181855)
--- trunk/Source/WebCore/css/CSSParser.cpp 2015-03-23 11:47:11 UTC (rev 181854)
+++ trunk/Source/WebCore/css/CSSParser.cpp 2015-03-23 16:57:06 UTC (rev 181855)
@@ -3353,6 +3353,7 @@
bool CSSParser::parseScrollSnapDestination(CSSPropertyID propId, bool important)
{
RefPtr<CSSValueList> position = CSSValueList::createSpaceSeparated();
+ ASSERT(m_valueList->size() == 2);
if (m_valueList->size() != 2)
return false;
Modified: trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp (181854 => 181855)
--- trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp 2015-03-23 11:47:11 UTC (rev 181854)
+++ trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp 2015-03-23 16:57:06 UTC (rev 181855)
@@ -29,6 +29,7 @@
#include "ElementChildIterator.h"
#include "HTMLCollection.h"
#include "HTMLElement.h"
+#include "Length.h"
#include "RenderBox.h"
#include "ScrollableArea.h"
#include "StyleScrollSnapPoints.h"
@@ -68,18 +69,25 @@
}
}
+static LayoutUnit destinationOffsetForViewSize(ScrollEventAxis axis, const LengthSize& destination, LayoutUnit viewSize)
+{
+ const Length& dimension = (axis == ScrollEventAxis::Horizontal) ? destination.width() : destination.height();
+ return valueForLength(dimension, viewSize);
+}
+
static void updateFromStyle(Vector<LayoutUnit>& snapOffsets, const RenderStyle& style, ScrollEventAxis axis, LayoutUnit viewSize, LayoutUnit scrollSize, Vector<LayoutUnit>& snapOffsetSubsequence)
{
std::sort(snapOffsetSubsequence.begin(), snapOffsetSubsequence.end());
if (snapOffsetSubsequence.isEmpty())
snapOffsetSubsequence.append(0);
- bool isHorizontalAxis = axis == ScrollEventAxis::Horizontal;
- auto* points = isHorizontalAxis ? style.scrollSnapPointsX() : style.scrollSnapPointsY();
- auto& destination = style.scrollSnapDestination();
+ // Always put a snap point on the zero offset.
+ snapOffsets.append(0);
+
+ auto* points = (axis == ScrollEventAxis::Horizontal) ? style.scrollSnapPointsX() : style.scrollSnapPointsY();
bool hasRepeat = points ? points->hasRepeat : false;
LayoutUnit repeatOffset = points ? valueForLength(points->repeatOffset, viewSize) : LayoutUnit();
- LayoutUnit destinationOffset = valueForLength(isHorizontalAxis ? destination.width() : destination.height(), viewSize);
+ LayoutUnit destinationOffset = destinationOffsetForViewSize(axis, style.scrollSnapDestination(), viewSize);
LayoutUnit curSnapPositionShift = 0;
LayoutUnit maxScrollOffset = scrollSize - viewSize;
LayoutUnit lastSnapPosition = curSnapPositionShift;
@@ -92,21 +100,32 @@
if (potentialSnapPosition >= maxScrollOffset)
break;
- snapOffsets.append(potentialSnapPosition);
+ // Don't add another zero offset value.
+ if (potentialSnapPosition)
+ snapOffsets.append(potentialSnapPosition);
+
lastSnapPosition = potentialSnapPosition + destinationOffset;
}
curSnapPositionShift = lastSnapPosition + repeatOffset;
} while (hasRepeat && curSnapPositionShift < maxScrollOffset);
- if (snapOffsets.isEmpty())
- snapOffsets.append(0);
-
// Always put a snap point on the maximum scroll offset.
// Not a part of the spec, but necessary to prevent unreachable content when snapping.
if (snapOffsets.last() != maxScrollOffset)
snapOffsets.append(maxScrollOffset);
}
+static bool styleUsesElements(ScrollEventAxis axis, const RenderStyle& style)
+{
+ const ScrollSnapPoints* scrollSnapPoints = (axis == ScrollEventAxis::Horizontal) ? style.scrollSnapPointsX() : style.scrollSnapPointsY();
+ if (scrollSnapPoints)
+ return scrollSnapPoints->usesElements;
+
+ const Length& destination = (axis == ScrollEventAxis::Horizontal) ? style.scrollSnapDestination().width() : style.scrollSnapDestination().height();
+
+ return !destination.isUndefined();
+}
+
void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, HTMLElement& scrollingElement, const RenderBox& scrollingElementBox, const RenderStyle& scrollingElementStyle)
{
if (scrollingElementStyle.scrollSnapType() == ScrollSnapType::None) {
@@ -133,8 +152,8 @@
Vector<LayoutUnit> horizontalSnapOffsetSubsequence;
Vector<LayoutUnit> verticalSnapOffsetSubsequence;
- bool scrollSnapPointsXUsesElements = scrollingElementStyle.scrollSnapPointsX() ? scrollingElementStyle.scrollSnapPointsX()->usesElements : false;
- bool scrollSnapPointsYUsesElements = scrollingElementStyle.scrollSnapPointsY() ? scrollingElementStyle.scrollSnapPointsY()->usesElements : false;
+ bool scrollSnapPointsXUsesElements = styleUsesElements(ScrollEventAxis::Horizontal, scrollingElementStyle);
+ bool scrollSnapPointsYUsesElements = styleUsesElements(ScrollEventAxis::Vertical , scrollingElementStyle);
if (scrollSnapPointsXUsesElements || scrollSnapPointsYUsesElements) {
bool shouldAddHorizontalChildOffsets = scrollSnapPointsXUsesElements && canComputeHorizontalOffsets;
