Implement spec tweaks on pseudo-element enimations (CSSWG#4616)

Update animation composite order on pseudo-element.
Implement change found in https://github.com/w3c/csswg-drafts/pull/4616
making the sort order on pseudo-elements include all selectors.

Add missing const declaration to KeyframeEffect::pseudoElement().

Remove empty-string case from KeyframeEffect::setPseudoElement().

Bug: 993365, 981894
Change-Id: I32ff6917b74fb490695299d0fde0c12393755437
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031986
Commit-Queue: George Steel <gtsteel@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740490}

third_party/blink/renderer/core/animation/animation.cc

@@ -68,7 +68,7 @@ namespace blink {
 
 namespace {
 
-enum PseudoPriority { kMarker, kBefore, kOther, kAfter };
+enum PseudoPriority { kNone, kMarker, kBefore, kOther, kAfter };
 
 unsigned NextSequenceNumber() {
   static unsigned next = 0;
@@ -96,6 +96,8 @@ double Min(base::Optional<double> a, double b) {
 }
 
 PseudoPriority ConvertStringtoPriority(const String& pseudo) {
+  if (pseudo.IsNull())
+    return PseudoPriority::kNone;
   if (pseudo == "::marker")
     return PseudoPriority::kMarker;
   if (pseudo == "::before")
@@ -479,46 +481,32 @@ bool Animation::HasLowerCompositeOrdering(const Animation* animation1,
     }
 
     // A pseudo-element has a higher composite ordering than its originating
-    // element, but lower than the originating element's children.  Two
-    // pseudo-elements sharing the same originating element are sorted as
-    // follows:
+    // element, but lower than the originating element's children.
+    // Two pseudo-elements sharing the same originating element are sorted
+    // as follows:
     // ::marker
     // ::before
-    // ::other
+    // other pseudo-elements (ordered by selector)
     // ::after
-    // The "::other" category is a catch-all for any pseudo-element that does
-    // not match another label. This category is not currently covered in the
-    // spec.
-    // TODO: revisit when the spec is clarified
-    // (https://github.com/w3c/csswg-drafts/issues/4502).
-    const String& pseudo1 =
-        const_cast<KeyframeEffect*>(effect1)->pseudoElement();
-    const String& pseudo2 =
-        const_cast<KeyframeEffect*>(effect2)->pseudoElement();
-
-    if (!pseudo1.IsEmpty() && !pseudo2.IsEmpty()) {
-      PseudoPriority priority1 = ConvertStringtoPriority(pseudo1);
-      PseudoPriority priority2 = ConvertStringtoPriority(pseudo2);
-      // In this case, we are comparing the SequenceNumber for now.
-      // TODO(crbug.com/1045835): compare them via Animation Name Property
-      if (priority1 == priority2)
-        return animation1->SequenceNumber() < animation2->SequenceNumber();
-      return priority1 < priority2;
-    }
-
-    // If one is pseudo element and the other one is not, then we compare the
-    // hosting element and the owning element.
-    if (!pseudo1.IsEmpty())
-      return false;
+    const String& pseudo1 = effect1->pseudoElement();
+    const String& pseudo2 = effect2->pseudoElement();
+    PseudoPriority priority1 = ConvertStringtoPriority(pseudo1);
+    PseudoPriority priority2 = ConvertStringtoPriority(pseudo2);
 
-    if (!pseudo2.IsEmpty())
-      return true;
-    // TODO: Sort animation1 and animation2 based on their position in the
-    // computed value of "animation-name" property
-  }
-  // If the anmiations are not-CSS web animation or both animations have the
-  // same owning element then just compare them via generation time/ sequence
-  // number
+    if (priority1 != priority2)
+      return priority1 < priority2;
+    if (priority1 == kOther && pseudo1 != pseudo2)
+      return CodeUnitCompareLessThan(pseudo1, pseudo2);
+
+    // For two animatiions with the same target (including the pseudo-element
+    // selector) compare the SequenceNumber for now.
+    // TODO(crbug.com/1045835): Sort animation1 and animation2 based on their
+    // position in the computed value of "animation-name" property for
+    // CSSAnimations and transition property for CSSTransitions.
+    return animation1->SequenceNumber() < animation2->SequenceNumber();
+  }
+  // If the anmiations are not-CSS WebAnimation just compare them via generation
+  // time/ sequence number.
   return animation1->SequenceNumber() < animation2->SequenceNumber();
 }
 

third_party/blink/renderer/core/animation/keyframe_effect.cc

@@ -54,9 +54,7 @@ namespace {
 
 // Verifies that a pseudo-element selector lexes and canonicalizes legacy forms
 bool ValidateAndCanonicalizePseudo(String& selector) {
-  if (selector.IsEmpty()) {
-    if (!selector.IsNull())
-      selector = String();  // null
+  if (selector.IsNull()) {
     return true;
   } else if (selector.StartsWith("::")) {
     return true;
@@ -104,7 +102,7 @@ KeyframeEffect* KeyframeEffect::Create(
         // https://github.com/w3c/csswg-drafts/issues/4586 resolves
         exception_state.ThrowDOMException(
             DOMExceptionCode::kSyntaxError,
-            "A valid pseudo-selector must start with ::.");
+            "A valid pseudo-selector must be null or start with ::.");
       }
     }
   }
@@ -178,7 +176,7 @@ void KeyframeEffect::setTarget(Element* new_target) {
   RefreshTarget();
 }
 
-const String& KeyframeEffect::pseudoElement() {
+const String& KeyframeEffect::pseudoElement() const {
   return target_pseudo_;
 }
 
@@ -189,7 +187,7 @@ void KeyframeEffect::setPseudoElement(String pseudo,
   } else {
     exception_state.ThrowDOMException(
         DOMExceptionCode::kSyntaxError,
-        "A valid pseudo-selector must start with ::.");
+        "A valid pseudo-selector must be null or start with ::.");
   }
 
   RefreshTarget();

third_party/blink/renderer/core/animation/keyframe_effect.h

@@ -82,7 +82,7 @@ class CORE_EXPORT KeyframeEffect final : public AnimationEffect {
   // this returns the originating element.
   Element* target() const { return target_element_; }
   void setTarget(Element*);
-  const String& pseudoElement();
+  const String& pseudoElement() const;
   void setPseudoElement(String, ExceptionState&);
   String composite() const;
   void setComposite(String);