scroll-snap-destination and scroll-snap-coordinate do not seem to work together properly
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Mar 2015 16:57:06 +0000 (16:57 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Mar 2015 16:57:06 +0000 (16:57 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181855 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/css/CSSParser.cpp
Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp

index 7e36bd3..6175880 100644 (file)
@@ -1,3 +1,30 @@
+2015-03-23  Brent Fulgham  <bfulgham@apple.com>
+
+        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  <yoav@yoav.ws>
 
         Refactor ImageLoader's setting of CachedImage
index 0d67423..506606e 100644 (file)
@@ -3353,6 +3353,7 @@ bool CSSParser::parseScrollSnapPositions(RefPtr<CSSValue>& cssValueX, RefPtr<CSS
 bool CSSParser::parseScrollSnapDestination(CSSPropertyID propId, bool important)
 {
     RefPtr<CSSValueList> position = CSSValueList::createSpaceSeparated();
+    ASSERT(m_valueList->size() == 2);
     if (m_valueList->size() != 2)
         return false;
 
index 02db439..0b2bd4e 100644 (file)
@@ -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 void appendChildSnapOffsets(HTMLElement& parent, bool shouldAddHorizontal
     }
 }
 
+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 @@ static void updateFromStyle(Vector<LayoutUnit>& snapOffsets, const RenderStyle&
             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 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, HTMLElem
     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;