diff --git a/lms/static/scripts/frontend_apps/components/BookPicker.tsx b/lms/static/scripts/frontend_apps/components/BookPicker.tsx index f548bf9c77..be55adf1f9 100644 --- a/lms/static/scripts/frontend_apps/components/BookPicker.tsx +++ b/lms/static/scripts/frontend_apps/components/BookPicker.tsx @@ -5,6 +5,7 @@ import { useCallback, useMemo, useEffect, useState } from 'preact/hooks'; import type { Book, TableOfContentsEntry } from '../api-types'; import { useService, VitalSourceService } from '../services'; import type { ContentRange, Selection } from '../services/vitalsource'; +import { documentCFI } from '../utils/cfi'; import { isPageRangeValid } from '../utils/vitalsource'; import BookSelector from './BookSelector'; import ErrorDisplay from './ErrorDisplay'; @@ -123,8 +124,17 @@ export default function BookPicker({ setStep('select-toc'); }, []); - // Return the next entry after `entry` which is at the same or higher level - // in the table of contents. + // Find the table of contents entry which comes after `entry`. This serves + // as the exclusive end point for a selection starting at `entry`. + // + // More specifically this finds the first entry after `entry` which: + // + // 1. Is at the same or higher level in the table of contents. + // 2. Refers to a different segment (HTML or PDF page) within the book. + // + // Condition (2) is needed because the minimum granularity that the Hypothesis + // client can filter annotations by is a single HTML or PDF page. Table of + // contents entries in the ePUB however can be more fine-grained than this. const nextEntryAfter = useCallback( (entry: TableOfContentsEntry) => { /* istanbul ignore next - early exit should be unreachable */ @@ -133,11 +143,14 @@ export default function BookPicker({ } const idx = tableOfContents.indexOf(entry); const level = entry.level ?? 0; + const currentDocCFI = documentCFI(entry.cfi); let nextEntry; for (let i = idx + 1; i < tableOfContents.length; i++) { - const entryLevel = tableOfContents[i].level ?? 0; - if (entryLevel <= level) { + const entryAtIndex = tableOfContents[i]; + const entryDocCFI = documentCFI(entryAtIndex.cfi); + const entryLevel = entryAtIndex.level ?? 0; + if (entryLevel <= level && entryDocCFI !== currentDocCFI) { nextEntry = tableOfContents[i]; break; } diff --git a/lms/static/scripts/frontend_apps/components/test/BookPicker-test.js b/lms/static/scripts/frontend_apps/components/test/BookPicker-test.js index a003ef593f..7c083e11e6 100644 --- a/lms/static/scripts/frontend_apps/components/test/BookPicker-test.js +++ b/lms/static/scripts/frontend_apps/components/test/BookPicker-test.js @@ -21,6 +21,11 @@ const fakeBookData = { title: 'Book Two', cover_image: 'https://bookstore.com/covers/book2.jpg', }, + book3: { + id: 'book3', + title: 'Book Three', + cover_image: 'https://bookstore.com/covers/book3.jpg', + }, }, chapters: { @@ -49,6 +54,31 @@ const fakeBookData = { page: '7', }, ], + + book3: [ + // EPUB book which is divided into two HTML pages. For the first page, + // there are multiple TOC entries. + { + title: 'Chapter A', + cfi: '/2', + page: '3', + }, + { + title: 'Chapter A - Part 1', + cfi: '/2!/2', + page: '3', + }, + { + title: 'Chapter A - Part 2', + cfi: '/2!/4', + page: '4', + }, + { + title: 'Chapter B', + cfi: '/4', + page: '7', + }, + ], }, }; @@ -88,9 +118,9 @@ describe('BookPicker', () => { $imports.$restore(); }); - const selectBook = wrapper => { + const selectBook = (wrapper, bookId = 'book1') => { const bookSelector = wrapper.find('BookSelector'); - const book = fakeBookData.books.book1; + const book = fakeBookData.books[bookId]; act(() => { bookSelector.props().onSelectBook(book); @@ -236,61 +266,113 @@ describe('BookPicker', () => { }); }); - it('displays page numbers corresponding to selected chapter', async () => { + [ + // Select a TOC entry which corresponds to a whole HTML document. + { + book: 'book1', + chapter: 0, + expectedEndChapter: 1, + expectedStartPage: '1', + expecteEndPage: '10', + }, + // Select a TOC entry which corresponds to a whole HTML document, and has + // child entries. + { + book: 'book3', + chapter: 0, + expectedEndChapter: 3, + expectedStartPage: '3', + expecteEndPage: '7', + }, + // Select a TOC entry which corresponds to part of an HTML document. The + // selection should be expanded to the whole page. + { + book: 'book3', + chapter: 1, // Chapter A - Part 1 + expectedEndChapter: 3, + expectedStartPage: '3', + expecteEndPage: '7', + }, + ].forEach( + ({ + book, + chapter, + expectedEndChapter, + expectedStartPage, + expecteEndPage, + }) => { + it('selects expected page number and CFI range when TOC entry is clicked', async () => { + const onSelectBook = sinon.stub(); + const picker = renderBookPicker({ + allowPageRangeSelection: true, + onSelectBook, + }); + + selectBook(picker, book); + clickSelectButton(picker); + + await waitForTableOfContents(picker); + selectChapter(picker, chapter); + + // Check page range displayed after selecting entry. + const startInput = picker.find('input[data-testid="start-page"]'); + const endInput = picker.find('input[data-testid="end-page"]'); + assert.equal(startInput.prop('placeholder'), expectedStartPage); + assert.equal(endInput.prop('placeholder'), expecteEndPage); + + // Check selection passed to callback when submitting form. + clickSelectButton(picker); + await waitFor(() => onSelectBook.called); + + const startEntry = fakeBookData.chapters[book][chapter]; + const endEntry = fakeBookData.chapters[book][expectedEndChapter]; + const bookData = fakeBookData.books[book]; + + assert.calledWith( + onSelectBook, + { + book: bookData, + content: { + type: 'toc', + start: startEntry, + end: endEntry, + }, + }, + `vitalsource://books/bookID/${bookData.id}/cfi/${startEntry.cfi}`, + ); + }); + }, + ); + + // This test covers only the case where `allowPageRangeSelection` is false. + // The case where it is true is handled above. + it('invokes `onSelectBook` callback after a book and chapter are chosen', async () => { const onSelectBook = sinon.stub(); const picker = renderBookPicker({ - allowPageRangeSelection: true, + allowPageRangeSelection: false, onSelectBook, }); - selectBook(picker); + const book = selectBook(picker); clickSelectButton(picker); await waitForTableOfContents(picker); - selectChapter(picker); - - const startInput = picker.find('input[data-testid="start-page"]'); - const endInput = picker.find('input[data-testid="end-page"]'); - assert.equal(startInput.prop('placeholder'), '1'); - assert.equal(endInput.prop('placeholder'), '10'); - }); - - [ - { allowPageRangeSelection: true }, - { allowPageRangeSelection: false }, - ].forEach(({ allowPageRangeSelection }) => { - it('invokes `onSelectBook` callback after a book and chapter are chosen', async () => { - const onSelectBook = sinon.stub(); - const picker = renderBookPicker({ - allowPageRangeSelection, - onSelectBook, - }); - - const book = selectBook(picker); - clickSelectButton(picker); - - await waitForTableOfContents(picker); - const chapter = selectChapter(picker); - clickSelectButton(picker); + const chapter = selectChapter(picker); + clickSelectButton(picker); - const expectedEnd = allowPageRangeSelection - ? fakeBookData.chapters.book1[1] // Start of chapter after selection - : undefined; - - await waitFor(() => onSelectBook.called); - assert.calledWith( - onSelectBook, - { - book, - content: { - type: 'toc', - start: chapter, - end: expectedEnd, - }, + await waitFor(() => onSelectBook.called); + assert.calledWith( + onSelectBook, + { + book, + content: { + type: 'toc', + start: chapter, + end: undefined, }, - 'vitalsource://books/bookID/book1/cfi//1', - ); - }); + }, + 'vitalsource://books/bookID/book1/cfi//1', + ); }); it('invokes `onSelectBook` callback after a book and page range are chosen', async () => { diff --git a/lms/static/scripts/frontend_apps/utils/cfi.ts b/lms/static/scripts/frontend_apps/utils/cfi.ts new file mode 100644 index 0000000000..63bf2e828e --- /dev/null +++ b/lms/static/scripts/frontend_apps/utils/cfi.ts @@ -0,0 +1,68 @@ +/** + * Functions for working with EPUB Canonical Fragment Identifiers. + * + * See https://idpf.org/epub/linking/cfi/. + */ + +/** + * Strip assertions from a Canonical Fragment Identifier. + * + * Assertions are `[...]` enclosed sections which act as checks on the validity + * of numbers but do not affect the sort order. + * + * @example + * stripCFIAssertions("/6/14[chap05ref]") // returns "/6/14" + */ +export function stripCFIAssertions(cfi: string): string { + // Fast path for CFIs with no assertions. + if (!cfi.includes('[')) { + return cfi; + } + + let result = ''; + + // Has next char been escaped? + let escaped = false; + + // Are we in a `[...]` assertion section? + let inAssertion = false; + + for (const ch of cfi) { + if (!escaped && ch === '^') { + escaped = true; + continue; + } + + if (!escaped && ch === '[') { + inAssertion = true; + } else if (!escaped && inAssertion && ch === ']') { + inAssertion = false; + } else if (!inAssertion) { + result += ch; + } + + escaped = false; + } + + return result; +} + +/** + * Return a slice of `cfi` up to the first step indirection [1], with assertions + * removed. + * + * A typical CFI consists of a path within the table of contents to indicate + * a content document, a step indirection ("!"), then the path of an element + * within the content document. For such a CFI, this function will retain only + * the content document path. + * + * [1] https://idpf.org/epub/linking/cfi/#sec-path-indirection + * + * @example + * documentCFI('/6/152[;vnd.vst.idref=ch13_01]!/4/2[ch13_sec_1]') // Returns "/6/152" + */ +export function documentCFI(cfi: string): string { + const stripped = stripCFIAssertions(cfi); + const sepIndex = stripped.indexOf('!'); + return sepIndex === -1 ? stripped : stripped.slice(0, sepIndex); +} diff --git a/lms/static/scripts/frontend_apps/utils/test/cfi-test.js b/lms/static/scripts/frontend_apps/utils/test/cfi-test.js new file mode 100644 index 0000000000..13a83e5e2c --- /dev/null +++ b/lms/static/scripts/frontend_apps/utils/test/cfi-test.js @@ -0,0 +1,39 @@ +import { documentCFI, stripCFIAssertions } from '../cfi'; + +describe('cfi', () => { + describe('stripCFIAssertions', () => { + it('returns CFI without assertions unchanged', () => { + assert.equal(stripCFIAssertions('/1/2/3/10'), '/1/2/3/10'); + }); + + it('removes assertions from CFI', () => { + assert.equal(stripCFIAssertions('/1/2[chap4ref]'), '/1/2'); + assert.equal( + stripCFIAssertions('/1[part1ref]/2[chapter2ref]/3[subsectionref]'), + '/1/2/3', + ); + }); + + it('ignores escaped characters', () => { + assert.equal(stripCFIAssertions('/1/2[chap4^[ignoreme^]ref]'), '/1/2'); + assert.equal(stripCFIAssertions('/1/2[a^[b^]]/3[c^[d^]]'), '/1/2/3'); + }); + }); + + describe('documentCFI', () => { + it('returns part of CFI before first step indirection', () => { + // Typical CFI with one step indirection. + assert.equal(documentCFI('/2/4/8!/10/12'), '/2/4/8'); + + // Rarer case of CFI with multiple step indirections. + assert.equal(documentCFI('/2/4/8!/10/12!/2/4'), '/2/4/8'); + }); + + it('strips assertions', () => { + assert.equal( + documentCFI('/6/152[;vnd.vst.idref=ch13_01]!/4/2[ch13_sec_1]'), + '/6/152', + ); + }); + }); +});