CSS Motion Path: offset-shorthand requires path before distance rotation

third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp

Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index 5699293aa7e0a0b411da7347ea41060f60caf947..40249ada98d9d535233d4fcd4942500fa97765b6 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -1605,6 +1605,35 @@ CSSValue* consumeOffsetPosition(CSSParserTokenRange& range,
   return consumePosition(range, cssParserMode, UnitlessQuirk::Forbid);
 }
 
+bool CSSPropertyParser::consumeOffsetShorthand(bool important) {

[alancutter (OOO until 2018), 2016/10/04 04:42:15]

Add a comment with the grammar this is implementing.

[Eric Willigers, 2016/10/04 04:59:39]

Done.

+  const CSSValue* offsetPath = consumePathOrNone(m_range);
+  if (!offsetPath)
+    return false;
+
+  const CSSValue* offsetRotation = consumeOffsetRotation(m_range);
+  const CSSValue* offsetDistance =
+      consumeLengthOrPercent(m_range, m_context.mode(), ValueRangeAll);
+  if (offsetDistance && !offsetRotation)
+    offsetRotation = consumeOffsetRotation(m_range);
+
+  addProperty(CSSPropertyOffsetPath, CSSPropertyOffset, *offsetPath, important);
+  addProperty(CSSPropertyOffsetDistance, CSSPropertyOffset,
+              offsetDistance ? *offsetDistance
+                             : *CSSPrimitiveValue::create(
+                                   0, CSSPrimitiveValue::UnitType::Pixels),

[alancutter (OOO until 2018), 2016/10/04 05:00:18]

Nit: Handle this the same way you handle !offsetRotation, as a separate if
construct.

[Eric Willigers, 2016/10/04 05:20:31]

Done.

+              important);
+
+  if (!offsetRotation) {
+    CSSValueList* list = CSSValueList::createSpaceSeparated();
+    list->append(*CSSPrimitiveValue::createIdentifier(CSSValueAuto));
+    offsetRotation = list;
+  }

[alancutter (OOO until 2018), 2016/10/04 05:00:18]

I don't see a reason to mix up parsing and calls to addProperty, let's leave the
adds to the end.

[Eric Willigers, 2016/10/04 05:20:31]

Changed.  This wasn't parsing, just creating default values.

+  addProperty(CSSPropertyOffsetRotation, CSSPropertyOffset, *offsetRotation,

[alancutter (OOO until 2018), 2016/10/04 05:00:18]

I don't think we should be adding properties if we're going to return false at
least that's the rule other functions seem to be following, timloh can probably
clarify on this.

[Eric Willigers, 2016/10/04 05:20:30]

Changed. Font shorthand, for example, does add properties even if we might
return false. None of the elements' properties change values if we return false.

+              important);
+
+  return m_range.atEnd();
+}
+
 static CSSValue* consumeTextEmphasisStyle(CSSParserTokenRange& range) {
   CSSValueID id = range.peek().id();
   if (id == CSSValueNone)
@@ -5067,7 +5096,7 @@ bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty,
     case CSSPropertyMotion:
       return consumeShorthandGreedily(motionShorthand(), important);
     case CSSPropertyOffset:
-      return consumeShorthandGreedily(offsetShorthand(), important);
+      return consumeOffsetShorthand(important);
     case CSSPropertyWebkitTextEmphasis:
       return consumeShorthandGreedily(webkitTextEmphasisShorthand(), important);
     case CSSPropertyOutline: