Normalize multi-range selections to a single range in Firefox

There are situations where Firefox creates a selection with multiple
ranges, even though the spec forbids this. In various places in our code
we called `selection.getRangeAt(0)` assuming there would only be one
range. In the case where FF had created multiple ranges, this led to
only part of the user's selection being respected.

 - Modifying the `selectedRange` utility to combine multi-range
   selections into a single Range, so that the results in Firefox are
   the same as in other browsers.

 - Replacing various direct calls to `selection.getRangeAt(0)` with
   calls to `selectedRange` so that they take get this normalization
   applied.

   In the tests for `Guest`, this led to refactoring to make some tests
   less tied to implementation details.

Fixes https://github.com/hypothesis/client/issues/5485

src/annotator/guest.ts

@@ -35,8 +35,13 @@ import {
   setHighlightsVisible,
 } from './highlighter';
 import { createIntegration } from './integrations';
-import * as rangeUtil from './range-util';
-import { SelectionObserver, selectedRange } from './selection-observer';
+import {
+  itemsForRange,
+  isSelectionBackwards,
+  selectionFocusRect,
+  selectedRange,
+} from './range-util';
+import { SelectionObserver } from './selection-observer';
 import { findClosestOffscreenAnchor } from './util/buckets';
 import { frameFillsAncestor } from './util/frame';
 import { normalizeURI } from './util/url';
