Skip to content

Commit

Permalink
Fix annotations highlights not rendered in Firefox 130
Browse files Browse the repository at this point in the history
  • Loading branch information
acelaya committed Sep 12, 2024
1 parent 4b60a07 commit 9427453
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 4 deletions.
25 changes: 23 additions & 2 deletions src/annotator/anchoring/pdf.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
TextQuoteSelector,
Selector,
} from '../../types/api';
import type { PDFPageView, PDFViewer } from '../../types/pdfjs';
import type { PDFPageView, PDFViewer, TextLayer } from '../../types/pdfjs';
import { translateOffsets } from '../util/normalize';
import { matchQuote } from './match-quote';
import { createPlaceholder } from './placeholder';
Expand Down Expand Up @@ -259,6 +259,27 @@ function isSpace(char: string) {

const isNotSpace = (char: string) => !isSpace(char);

/**
* Determines if provided text layer is done rendering.
* It works on older PDF.js versions which expose a public renderingDone prop,
* and newer versions as well
*/
export function isTextLayerRenderingDone(textLayer: TextLayer): boolean {
if (textLayer.renderingDone !== undefined) {
return textLayer.renderingDone;
}

if (!textLayer.div) {
return false;
}

// When a Page is rendered, the div gets an element with the class
// endOfContent appended to it. If that element exists, we can consider the
// text layer is done rendering.
// See https://github.com/mozilla/pdf.js/blob/1ab9ab67eed886f27127bd801bc349949af5054e/web/text_layer_builder.js#L103-L107
return textLayer.div.querySelector('.endOfContent') !== null;
}

/**
* Locate the DOM Range which a position selector refers to.
*
Expand All @@ -285,7 +306,7 @@ async function anchorByPosition(
if (
page.renderingState === RenderingStates.FINISHED &&
page.textLayer &&
page.textLayer.renderingDone
isTextLayerRenderingDone(page.textLayer)
) {
// The page has been rendered. Locate the position in the text layer.
//
Expand Down
32 changes: 32 additions & 0 deletions src/annotator/anchoring/test/pdf-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { delay } from '@hypothesis/frontend-testing';

import { matchQuote } from '../match-quote';
import * as pdfAnchoring from '../pdf';
import { isTextLayerRenderingDone } from '../pdf';
import { TextRange } from '../text-range';
import { FakePDFViewerApplication } from './fake-pdf-viewer-application';

Expand Down Expand Up @@ -724,4 +725,35 @@ describe('annotator/anchoring/pdf', () => {
});
});
});

describe('isTextLayerRenderingDone', () => {
[true, false].forEach(renderingDone => {
it('returns renderingDone if present', () => {
assert.equal(
isTextLayerRenderingDone({ renderingDone }),
renderingDone,
);
});
});

it('returns false if neither renderingDone nor the div are set', () => {
assert.isFalse(isTextLayerRenderingDone({}));
});

[
{ element: null, expectedResult: false },
{ element: {}, expectedResult: true },
].forEach(({ element, expectedResult }) => {
it('returns true if the div contains an endOfContent element', () => {
assert.equal(
isTextLayerRenderingDone({
div: {
querySelector: () => element,
},
}),
expectedResult,
);
});
});
});
});
3 changes: 2 additions & 1 deletion src/annotator/integrations/pdf.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
canDescribe,
describe,
documentHasText,
isTextLayerRenderingDone,
} from '../anchoring/pdf';
import { isInPlaceholder, removePlaceholder } from '../anchoring/placeholder';
import { TextRange } from '../anchoring/text-range';
Expand Down Expand Up @@ -356,7 +357,7 @@ export class PDFIntegration extends TinyEmitter implements Integration {
const pageCount = this._pdfViewer.pagesCount;
for (let pageIndex = 0; pageIndex < pageCount; pageIndex++) {
const page = this._pdfViewer.getPageView(pageIndex);
if (!page?.textLayer?.renderingDone) {
if (!page?.textLayer || !isTextLayerRenderingDone(page.textLayer)) {
continue;
}

Expand Down
6 changes: 5 additions & 1 deletion src/types/pdfjs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,11 @@ export type PDFViewerApplication = {
};

export type TextLayer = {
renderingDone: boolean;
/**
* This prop is private in PDF.js >=4.5, so we cannot safely trust it's
* publicly exposed
*/
renderingDone?: boolean;
/**
* New name for root element of text layer in PDF.js >= v3.2.146
*/
Expand Down

0 comments on commit 9427453

Please sign in to comment.