Treora commented on a change in pull request #88:
URL: https://github.com/apache/incubator-annotator/pull/88#discussion_r484861032
##########
File path: packages/dom/src/highlight-range.ts
##########
@@ -73,8 +73,9 @@ function textNodesInRange(range: Range): Text[] {
}
// Collect the text nodes.
- const document =
- range.startContainer.ownerDocument || (range.startContainer as Document);
+ const { commonAncestorContainer } = range;
+ const { ownerDocument } = commonAncestorContainer;
+ const document = ownerDocument ?? (commonAncestorContainer as Document);
Review comment:
Any reason for this change?
In several places, I have so far tried to only use the properties of
[AbstractRange](https://developer.mozilla.org/en-US/docs/Web/API/AbstractRange)
when possible, to see if we can let users pass anything with this minimal (5
attribute) interface instead an actual Range. Do you think it may be worth
pursuing? If we go this way we could change our types throughout, and add a
simple helper function `scopeToRange` again to ‘hydrate’ our abstract ranges at
the start of our functions. :)
##########
File path: packages/dom/src/text-quote/describe.ts
##########
@@ -21,44 +21,31 @@
import seek from 'dom-seek';
import type { TextQuoteSelector } from '@annotator/selector';
-import type { DomScope } from '../types';
-import { ownerDocument, rangeFromScope } from '../scope';
-
export async function describeTextQuote(
range: Range,
- scope: DomScope = ownerDocument(range).documentElement,
+ scope: Range = range,
Review comment:
Should be the document element, not the range itself!
##########
File path: packages/dom/src/range/match.ts
##########
@@ -18,22 +18,21 @@
* under the License.
*/
-import type { RangeSelector, Selector } from '@annotator/selector';
-
-import { ownerDocument } from '../scope';
-import type { DomMatcher, DomScope } from '../types';
Review comment:
If we stop using them, we’d better remove the type file, and the scope
file too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
