This is an automated email from the ASF dual-hosted git repository. randall pushed a commit to branch range-as-dom-scope in repository https://gitbox.apache.org/repos/asf/incubator-annotator.git
commit ab1b69009879631cf8161bf8c93c051ed7631b25 Author: Randall Leeds <[email protected]> AuthorDate: Sat Aug 22 20:32:56 2020 -0700 Restrict DOM scopes to be instances of Range Define the DOM match functions in terms of Range and place the burden on the caller to pass a Range. Remove the scope helper functions. This change simplifies the code at the possible cost of developer experience, but understanding the tradeoff will require some experience and feedback. --- packages/dom/src/css.ts | 20 +++++++++-- packages/dom/src/highlight-range.ts | 5 +-- packages/dom/src/range/match.ts | 17 +++++---- packages/dom/src/text-quote/describe.ts | 52 ++++++++++----------------- packages/dom/src/text-quote/match.ts | 25 +++++++------ packages/dom/test/text-quote/describe.test.ts | 8 +++-- packages/dom/test/text-quote/match.test.ts | 35 +++++++++++++----- 7 files changed, 91 insertions(+), 71 deletions(-) diff --git a/packages/dom/src/css.ts b/packages/dom/src/css.ts index 28ffb94..4e1f639 100644 --- a/packages/dom/src/css.ts +++ b/packages/dom/src/css.ts @@ -22,8 +22,22 @@ import type { CssSelector, Matcher } from '@annotator/selector'; export function createCssSelectorMatcher( selector: CssSelector, -): Matcher<Document, Element> { - return async function* matchAll(scope: Document) { - yield* scope.querySelectorAll(selector.value); +): Matcher<Range, Range> { + return async function* matchAll(scope) { + const { commonAncestorContainer } = scope; + const { ownerDocument } = commonAncestorContainer; + const document = ownerDocument ?? (commonAncestorContainer as Document); + + for (const element of document.querySelectorAll(selector.value)) { + const range = document.createRange(); + range.selectNode(element); + + if ( + scope.isPointInRange(range.startContainer, range.startOffset) && + scope.isPointInRange(range.endContainer, range.endOffset) + ) { + yield range; + } + } }; } diff --git a/packages/dom/src/highlight-range.ts b/packages/dom/src/highlight-range.ts index e7b9b8e..915ae45 100644 --- a/packages/dom/src/highlight-range.ts +++ b/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); const walker = document.createTreeWalker( range.commonAncestorContainer, NodeFilter.SHOW_TEXT, diff --git a/packages/dom/src/range/match.ts b/packages/dom/src/range/match.ts index df87db7..0972071 100644 --- a/packages/dom/src/range/match.ts +++ b/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'; +import type { Matcher, RangeSelector, Selector } from '@annotator/selector'; import { product } from './cartesian'; export function makeCreateRangeSelectorMatcher( - createMatcher: <T extends Selector>(selector: T) => DomMatcher, -): (selector: RangeSelector) => DomMatcher { - return function createRangeSelectorMatcher(selector: RangeSelector) { + createMatcher: <T extends Selector>(selector: T) => Matcher<Range, Range>, +): (selector: RangeSelector) => Matcher<Range, Range> { + return function createRangeSelectorMatcher(selector) { const startMatcher = createMatcher(selector.startSelector); const endMatcher = createMatcher(selector.endSelector); - return async function* matchAll(scope: DomScope) { - const document = ownerDocument(scope); + return async function* matchAll(scope) { + const { commonAncestorContainer } = scope; + const { ownerDocument } = commonAncestorContainer; + const document = ownerDocument ?? (commonAncestorContainer as Document); const startMatches = startMatcher(scope); const endMatches = endMatcher(scope); diff --git a/packages/dom/src/text-quote/describe.ts b/packages/dom/src/text-quote/describe.ts index b048914..7ad9662 100644 --- a/packages/dom/src/text-quote/describe.ts +++ b/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, ): Promise<TextQuoteSelector> { range = range.cloneRange(); // Take the part of the range that falls within the scope. - const scopeAsRange = rangeFromScope(scope); - if (!scopeAsRange.isPointInRange(range.startContainer, range.startOffset)) - range.setStart(scopeAsRange.startContainer, scopeAsRange.startOffset); - if (!scopeAsRange.isPointInRange(range.endContainer, range.endOffset)) - range.setEnd(scopeAsRange.endContainer, scopeAsRange.endOffset); - - const exact = range.toString(); - - const result: TextQuoteSelector = { type: 'TextQuoteSelector', exact }; - - const { prefix, suffix } = calculateContextForDisambiguation( - range, - result, - scope, - ); - result.prefix = prefix; - result.suffix = suffix; - - return result; + if (!scope.isPointInRange(range.startContainer, range.startOffset)) + range.setStart(scope.startContainer, scope.startOffset); + if (!scope.isPointInRange(range.endContainer, range.endOffset)) + range.setEnd(scope.endContainer, scope.endOffset); + + return { + type: 'TextQuoteSelector', + exact: range.toString(), + ...calculateContextForDisambiguation(range, scope), + }; } function calculateContextForDisambiguation( range: Range, - selector: TextQuoteSelector, - scope: DomScope, + scope: Range, ): { prefix?: string; suffix?: string } { - const exactText = selector.exact; - const scopeText = rangeFromScope(scope).toString(); + const exactText = range.toString(); + const scopeText = scope.toString(); const targetStartIndex = getRangeTextPosition(range, scope); const targetEndIndex = targetStartIndex + exactText.length; @@ -153,23 +140,20 @@ function minimalSolution( } // Get the index of the first character of range within the text of scope. -function getRangeTextPosition(range: Range, scope: DomScope): number { - const scopeAsRange = rangeFromScope(scope); +function getRangeTextPosition(range: Range, scope: Range): number { const iter = document.createNodeIterator( - scopeAsRange.commonAncestorContainer, + scope.commonAncestorContainer, NodeFilter.SHOW_TEXT, { acceptNode(node: Text) { // Only reveal nodes within the range - return scopeAsRange.intersectsNode(node) + return scope.intersectsNode(node) ? NodeFilter.FILTER_ACCEPT : NodeFilter.FILTER_REJECT; }, }, ); - const scopeOffset = isTextNode(scopeAsRange.startContainer) - ? scopeAsRange.startOffset - : 0; + const scopeOffset = isTextNode(scope.startContainer) ? scope.startOffset : 0; if (isTextNode(range.startContainer)) return seek(iter, range.startContainer) + range.startOffset - scopeOffset; else return seek(iter, firstTextNodeInRange(range)) - scopeOffset; diff --git a/packages/dom/src/text-quote/match.ts b/packages/dom/src/text-quote/match.ts index a78d057..1bd9e92 100644 --- a/packages/dom/src/text-quote/match.ts +++ b/packages/dom/src/text-quote/match.ts @@ -18,19 +18,18 @@ * under the License. */ -import type { TextQuoteSelector } from '@annotator/selector'; +import type { Matcher, TextQuoteSelector } from '@annotator/selector'; import seek from 'dom-seek'; -import type { DomScope, DomMatcher } from '../types'; -import { ownerDocument, rangeFromScope } from '../scope'; - export function createTextQuoteSelectorMatcher( selector: TextQuoteSelector, -): DomMatcher { - return async function* matchAll(scope: DomScope) { - const document = ownerDocument(scope); - const scopeAsRange = rangeFromScope(scope); - const scopeText = scopeAsRange.toString(); +): Matcher<Range, Range> { + return async function* matchAll(scope) { + const { commonAncestorContainer } = scope; + const { ownerDocument } = commonAncestorContainer; + const document = ownerDocument ?? (commonAncestorContainer as Document); + + const scopeText = scope.toString(); const exact = selector.exact; const prefix = selector.prefix || ''; @@ -38,12 +37,12 @@ export function createTextQuoteSelectorMatcher( const searchPattern = prefix + exact + suffix; const iter = document.createNodeIterator( - scopeAsRange.commonAncestorContainer, + commonAncestorContainer, NodeFilter.SHOW_TEXT, { acceptNode(node: Text) { // Only reveal nodes within the range; and skip any empty text nodes. - return scopeAsRange.intersectsNode(node) && node.length > 0 + return scope.intersectsNode(node) && node.length > 0 ? NodeFilter.FILTER_ACCEPT : NodeFilter.FILTER_REJECT; }, @@ -51,8 +50,8 @@ export function createTextQuoteSelectorMatcher( ); // The index of the first character of iter.referenceNode inside the text. - let referenceNodeIndex = isTextNode(scopeAsRange.startContainer) - ? -scopeAsRange.startOffset + let referenceNodeIndex = isTextNode(scope.startContainer) + ? -scope.startOffset : 0; let fromIndex = 0; diff --git a/packages/dom/test/text-quote/describe.test.ts b/packages/dom/test/text-quote/describe.test.ts index 9219147..3d36c4e 100644 --- a/packages/dom/test/text-quote/describe.test.ts +++ b/packages/dom/test/text-quote/describe.test.ts @@ -32,7 +32,9 @@ describe('describeTextQuote', () => { for (const [name, { html, range, expected }] of Object.entries(testCases)) { it(`works for case: ${name}`, async () => { const doc = domParser.parseFromString(html, 'text/html'); - const result = await describeTextQuote(hydrateRange(range, doc), doc); + const scope = doc.createRange(); + scope.selectNodeContents(doc); + const result = await describeTextQuote(hydrateRange(range, doc), scope); assert.deepEqual(result, expected); }); } @@ -85,9 +87,11 @@ describe('describeTextQuote', () => { for (const [name, { html, selector, expected }] of applicableTestCases) { it(`case: '${name}'`, async () => { const doc = domParser.parseFromString(html, 'text/html'); + const scope = doc.createRange(); + scope.selectNodeContents(doc); for (const rangeInfo of expected) { const range = hydrateRange(rangeInfo, doc); - const result = await describeTextQuote(range, doc); + const result = await describeTextQuote(range, scope); assert.equal(result.exact, selector.exact); // Our result may have a different combination of prefix/suffix; only check for obvious inconsistency. if (selector.prefix && result.prefix) diff --git a/packages/dom/test/text-quote/match.test.ts b/packages/dom/test/text-quote/match.test.ts index 78b1239..7bdd83c 100644 --- a/packages/dom/test/text-quote/match.test.ts +++ b/packages/dom/test/text-quote/match.test.ts @@ -22,7 +22,6 @@ import { assert } from 'chai'; import type { TextQuoteSelector } from '@annotator/selector'; import { createTextQuoteSelectorMatcher } from '../../src/text-quote/match'; -import type { DomScope } from '../../src/types'; import { evaluateXPath, RangeInfo } from '../utils'; import { testCases } from './match-cases'; @@ -35,13 +34,21 @@ describe('createTextQuoteSelectorMatcher', () => { )) { it(`works for case: '${name}'`, async () => { const doc = domParser.parseFromString(html, 'text/html'); - await testMatcher(doc, doc, selector, expected); + + const scope = doc.createRange(); + scope.selectNodeContents(doc); + + await testMatcher(doc, scope, selector, expected); }); } it('handles adjacent text nodes', async () => { const { html, selector } = testCases['simple']; const doc = domParser.parseFromString(html, 'text/html'); + + const scope = doc.createRange(); + scope.selectNodeContents(doc); + const textNode = evaluateXPath(doc, '//b/text()') as Text; for (let index = textNode.length - 1; index > 0; index--) @@ -49,7 +56,7 @@ describe('createTextQuoteSelectorMatcher', () => { // console.log([...textNode.parentNode.childNodes].map(node => node.textContent)) // → 'l', 'o', 'r', 'e', 'm', … - await testMatcher(doc, doc, selector, [ + await testMatcher(doc, scope, selector, [ { startContainerXPath: '//b/text()[13]', startOffset: 0, @@ -63,6 +70,9 @@ describe('createTextQuoteSelectorMatcher', () => { const { html, selector } = testCases['simple']; const doc = domParser.parseFromString(html, 'text/html'); + const scope = doc.createRange(); + scope.selectNodeContents(doc); + const textNode = evaluateXPath(doc, '//b/text()') as Text; textNode.splitText(textNode.length); textNode.splitText(20); @@ -75,7 +85,7 @@ describe('createTextQuoteSelectorMatcher', () => { // console.log([...textNode.parentNode.childNodes].map(node => node.textContent)) // → '', 'lorem ipsum ', '', 'dolor', '', ' am', '', 'et yada yada', '' - await testMatcher(doc, doc, selector, [ + await testMatcher(doc, scope, selector, [ { startContainerXPath: '//b/text()[4]', // "dolor" startOffset: 0, @@ -89,24 +99,33 @@ describe('createTextQuoteSelectorMatcher', () => { const { html, selector, expected } = testCases['simple']; const doc = domParser.parseFromString(html, 'text/html'); - await testMatcher(doc, evaluateXPath(doc, '//b'), selector, expected); + const scope = doc.createRange(); + scope.selectNodeContents(evaluateXPath(doc, '//b')); + + await testMatcher(doc, scope, selector, expected); }); it('works with parent of text as scope, when matching its first characters', async () => { const { html, selector, expected } = testCases['first characters']; const doc = domParser.parseFromString(html, 'text/html'); - await testMatcher(doc, evaluateXPath(doc, '//b'), selector, expected); + const scope = doc.createRange(); + scope.selectNodeContents(evaluateXPath(doc, '//b')); + + await testMatcher(doc, scope, selector, expected); }); it('works with parent of text as scope, when matching its first characters, with an empty text node', async () => { const { html, selector } = testCases['first characters']; const doc = domParser.parseFromString(html, 'text/html'); + const scope = doc.createRange(); + scope.selectNodeContents(evaluateXPath(doc, '//b')); + const textNode = evaluateXPath(doc, '//b/text()') as Text; textNode.splitText(0); - await testMatcher(doc, evaluateXPath(doc, '//b'), selector, [ + await testMatcher(doc, scope, selector, [ { startContainerXPath: '//b/text()[2]', startOffset: 0, @@ -179,7 +198,7 @@ describe('createTextQuoteSelectorMatcher', () => { async function testMatcher( doc: Document, - scope: DomScope, + scope: Range, selector: TextQuoteSelector, expected: RangeInfo[], ) {
