Skip to content

Commit

Permalink
Remove rAF from highlight drawing and make synchronous
Browse files Browse the repository at this point in the history
Bug #2502 arises from an issue with the asynchronous code in guest
that relies on `requestAnimationFrame` before drawing highlights for
a particular anchor range.

When the DOM is being mutated in multiple ways, the range present in
the synchronous `highlight` code can be different from the range once
the `requestAnimationFrame` — i.e. it could have changed again since
the range was sniffed in the `highlight` method.

The call to `range.normalize` in the rAF callback in these cases might
be operating on a range that is effectively invalid per the `normalize`
method's (unwritten) expectations. That in turn throws and then causes
a failure of highlight drawing until the document is reloaded.

Some brittleness in `range` methods and other dependencies makes this
challenging to fix easily in an asynchronous manner. It's unclear
exactly why rAF asynchronicity was added
(see ca87c1c ).

As such, we've elected for now to remove `rAF` and make `highlight`
code synchronous here.

Fixes #2502
  • Loading branch information
robertknight authored and lyzadanger committed Sep 25, 2020
1 parent 88b3b4f commit 03f1821
Showing 1 changed file with 9 additions and 22 deletions.
31 changes: 9 additions & 22 deletions src/annotator/guest.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,6 @@ import { normalizeURI } from './util/url';
* @typedef {Element & { _annotation?: AnnotationData }} AnnotationHighlight
*/

const animationPromise = fn =>
new Promise((resolve, reject) =>
requestAnimationFrame(() => {
try {
resolve(fn());
} catch (error) {
reject(error);
}
})
);

/**
* Return all the annotations associated with the selected text.
*
Expand Down Expand Up @@ -391,18 +380,16 @@ export default class Guest extends Delegator {
if (!anchor.range) {
return anchor;
}
return animationPromise(() => {
const range = xpathRange.sniff(anchor.range);
const normedRange = range.normalize(root);
const highlights = /** @type {AnnotationHighlight[]} */ (highlightRange(
normedRange
));
highlights.forEach(h => {
h._annotation = anchor.annotation;
});
anchor.highlights = highlights;
return anchor;
const range = xpathRange.sniff(anchor.range);
const normedRange = range.normalize(root);
const highlights = /** @type {AnnotationHighlight[]} */ (highlightRange(
normedRange
));
highlights.forEach(h => {
h._annotation = anchor.annotation;
});
anchor.highlights = highlights;
return anchor;
};

// Store the results of anchoring.
Expand Down

0 comments on commit 03f1821

Please sign in to comment.