Make sure that enum documentation contains unique IDs for animations (#…

…2060) * Make sure that enum documentation contains unique IDs for animations * fix pubspec warning for CI to pass
dart-lang · Nov 8, 2019 · 8c4227b · 8c4227b
1 parent fc46f99
commit 8c4227b
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 11 deletions.
diff --git a/lib/src/model.dart b/lib/src/model.dart
@@ -4469,13 +4469,17 @@ abstract class ModelElement extends Canonicalization
     // Matches valid javascript identifiers.
     final RegExp validIdRegExp = RegExp(r'^[a-zA-Z_]\w*$');
 
-    final Set<String> uniqueIds = Set<String>();
+    // Make sure we have a set to keep track of used IDs for this href.
+    package.usedAnimationIdsByHref[href] ??= {};
+
     String getUniqueId(String base) {
-      int count = 1;
-      String id = '$base$count';
-      while (uniqueIds.contains(id)) {
-        count++;
-        id = '$base$count';
+      int animationIdCount = 1;
+      String id = '$base$animationIdCount';
+      // We check for duplicate IDs so that we make sure not to collide with
+      // user-supplied ids on the same page.
+      while (package.usedAnimationIdsByHref[href].contains(id)) {
+        animationIdCount++;
+        id = '$base$animationIdCount';
       }
       return id;
     }
@@ -4513,13 +4517,13 @@ abstract class ModelElement extends Canonicalization
                 'and must not begin with a number.');
         return '';
       }
-      if (uniqueIds.contains(uniqueId)) {
+      if (package.usedAnimationIdsByHref[href].contains(uniqueId)) {
         warn(PackageWarning.invalidParameter,
             message: 'An animation has a non-unique identifier, "$uniqueId". '
                 'Animation identifiers must be unique.');
         return '';
       }
-      uniqueIds.add(uniqueId);
+      package.usedAnimationIdsByHref[href].add(uniqueId);
 
       int width;
       try {
@@ -4569,7 +4573,8 @@ abstract class ModelElement extends Canonicalization
 
 <div style="position: relative;">
   <div id="${overlayId}"
-       onclick="if ($uniqueId.paused) {
+       onclick="var $uniqueId = document.getElementById('$uniqueId');
+                if ($uniqueId.paused) {
                   $uniqueId.play();
                   this.style.display = 'none';
                 } else {
@@ -4586,7 +4591,8 @@ abstract class ModelElement extends Canonicalization
   </div>
   <video id="$uniqueId"
          style="width:${width}px; height:${height}px;"
-         onclick="if (this.paused) {
+         onclick="var $overlayId = document.getElementById('$overlayId');
+                  if (this.paused) {
                     this.play();
                     $overlayId.style.display = 'none';
                   } else {
@@ -6308,6 +6314,10 @@ class Package extends LibraryContainer
   /// Number of times we have invoked a tool for this package.
   int toolInvocationIndex = 0;
 
+  // The animation IDs that have already been used, indexed by the [href] of the
+  // object that contains them.
+  Map<String, Set<String>> usedAnimationIdsByHref = {};
+
   /// Pieces of the location split by [locationSplitter] (removing package: and
   /// slashes).
   @override

diff --git a/test/model_test.dart b/test/model_test.dart
@@ -1176,8 +1176,16 @@ void main() {
     Method withAnimationInOneLineDoc;
     Method withAnimationInline;
     Method withAnimationOutOfOrder;
+    Enum enumWithAnimation;
+    EnumField enumValue1;
+    EnumField enumValue2;
 
     setUpAll(() {
+      enumWithAnimation = exLibrary.enums.firstWhere((c) => c.name == 'EnumWithAnimation');
+      enumValue1 = enumWithAnimation.constants
+          .firstWhere((m) => m.name == 'value1');
+      enumValue2 = enumWithAnimation.constants
+          .firstWhere((m) => m.name == 'value2');
       dog = exLibrary.classes.firstWhere((c) => c.name == 'Dog');
       withAnimation =
           dog.allInstanceMethods.firstWhere((m) => m.name == 'withAnimation');
@@ -1237,6 +1245,14 @@ void main() {
       expect(withAnimationOutOfOrder.documentation,
           contains('<video id="outOfOrder"'));
     });
+    test("Enum field animation identifiers are unique.", () {
+      expect(enumValue1.documentationAsHtml, contains('<video id="animation_1"'));
+      expect(enumValue1.documentationAsHtml, contains('<video id="animation_2"'));
+      expect(enumValue2.documentationAsHtml, isNot(contains('<video id="animation_1"')));
+      expect(enumValue2.documentationAsHtml, isNot(contains('<video id="animation_2"')));
+      expect(enumValue2.documentationAsHtml, contains('<video id="animation_3"'));
+      expect(enumValue2.documentationAsHtml, contains('<video id="animation_4"'));
+    });
   });
 
   group('MultiplyInheritedExecutableElement handling', () {

diff --git a/testing/test_package/lib/example.dart b/testing/test_package/lib/example.dart
@@ -452,6 +452,23 @@ class Dog implements Cat, E {
   void abstractMethod() {}
 }
 
+/// Animation in an enum
+enum EnumWithAnimation {
+  /// Animation enum value1
+  ///
+  /// {@animation 100 100 http://host/path/to/video1.mp4}
+  /// {@animation 100 100 http://host/path/to/video2.mp4}
+  /// More docs
+  value1,
+
+  /// Animation enum value2
+  ///
+  /// {@animation 100 100 http://host/path/to/video1.mp4}
+  /// {@animation 100 100 http://host/path/to/video2.mp4}
+  /// More docs
+  value2,
+}
+
 abstract class E {}
 
 class F<T extends String> extends Dog with _PrivateAbstractClass {
@@ -678,4 +695,4 @@ extension on Object {
 /// This class has nothing to do with [_Shhh], [FancyList], or [AnExtension.call],
 /// but should not crash because we referenced them.
 /// We should be able to find [DocumentThisExtensionOnce], too.
-class ExtensionReferencer {}
+class ExtensionReferencer {}