Commit a2336a7c authored by Robert Knight's avatar Robert Knight

Add missing page index bounds check

When anchoring an annotation with a position and a quote selector, if
the position selector fails, then PDF anchoring searches page contents
starting with those pages nearest the position.

If the position selector's `start` offset was greater than the length of
the PDF's text, `prioritizePages` would try to fetch the text of page
indexes beyond the valid range, causing PDF.js to throw an exception and
quote anchoring to fail.

Fix this by adding a missing bounds check.

This is a partial fix for #558. It fixes anchoring of one of two test
annotations on that page. The other fails due to differences in the
extracted text between the HTML and PDF versions of the article.
parent d2eaac97
...@@ -97,7 +97,8 @@ findPage = (offset) -> ...@@ -97,7 +97,8 @@ findPage = (offset) ->
# 150 | 2 # 150 | 2
# #
count = (textContent) -> count = (textContent) ->
if total + textContent.length > offset lastPageIndex = PDFViewerApplication.pdfViewer.pagesCount - 1
if total + textContent.length > offset or index == lastPageIndex
offset = total offset = total
return Promise.resolve({index, offset, textContent}) return Promise.resolve({index, offset, textContent})
else else
......
...@@ -35,7 +35,7 @@ var fixtures = { ...@@ -35,7 +35,7 @@ var fixtures = {
], ],
}; };
describe('PDF anchoring', function () { describe('annotator.anchoring.pdf', function () {
var container; var container;
var viewer; var viewer;
...@@ -166,40 +166,31 @@ describe('PDF anchoring', function () { ...@@ -166,40 +166,31 @@ describe('PDF anchoring', function () {
}); });
}); });
it('anchors using a quote if the position anchor fails', function () { [{
// Position on same page as quote but different text.
offset: 5,
},{
// Position on a different page to the quote.
offset: fixtures.pdfPages[0].length + 10,
},{
// Position invalid for document.
offset: 100000,
}].forEach(({ offset }) => {
it('anchors using a quote if the position selector fails', function () {
viewer.setCurrentPage(0); viewer.setCurrentPage(0);
var range = findText(container, 'Pride And Prejudice'); var range = findText(container, 'Pride And Prejudice');
return pdfAnchoring.describe(container, range).then(function (selectors) { return pdfAnchoring.describe(container, range).then(function (selectors) {
var position = selectors[0]; var position = selectors[0];
var quote = selectors[1]; var quote = selectors[1];
// Manipulate the position selector so that it is no longer valid. position.start += offset;
// Anchoring should fall back to the quote selector instead. position.end += offset;
position.start += 5;
position.end += 5;
return pdfAnchoring.anchor(container, [position, quote]); return pdfAnchoring.anchor(container, [position, quote]);
}).then(function (range) { }).then(range => {
assert.equal(range.toString(), 'Pride And Prejudice'); assert.equal(range.toString(), 'Pride And Prejudice');
}); });
}); });
it('anchors using a quote if the position selector refers to the wrong page', function () {
viewer.setCurrentPage(0);
var range = findText(container, 'Pride And Prejudice');
return pdfAnchoring.describe(container, range).then(function (selectors) {
var position = selectors[0];
var quote = selectors[1];
// Manipulate the position selector so that it refers to a location
// a long way away, on a different page, than the quote.
position.start += fixtures.pdfPages[0].length + 10;
position.end += fixtures.pdfPages[0].length + 10;
return pdfAnchoring.anchor(container, [position, quote]);
}).then(function (range) {
assert.equal(range.toString(), 'Pride And Prejudice');
});
}); });
it('anchors to a placeholder element if the page is not rendered', function () { it('anchors to a placeholder element if the page is not rendered', function () {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment