Fix non-standard quadpoints orders for annotations

[timvandermeij]

I have made a PDF file that serves as a minimal reduced test case for all annotation types with quadpoints support. Currently all markup annotations are supported for quadpoints (highlight, underline, strikethrough and squiggly annotations), but link annotations can also have quadpoints which we don't support yet. This can be done similar to the existing annotation types, but in my test file I actually ran into https://stackoverflow.com/questions/9855814/pdf-spec-vs-acrobat-creation-quadpoints which should be addressed first before we can do that.

In short:

• Handle https://stackoverflow.com/questions/9855814/pdf-spec-vs-acrobat-creation-quadpoints properly.
• Implement quadpoints support for link annotations.
• Include the reduced test case for all types.

[Snuffleupagus]

If you're going to work on this code anyway, might I suggest moving the _createQuadrilaterals-call into the AnnotationElement constructor to avoid having to sprinkle those throughout lots of classes instead?

Basically, I'm imagining something like this (however untested) which also re-factors how arguments are passed to the base AnnotationElement class:

diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js
index 60c3cc399..ab1bb8af7 100644
--- a/src/display/annotation_layer.js
+++ b/src/display/annotation_layer.js
@@ -133,7 +133,14 @@ class AnnotationElementFactory {
 }
 
 class AnnotationElement {
-  constructor(parameters, isRenderable = false, ignoreBorder = false) {
+  constructor(
+    parameters,
+    {
+      isRenderable = false,
+      ignoreBorder = false,
+      createQuadrilaterals = false,
+    } = {}
+  ) {
     this.isRenderable = isRenderable;
     this.data = parameters.data;
     this.layer = parameters.layer;
@@ -151,6 +158,9 @@ class AnnotationElement {
     if (isRenderable) {
       this.container = this._createContainer(ignoreBorder);
     }
+    if (createQuadrilaterals) {
+      this.quadrilaterals = this._createQuadrilaterals(ignoreBorder);
+    }
   }
 
   /**
@@ -333,7 +343,7 @@ class LinkAnnotationElement extends AnnotationElement {
       parameters.data.action ||
       parameters.data.isTooltipOnly
     );
-    super(parameters, isRenderable);
+    super(parameters, { isRenderable });
   }
 
   /**
@@ -416,7 +426,7 @@ class TextAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable);
+    super(parameters, { isRenderable });
   }
 
   /**
@@ -473,7 +483,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
     const isRenderable =
       parameters.renderInteractiveForms ||
       (!parameters.data.hasAppearance && !!parameters.data.fieldValue);
-    super(parameters, isRenderable);
+    super(parameters, { isRenderable });
   }
 
   /**
@@ -680,7 +690,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
 
 class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {
   constructor(parameters) {
-    super(parameters, parameters.renderInteractiveForms);
+    super(parameters, { isRenderable: parameters.renderInteractiveForms });
   }
 
   /**
@@ -720,7 +730,7 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {
 
 class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement {
   constructor(parameters) {
-    super(parameters, parameters.renderInteractiveForms);
+    super(parameters, { isRenderable: parameters.renderInteractiveForms });
   }
 
   /**
@@ -792,7 +802,7 @@ class PushButtonWidgetAnnotationElement extends LinkAnnotationElement {
 
 class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement {
   constructor(parameters) {
-    super(parameters, parameters.renderInteractiveForms);
+    super(parameters, { isRenderable: parameters.renderInteractiveForms });
   }
 
   /**
@@ -857,7 +867,7 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement {
 class PopupAnnotationElement extends AnnotationElement {
   constructor(parameters) {
     const isRenderable = !!(parameters.data.title || parameters.data.contents);
-    super(parameters, isRenderable);
+    super(parameters, { isRenderable });
   }
 
   /**
@@ -1082,7 +1092,7 @@ class FreeTextAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
   }
 
   /**
@@ -1109,7 +1119,7 @@ class LineAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
   }
 
   /**
@@ -1160,7 +1170,7 @@ class SquareAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
   }
 
   /**
@@ -1214,7 +1224,7 @@ class CircleAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
   }
 
   /**
@@ -1268,7 +1278,7 @@ class PolylineAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
 
     this.containerClassName = "polylineAnnotation";
     this.svgElementName = "svg:polyline";
@@ -1340,7 +1350,7 @@ class CaretAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
   }
 
   /**
@@ -1367,7 +1377,7 @@ class InkAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
 
     this.containerClassName = "inkAnnotation";
 
@@ -1433,8 +1443,11 @@ class HighlightAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
-    this.quadrilaterals = this._createQuadrilaterals(/* ignoreBorder = */ true);
+    super(parameters, {
+      isRenderable,
+      ignoreBorder: true,
+      createQuadrilaterals: true,
+    });
   }
 
   /**
@@ -1468,8 +1481,11 @@ class UnderlineAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
-    this.quadrilaterals = this._createQuadrilaterals(/* ignoreBorder = */ true);
+    super(parameters, {
+      isRenderable,
+      ignoreBorder: true,
+      createQuadrilaterals: true,
+    });
   }
 
   /**
@@ -1503,8 +1519,11 @@ class SquigglyAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
-    this.quadrilaterals = this._createQuadrilaterals(/* ignoreBorder = */ true);
+    super(parameters, {
+      isRenderable,
+      ignoreBorder: true,
+      createQuadrilaterals: true,
+    });
   }
 
   /**
@@ -1538,8 +1557,11 @@ class StrikeOutAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
-    this.quadrilaterals = this._createQuadrilaterals(/* ignoreBorder = */ true);
+    super(parameters, {
+      isRenderable,
+      ignoreBorder: true,
+      createQuadrilaterals: true,
+    });
   }
 
   /**
@@ -1573,7 +1595,7 @@ class StampAnnotationElement extends AnnotationElement {
       parameters.data.title ||
       parameters.data.contents
     );
-    super(parameters, isRenderable, /* ignoreBorder = */ true);
+    super(parameters, { isRenderable, ignoreBorder: true });
   }
 
   /**
@@ -1595,7 +1617,7 @@ class StampAnnotationElement extends AnnotationElement {
 
 class FileAttachmentAnnotationElement extends AnnotationElement {
   constructor(parameters) {
-    super(parameters, /* isRenderable = */ true);
+    super(parameters, { isRenderable: true });
 
     const { filename, content } = this.data.file;
     this.filename = getFilenameFromUrl(filename);

[Snuffleupagus]

In addition to #12675 (comment), perhaps it would also make sense to introduce a _renderQuadrilaterals method (on the AnnotationElement-class) looking something like:

_renderQuadrilaterals(className) {
  if (
    typeof PDFJSDev === "undefined" ||
    PDFJSDev.test("!PRODUCTION || TESTING")
  ) {
    assert(this.quadrilaterals, ...);
  }
  this.quadrilaterals.forEach(quadrilateral => { 
    quadrilateral.className = className;
  }); 
  return this.quadrilaterals; 
 } 

since that'd would then allow replacing all instances of code such as this with something like:

    if (this.quadrilaterals) {
      return this._renderQuadrilaterals("highlightAnnotation");
    }

The above is just a few quick ideas on how to reduce unnecessary boilerplate for this code; please feel free to disregard this if you don't agree :-)

[timvandermeij]

Yes, that sounds like a good idea since I'll need to touch that particular code anyway; thanks!