Compare annotation locations by CFI and text position

For annotations on EPUBs, text position selectors can only be validly compared
when they come from annotations made on the same EPUB Content Document. For
annotations that come from different content documents, it is necessary to first
compare the ordering of the content documents using CFIs, and then compare text
positions.

The use of `Number.POSITIVE_INFINITY` with replaced with
`Number.MAX_SAFE_INTEGER` when handling annotations without positions, as
`Math.sign(MAX_SAFE_INTEGER - MAX_SAFE_INTEGER)` is 0 (ie. treat two annotations
with missing positions as having the same position), but
`Math.sign(POSITIVE_INFINITY - POSITIVE_INFINITY)` is NaN.

src/sidebar/helpers/annotation-metadata.js

@@ -292,26 +292,45 @@ export function annotationRole(annotation) {
   return 'Annotation';
 }
 
-/** Return a numeric key that can be used to sort annotations by location.
+/**
+ * Key containing information needed to sort annotations based on their
+ * associated position within the document.
+ *
+ * @typedef LocationKey
+ * @prop {string} [cfi] - EPUB Canonical Fragment Identifier. For annotations
+ *   on EPUBs, this identifies the location of the chapter within the book's
+ *   table of contents.
+ * @prop {number} [position] - Text offset within the document segment, in UTF-16
+ *   code units. For web pages and PDFs this refers to the offset from the start
+ *   of the document. In EPUBs this refers to the offset from the start of the
+ *   Content Document (ie. chapter).
+ */
+
+/**
+ * Return a key that can be used to sort annotations by document position.
+ *
+ * Note that the key may not have any fields set if the annotation is a page
+ * note or was created via the Hypothesis API without providing the selectors
+ * that this function uses.
  *
  * @param {Annotation} annotation
- * @return {number} - A key representing the location of the annotation in
- *                    the document, where lower numbers mean closer to the
- *                    start.
+ * @return {LocationKey}
  */
 export function location(annotation) {
-  if (annotation) {
-    const targets = annotation.target || [];
-    for (let i = 0; i < targets.length; i++) {
-      const selectors = targets[i].selector || [];
-      for (const selector of selectors) {
-        if (selector.type === 'TextPositionSelector') {
-          return selector.start;
-        }
-      }
+  const targets = annotation.target;
+
+  let cfi;
+  let position;
+
+  for (const selector of targets[0]?.selector ?? []) {
+    if (selector.type === 'TextPositionSelector') {
+      position = selector.start;
+    } else if (selector.type === 'EPUBContentSelector' && selector.cfi) {
+      cfi = selector.cfi;
     }
   }
-  return Number.POSITIVE_INFINITY;
+
+  return { cfi, position };
 }
 
 /**

src/sidebar/helpers/test/annotation-metadata-test.js

@@ -222,8 +222,8 @@ describe('sidebar/helpers/annotation-metadata', () => {
   });
 
   describe('location', () => {
-    it('returns the position for annotations with a text position', () => {
-      assert.equal(
+    it('returns position for annotations with a text position', () => {
+      assert.deepEqual(
         annotationMetadata.location({
           target: [
             {
@@ -236,12 +236,35 @@ describe('sidebar/helpers/annotation-metadata', () => {
             },
           ],
         }),
-        100
+        { cfi: undefined, position: 100 }
       );
     });
 
-    it('returns +ve infinity for annotations without a text position', () => {
-      assert.equal(
+    it('returns CFI for annotations with a CFI', () => {
+      assert.deepEqual(
+        annotationMetadata.location({
+          target: [
+            {
+              selector: [
+                {
+                  type: 'EPUBContentSelector',
+                  cfi: '/2/4',
+                  url: 'content/chapter2.xhtml',
+                },
+                {
+                  type: 'TextPositionSelector',
+                  start: 200,
+                },
+              ],
+            },
+          ],
+        }),
+        { cfi: '/2/4', position: 200 }
+      );
+    });
+
+    it('returns undefined for annotations without a text position', () => {
+      assert.deepEqual(
         annotationMetadata.location({
           target: [
             {
@@ -249,7 +272,7 @@ describe('sidebar/helpers/annotation-metadata', () => {
             },
           ],
         }),
-        Number.POSITIVE_INFINITY
+        { cfi: undefined, position: undefined }
       );
     });
   });

src/sidebar/helpers/test/thread-sorters-test.js

@@ -77,25 +77,37 @@ describe('sidebar/util/thread-sorters', () => {
   });
 
   describe('sorting by document location', () => {
+    // Create a position-only location. This is the common case for a web page
+    // or PDF.
+    function posLocation(pos) {
+      return { position: pos };
+    }
+
+    // Create a location with an EPUB CFI and position. This would occur in
+    // an ebook.
+    function cfiLocation(cfi, pos) {
+      return { cfi, position: pos };
+    }
+
     [
       {
-        a: { annotation: { location: 5 } },
-        b: { annotation: { location: 10 } },
+        a: { annotation: { location: posLocation(5) } },
+        b: { annotation: { location: posLocation(10) } },
         expected: -1,
       },
       {
-        a: { annotation: { location: 10 } },
-        b: { annotation: { location: 10 } },
+        a: { annotation: { location: posLocation(10) } },
+        b: { annotation: { location: posLocation(10) } },
         expected: 0,
       },
       {
-        a: { annotation: { location: 10 } },
-        b: { annotation: { location: 5 } },
+        a: { annotation: { location: posLocation(10) } },
+        b: { annotation: { location: posLocation(5) } },
         expected: 1,
       },
       {
         a: {},
-        b: { annotation: { location: 5 } },
+        b: { annotation: { location: posLocation(5) } },
         expected: -1,
       },
       {
@@ -104,7 +116,7 @@ describe('sidebar/util/thread-sorters', () => {
         expected: 0,
       },
       {
-        a: { annotation: { location: 10 } },
+        a: { annotation: { location: posLocation(10) } },
         b: {},
         expected: 1,
       },
@@ -119,5 +131,46 @@ describe('sidebar/util/thread-sorters', () => {
         );
       });
     });
+
+    [
+      // CFI only
+      {
+        a: { annotation: { location: cfiLocation('/2/2') } },
+        b: { annotation: { location: cfiLocation('/2/4') } },
+        expected: -1,
+      },
+      {
+        a: { annotation: { location: cfiLocation('/2/4') } },
+        b: { annotation: { location: cfiLocation('/2/4') } },
+        expected: 0,
+      },
+      {
+        a: { annotation: { location: cfiLocation('/2/4') } },
+        b: { annotation: { location: cfiLocation('/2/2') } },
+        expected: 1,
+      },
+
+      // CFI and position
+      {
+        a: { annotation: { location: cfiLocation('/2/2', 100) } },
+        b: { annotation: { location: cfiLocation('/2/4', 10) } },
+        expected: -1,
+      },
+      {
+        a: { annotation: { location: cfiLocation('/2/4', 100) } },
+        b: { annotation: { location: cfiLocation('/2/4', 10) } },
+        expected: 1,
+      },
+    ].forEach((testCase, index) => {
+      it(`sorts by CFI when present (${index})`, () => {
+        assert.equal(
+          // Disable eslint: `sorters` properties start with capital letters
+          // to match their displayed sort option values
+          /* eslint-disable-next-line new-cap */
+          sorters.Location(testCase.a, testCase.b),
+          testCase.expected
+        );
+      });
+    });
   });
 });

src/sidebar/helpers/thread-sorters.js

+import { compareCFIs } from '../util/cfi';
 import { location } from './annotation-metadata';
 import { rootAnnotations } from './thread';
 
@@ -89,11 +90,25 @@ export const sorters = {
     }
     const aLocation = location(a.annotation);
     const bLocation = location(b.annotation);
-    if (aLocation < bLocation) {
+
+    // If these annotations come from an EPUB and specify which chapter they
+    // came from via a CFI, compare the chapter order first.
+    if (aLocation.cfi && bLocation.cfi) {
+      const cfiResult = compareCFIs(aLocation.cfi, bLocation.cfi);
+      if (cfiResult !== 0) {
+        return Math.sign(cfiResult);
+      }
+    } else if (aLocation.cfi) {
       return -1;
-    } else if (aLocation > bLocation) {
+    } else if (bLocation.cfi) {
       return 1;
     }
-    return 0;
+
+    // If the chapter number is the same or for other document types, compare
+    // the text position instead. Missing positions sort after any present
+    // positions.
+    const aPos = aLocation.position ?? Number.MAX_SAFE_INTEGER;
+    const bPos = bLocation.position ?? Number.MAX_SAFE_INTEGER;
+    return Math.sign(aPos - bPos);
   },
 };