Changeset 181855 in webkit

Timestamp:

Mar 23, 2015, 9:57:06 AM (11 years ago)

Author:

Brent Fulgham

Message:

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/CSSParser.cpp (modified) (1 diff)
• page/scrolling/AxisScrollSnapOffsets.cpp (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/css/CSSParser.cpp

@@ -3354 +3354 @@
 {
     RefPtr<CSSValueList> position = CSSValueList::createSpaceSeparated();
+    ASSERT(m_valueList->size() == 2);
     if (m_valueList->size() != 2)
         return false;

trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp

@@ -30 +30 @@
 #include "HTMLCollection.h"
 #include "HTMLElement.h"
+#include "Length.h"
 #include "RenderBox.h"
 #include "ScrollableArea.h"
@@ -69 +70 @@
 }
 
+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)
 {
@@ -75 +82 @@
         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;
@@ -93 +101 @@
                 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.
@@ -108 +116 @@
 }
 
+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)
 {
@@ -134 +153 @@
     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) {