Use `TextRange` to improve handling of overlapping highlights

After resolving an annotation's selectors to a `Range`, convert the
range to a `TextRange` object which is stored with the anchor and later resolved
to a concrete `Range` just before inserting highlights. Representing the
anchored region using a `TextRange` allows for the correct text to be
highlighted later if the region has been modified as a result of
inserting other highlights, providing that the text content of the
region has not been changed.

Fixes https://github.com/hypothesis/h/issues/3278
Fixes https://github.com/hypothesis/client/issues/2326
Fixes https://github.com/hypothesis/h/issues/5997

src/annotator/guest.js

@@ -4,6 +4,7 @@ import Delegator from './delegator';
 import { Adder } from './adder';
 
 import * as htmlAnchoring from './anchoring/html';
+import { TextRange } from './anchoring/text-range';
 import {
   getHighlightsContainingNode,
   highlightRange,
@@ -60,6 +61,26 @@ function annotationsAt(node) {
   return /** @type {AnnotationData[]} */ (items);
 }
 
+/**
+ * Resolve an anchor's associated document region to a concrete `Range`.
+ *
+ * This may fail if anchoring failed or if the document has been mutated since
+ * the anchor was created in a way that invalidates the anchor.
+ *
+ * @param {Anchor} anchor
+ * @return {Range|null}
+ */
+function resolveAnchor(anchor) {
+  if (!anchor.range) {
+    return null;
+  }
+  try {
+    return anchor.range.toRange();
+  } catch {
+    return null;
+  }
+}
+
 /**
  * `Guest` is the central class of the annotator that handles anchoring (locating)
  * annotations in the document when they are fetched by the sidebar, rendering
@@ -300,10 +321,15 @@ export default class Guest extends Delegator {
       for (let anchor of this.anchors) {
         if (anchor.highlights) {
           if (anchor.annotation.$tag === tag) {
+            const range = resolveAnchor(anchor);
+            if (!range) {
+              continue;
+            }
+
             const event = new CustomEvent('scrolltorange', {
               bubbles: true,
               cancelable: true,
-              detail: anchor.range,
+              detail: range,
             });
             const defaultNotPrevented = this.element.dispatchEvent(event);
             if (defaultNotPrevented) {
@@ -396,7 +422,12 @@ export default class Guest extends Delegator {
         .then(range => ({
           annotation,
           target,
-          range,
+
+          // Convert the `Range` to a `TextRange` which can be converted back to
+          // a `Range` later. The `TextRange` representation allows for highlights
+          // to be inserted during anchoring other annotations without "breaking"
+          // this anchor.
+          range: TextRange.fromRange(range),
         }))
         .catch(() => ({
           annotation,
@@ -410,11 +441,13 @@ export default class Guest extends Delegator {
      * @param {Anchor} anchor
      */
     const highlight = anchor => {
-      if (!anchor.range) {
+      const range = resolveAnchor(anchor);
+      if (!range) {
         return anchor;
       }
+
       const highlights = /** @type {AnnotationHighlight[]} */ (highlightRange(
-        anchor.range
+        range
       ));
       highlights.forEach(h => {
         h._annotation = anchor.annotation;

src/annotator/test/guest-test.js

@@ -24,6 +24,20 @@ class FakePlugin extends Delegator {
 }
 FakePlugin.instance = null;
 
+class FakeTextRange {
+  constructor(range) {
+    this.range = range;
+  }
+
+  toRange() {
+    return this.range;
+  }
+
+  static fromRange(range) {
+    return new FakeTextRange(range);
+  }
+}
+
 // A little helper which returns a promise that resolves after a timeout
 const timeoutPromise = (millis = 0) =>
   new Promise(resolve => setTimeout(resolve, millis));
@@ -86,6 +100,9 @@ describe('Guest', () => {
     $imports.$mock({
       './adder': { Adder: FakeAdder },
       './anchoring/html': htmlAnchoring,
+      './anchoring/text-range': {
+        TextRange: FakeTextRange,
+      },
       './highlighter': highlighter,
       './range-util': rangeUtil,
       './selection-observer': {
@@ -262,16 +279,22 @@ describe('Guest', () => {
       it('scrolls to the anchor with the matching tag', () => {
         const highlight = document.createElement('span');
         const guest = createGuest();
+        const fakeRange = sinon.stub();
         guest.anchors = [
-          { annotation: { $tag: 'tag1' }, highlights: [highlight] },
+          {
+            annotation: { $tag: 'tag1' },
+            highlights: [highlight],
+            range: new FakeTextRange(fakeRange),
+          },
         ];
+
         emitGuestEvent('scrollToAnnotation', 'tag1');
+
         assert.called(scrollIntoView);
         assert.calledWith(scrollIntoView, highlight);
       });
 
-      context('when dispatching the "scrolltorange" event', () => {
-        it('emits with the range', () => {
+      it('emits a "scrolltorange" DOM event', () => {
         const highlight = document.createElement('span');
         const guest = createGuest();
         const fakeRange = sinon.stub();
@@ -279,7 +302,7 @@ describe('Guest', () => {
           {
             annotation: { $tag: 'tag1' },
             highlights: [highlight],
-              range: fakeRange,
+            range: new FakeTextRange(fakeRange),
           },
         ];
 
@@ -301,16 +324,37 @@ describe('Guest', () => {
           {
             annotation: { $tag: 'tag1' },
             highlights: [highlight],
-              range: fakeRange,
+            range: new FakeTextRange(fakeRange),
           },
         ];
-
         guest.element.addEventListener('scrolltorange', event =>
           event.preventDefault()
         );
+
         emitGuestEvent('scrollToAnnotation', 'tag1');
+
         assert.notCalled(scrollIntoView);
       });
+
+      it("does nothing if the anchor's range cannot be resolved", () => {
+        const highlight = document.createElement('span');
+        const guest = createGuest();
+        guest.anchors = [
+          {
+            annotation: { $tag: 'tag1' },
+            highlights: [highlight],
+            range: {
+              toRange: sinon.stub().throws(new Error('Something went wrong')),
+            },
+          },
+        ];
+        const eventEmitted = sinon.stub();
+        guest.element.addEventListener('scrolltorange', eventEmitted);
+
+        emitGuestEvent('scrollToAnnotation', 'tag1');
+
+        assert.notCalled(eventEmitted);
+        assert.notCalled(scrollIntoView);
       });
     });
 
@@ -763,7 +807,7 @@ describe('Guest', () => {
         assert.equal(guest.anchors.length, 1);
         assert.strictEqual(guest.anchors[0].annotation, annotation);
         assert.strictEqual(guest.anchors[0].target, target);
-        assert.strictEqual(guest.anchors[0].range, range);
+        assert.strictEqual(guest.anchors[0].range.toRange(), range);
         assert.strictEqual(guest.anchors[0].highlights, highlights);
       });
     });

src/annotator/test/integration/anchoring-test.js

@@ -74,16 +74,12 @@ describe('anchoring', function () {
       quotes: ["This has not been a scientist's war"],
     },
     {
-      // Known failure with nested annotations that are anchored via quotes
-      // or positions. See https://github.com/hypothesis/h/pull/3313 and
-      // https://github.com/hypothesis/h/issues/3278
       tag: 'nested quotes',
       quotes: [
         "This has not been a scientist's war;" +
           ' it has been a war in which all have had a part',
         "scientist's war",
       ],
-      expectFail: true,
     },
   ].forEach(testCase => {
     it(`should highlight ${testCase.tag} when annotations are loaded`, () => {

src/types/annotator.js

@@ -2,6 +2,22 @@
  * Type definitions for objects passed between the annotator and sidebar.
  */
 
+/**
+ * Object representing a region of a document that an annotation
+ * has been anchored to.
+ *
+ * This representation of anchor ranges allows for certain document mutations
+ * in between anchoring an annotation and later making use of the anchored range,
+ * such as inserting highlights for other anchors. Compared to the initial
+ * anchoring of serialized selectors, resolving these ranges should be a
+ * cheap operation.
+ *
+ * @typedef AbstractRange
+ * @prop {() => Range} toRange -
+ *   Resolve the abstract range to a concrete live `Range`. The implementation
+ *   may or may not return the same `Range` each time.
+ */
+
 /**
  * @typedef {import("./api").Selector} Selector
  * @typedef {import("./api").Target} Target
@@ -30,8 +46,10 @@
  *
  * @typedef Anchor
  * @prop {AnnotationData} annotation
- * @prop {HTMLElement[]} [highlights]
- * @prop {Range} [range]
+ * @prop {HTMLElement[]} [highlights] -
+ *   The HTML elements that create the highlight for this annotation.
+ * @prop {AbstractRange} [range] -
+ *   Region of the document that this annotation's selectors were resolved to.
  * @prop {Target} target
  */