@@ -46,10 +51,8 @@ type AnnotationHighlight = HTMLElement & { _annotation?: AnnotationData };
 
 /** Return all the annotations tags associated with the selected text. */
 function annotationsForSelection(): string[] {
-  const selection = window.getSelection()!;
-  const range = selection.getRangeAt(0);
-  const tags = rangeUtil.itemsForRange(
-    range,
+  const tags = itemsForRange(
+    selectedRange() ?? new Range(),
     node => (node as AnnotationHighlight)._annotation?.$tag
   );
   return tags;
@@ -408,10 +411,10 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable {
    * Shift the position of the adder on window 'resize' events
    */
   _repositionAdder() {
-    if (this._isAdderVisible === false) {
+    if (!this._isAdderVisible) {
       return;
     }
-    const range = window.getSelection()?.getRangeAt(0);
+    const range = selectedRange();
     if (range) {
       this._onSelection(range);
     }
@@ -419,7 +422,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable {
 
   async _connectHost(hostFrame: Window) {
     this._hostRPC.on('clearSelection', () => {
-      if (selectedRange(document)) {
+      if (selectedRange()) {
         this._informHostOnNextSelectionClear = false;
         removeTextSelection();
       }
@@ -753,8 +756,8 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable {
     }
 
     const selection = document.getSelection()!;
-    const isBackwards = rangeUtil.isSelectionBackwards(selection);
-    const focusRect = rangeUtil.selectionFocusRect(selection);
+    const isBackwards = isSelectionBackwards(selection);
+    const focusRect = selectionFocusRect(selection);
     if (!focusRect) {
       // The selected range does not contain any text
       this._onClearSelection();

src/annotator/range-util.ts

 import { nodeIsText } from './util/node';
 
+/**
+ * Return a range that spans from the earlier of a or b's start point to
+ * the later of a or b's end point, in document order.
+ */
+function unionRanges(a: Range, b: Range): Range {
+  const result = new Range();
+
+  if (a.compareBoundaryPoints(Range.START_TO_START, b) <= 0) {
+    result.setStart(a.startContainer, a.startOffset);
+  } else {
+    result.setStart(b.startContainer, b.startOffset);
+  }
+
+  if (a.compareBoundaryPoints(Range.END_TO_END, b) >= 0) {
+    result.setEnd(a.endContainer, a.endOffset);
+  } else {
+    result.setEnd(b.endContainer, b.endOffset);
+  }
+
+  return result;
+}
+
+/**
+ * Return the currently selected {@link Range} or `null` if there is no
+ * selection.
+ */
+export function selectedRange(
+  selection: Selection | null = document.getSelection()
+): Range | null {
+  if (!selection || selection.rangeCount === 0) {
+    return null;
+  }
+
+  let range = selection.getRangeAt(0);
+
+  // Work around a Firefox issue [1] where a selection can have multiple ranges,
+  // in contradiction to the Selection API [2] spec. The workaround is to
+  // union the ranges to produce the same single range as other browsers.
+  //
+  // [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1773065
+  // [2] https://w3c.github.io/selection-api/#dom-selection-rangecount
+  for (let i = 1; i < selection.rangeCount; i++) {
+    range = unionRanges(range, selection.getRangeAt(i));
+  }
+
+  if (range.collapsed) {
+    return null;
+  }
+  return range;
+}
+
 /**
  * Returns true if the start point of a selection occurs after the end point,
  * in document order.
@@ -9,7 +60,8 @@ export function isSelectionBackwards(selection: Selection) {
     return selection.focusOffset < selection.anchorOffset;
   }
 
-  const range = selection.getRangeAt(0);
+  const range = selectedRange(selection)!;
+
   // Does not work correctly on iOS when selecting nodes backwards.
   // https://bugs.webkit.org/show_bug.cgi?id=220523
   return range.startContainer === selection.focusNode;
@@ -100,10 +152,11 @@ export function getTextBoundingBoxes(range: Range): DOMRect[] {
  * Returns null if the selection is empty.
  */
 export function selectionFocusRect(selection: Selection): DOMRect | null {
-  if (selection.isCollapsed) {
+  const range = selectedRange(selection);
+  if (!range) {
     return null;
   }
-  const textBoxes = getTextBoundingBoxes(selection.getRangeAt(0));
+  const textBoxes = getTextBoundingBoxes(range);
   if (textBoxes.length === 0) {
     return null;
   }

src/annotator/selection-observer.ts

 import { ListenerCollection } from '../shared/listener-collection';
-
-/**
- * Return the current selection or `null` if there is no selection, or it is empty.
- */
-export function selectedRange(document: Document): Range | null {
-  const selection = document.getSelection();
-  if (!selection || selection.rangeCount === 0) {
-    return null;
-  }
-  const range = selection.getRangeAt(0);
-  if (range.collapsed) {
-    return null;
-  }
-  return range;
-}
+import { selectedRange } from './range-util';
 
 /**
  * An observer that watches for and buffers changes to the document's current selection.
@@ -41,7 +27,7 @@ export class SelectionObserver {
 
     const scheduleCallback = (delay = 10) => {
       this._pendingCallback = setTimeout(() => {
-        callback(selectedRange(document_));
+        callback(selectedRange(document_.getSelection()));
       }, delay);
     };
 

src/annotator/test/guest-test.js

@@ -48,7 +48,6 @@ describe('Guest', () => {
   let fakePortFinder;
   let FakePortRPC;
   let fakePortRPCs;
-  let fakeSelectedRange;
 
   const createGuest = (config = {}) => {
     const element = document.createElement('div');
@@ -85,6 +84,34 @@ describe('Guest', () => {
     }
   };
 
+  const simulateSelectionWithText = () => {
+    rangeUtil.selectionFocusRect.returns({
+      left: 0,
+      top: 0,
+      width: 5,
+      height: 5,
+    });
+
+    const element = document.createElement('div');
+    element.textContent = 'foobar';
+    const range = new Range();
+    range.selectNodeContents(element);
+
+    rangeUtil.selectedRange.returns(range);
+    notifySelectionChanged(range);
+  };
+
+  const simulateSelectionWithoutText = () => {
+    rangeUtil.selectionFocusRect.returns(null);
+
+    const element = document.createElement('div');
+    const range = new Range();
+    range.selectNodeContents(element);
+
+    rangeUtil.selectedRange.returns(range);
+    notifySelectionChanged(range);
+  };
+
   beforeEach(() => {
     guests = [];
     highlighter = {
@@ -103,6 +130,7 @@ describe('Guest', () => {
       itemsForRange: sinon.stub().returns([]),
       isSelectionBackwards: sinon.stub(),
       selectionFocusRect: sinon.stub(),
+      selectedRange: sinon.stub().returns(null),
     };
 
     FakeAdder.instance = null;
@@ -169,8 +197,6 @@ describe('Guest', () => {
       }
     }
 
-    fakeSelectedRange = sinon.stub();
-
     $imports.$mock({
       '../shared/messaging': {
         PortFinder: sinon.stub().returns(fakePortFinder),
@@ -193,7 +219,6 @@ describe('Guest', () => {
       './range-util': rangeUtil,
       './selection-observer': {
         SelectionObserver: FakeSelectionObserver,
-        selectedRange: fakeSelectedRange,
       },
       './util/buckets': {
         findClosestOffscreenAnchor: fakeFindClosestOffscreenAnchor,
@@ -617,26 +642,19 @@ describe('Guest', () => {
       }
     });
 
-    it('does not reposition the adder on window "resize" event if the adder is hidden', () => {
-      sandbox.stub(guest, '_repositionAdder').callThrough();
-      sandbox.stub(guest, '_onSelection'); // Calling _onSelect makes the adder to reposition
-
+    it('does not reposition the adder if hidden when the window is resized', () => {
       window.dispatchEvent(new Event('resize'));
-
-      assert.called(guest._repositionAdder);
-      assert.notCalled(guest._onSelection);
+      assert.notCalled(FakeAdder.instance.show);
     });
 
-    it('reposition the adder on window "resize" event if the adder is shown', () => {
-      sandbox.stub(guest, '_repositionAdder').callThrough();
-      sandbox.stub(guest, '_onSelection'); // Calling _onSelect makes the adder to reposition
-
-      guest._isAdderVisible = true;
-      sandbox.stub(window, 'getSelection').returns({ getRangeAt: () => true });
+    it('repositions the adder when the window is resized', () => {
+      simulateSelectionWithText();
+      assert.calledOnce(FakeAdder.instance.show);
+      FakeAdder.instance.show.resetHistory();
 
       window.dispatchEvent(new Event('resize'));
 
-      assert.called(guest._onSelection);
+      assert.called(FakeAdder.instance.show);
     });
 
     it('focuses annotations in the sidebar when hovering highlights in the document', () => {
@@ -691,34 +709,6 @@ describe('Guest', () => {
   });
 
   describe('when the selection changes', () => {
-    let container;
-
-    beforeEach(() => {
-      container = document.createElement('div');
-      container.innerHTML = 'test text';
-      document.body.appendChild(container);
-      window.getSelection().selectAllChildren(container);
-    });
-
-    afterEach(() => {
-      container.remove();
-    });
-
-    const simulateSelectionWithText = () => {
-      rangeUtil.selectionFocusRect.returns({
-        left: 0,
-        top: 0,
-        width: 5,
-        height: 5,
-      });
-      notifySelectionChanged({});
-    };
-
-    const simulateSelectionWithoutText = () => {
-      rangeUtil.selectionFocusRect.returns(null);
-      notifySelectionChanged({});
-    };
-
     it('shows the adder if the selection contains text', () => {
       createGuest();
       simulateSelectionWithText();
@@ -728,10 +718,10 @@ describe('Guest', () => {
     it('sets the annotations associated with the selection', () => {
       createGuest();
       const ann = { $tag: 't1' };
-      container._annotation = ann;
-      rangeUtil.itemsForRange.callsFake((range, callback) => [
-        callback(range.startContainer),
-      ]);
+      rangeUtil.itemsForRange.callsFake((range, callback) => {
+        range.startContainer._annotation = ann;
+        return [callback(range.startContainer)];
+      });
       simulateSelectionWithText();
 
       assert.deepEqual(FakeAdder.instance.annotationsForSelection, ['t1']);
@@ -787,7 +777,6 @@ describe('Guest', () => {
 
         // Guest has text selected
         simulateSelectionWithText();
-        fakeSelectedRange.returns({});
 
         hostRPC().call.resetHistory();
         emitHostEvent('clearSelection');
@@ -811,7 +800,7 @@ describe('Guest', () => {
         guest.selectedRanges = [1];
 
         // Guest has no text selected
-        fakeSelectedRange.returns(null);
+        rangeUtil.selectedRange.returns(null);
 
         hostRPC().call.resetHistory();
         emitHostEvent('clearSelection');

src/annotator/test/range-util-test.js

 import * as rangeUtil from '../range-util';
+import { selectedRange } from '../range-util';
 
 function createRange(node, start, end) {
   const range = node.ownerDocument.createRange();
@@ -149,6 +150,66 @@ describe('annotator.range-util', () => {
     });
   });
 
+  describe('selectedRange', () => {
+    it('returns `null` if selection has no ranges', () => {
+      window.getSelection().empty();
+      assert.isNull(selectedRange());
+    });
+
+    it('returns `null` if selected range is collapsed', () => {
+      const range = new Range();
+      range.setStart(document.body, 0);
+      range.setEnd(document.body, 0);
+
+      window.getSelection().addRange(range);
+
+      assert.isNull(selectedRange());
+    });
+
+    it('returns first range in selection if not collapsed', () => {
+      const range = new Range();
+      range.selectNodeContents(document.body);
+
+      window.getSelection().addRange(range);
+
+      assert.instanceOf(selectedRange(), Range);
+    });
+
+    // Test handling of a Firefox-specific issue where selection may contain
+    // multiple ranges. In spec-compliant browsers (eg. Chrome), the selection
+    // only contains zero or one range.
+    it('returns union of all ranges in selection if there are multiple', () => {
+      const parent = document.createElement('div');
+      const el1 = document.createElement('div');
+      el1.textContent = 'foo';
+      const el2 = document.createElement('div');
+      el2.textContent = 'bar';
+      const el3 = document.createElement('div');
+      el3.textContent = 'baz';
+      parent.append(el1, el2, el3);
+
+      const ranges = [new Range(), new Range(), new Range()];
+      ranges[0].selectNodeContents(el1);
+      ranges[1].selectNodeContents(el2);
+      ranges[2].selectNodeContents(el3);
+
+      const fakeSelection = {
+        rangeCount: 3,
+        getRangeAt: index => ranges[index],
+      };
+
+      let range = selectedRange(fakeSelection);
+      assert.equal(range.toString(), 'foobarbaz');
+
+      // Test with the ordering of ranges reversed. The merged range should
+      // be the same.
+      ranges.reverse();
+
+      range = selectedRange(fakeSelection);
+      assert.equal(range.toString(), 'foobarbaz');
+    });
+  });
+
   describe('itemsForRange', () => {
     it('returns unique items for range', () => {
       const range = document.createRange();