Skip to content

Commit

Permalink
fixup! Render high-res partial page views when falling back to CSS zo…
Browse files Browse the repository at this point in the history
…om (bug 1492303)

Clean up the PDFDetailView after cancellation synchronously.

This ensures that, when rendering is cancelled, we always restore tracking
the canvas that is still in the DOM (done in the `onCancel` callback of
`_drawCanvas`) before calling `.remove()` on whatever the tracked canvas
is.

Given this algorithm that starts with a 1000% zoom already rendered,
with a detail view canvas that we'll call #1:
- zoom 0.3x (!needs detail view)
- await 1 microtick (Promise.resolve())
- zoom 0.3x (!does not need detail view)

Before this patch, the order of operations would be:
- Create a new detail canvas #2 for the 300% zoom level
- Change detailView.canvas from #1 to #2
- await 1 microtick (between the two zoom changes)
- In .reset(), cancel the rendering
- In .reset(), call detailView.canvas.remove() (removing #2, even if the
  one in the dom is actually still #1)
- Set .detailView to `null`, because we don't need it anymore.
- await, because the `onCancel` logic was run asyncronously
- Change detailView.canvas from #2 to #1

After this patch, the order becomes:
- Create a new detail canvas #2 for the 300% zoom level
- Change detailView.canvas from #1 to #2
- await 1 microtick (between the two zoom changes)
- In .reset(), cancel the rendering
  - It runs `onCancel`, changing detailView.canvas from #2 to #1
- In .reset(), call detailView.canvas.remove() (which now removes #1)
- Set .detailView to `null`, because we don't need it anymore.
  • Loading branch information
nicolo-ribaudo committed Feb 5, 2025
1 parent 7d16a5a commit 9c2f1fc
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 22 deletions.
28 changes: 20 additions & 8 deletions src/display/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -1544,6 +1544,8 @@ class PDFPageProxy {
intentState,
reason: error instanceof Error ? error : new Error(error),
});

internalRenderTask.task.onError?.(error);
} else {
internalRenderTask.capability.resolve();
}
Expand Down Expand Up @@ -3289,17 +3291,27 @@ class PDFObjects {
class RenderTask {
#internalRenderTask = null;

/**
* Callback for incremental rendering -- a function that will be called
* each time the rendering is paused. To continue rendering call the
* function that is the first argument to the callback.
* @type {function}
*/
onContinue = null;

/**
* A function that will be synchronously called when the rendering tasks
* finishes with an error (either because of an actual error, or because the
* rendering is cancelled)
*
* @type {function}
* @param {Error} error
*/
onError = null;

constructor(internalRenderTask) {
this.#internalRenderTask = internalRenderTask;

/**
* Callback for incremental rendering -- a function that will be called
* each time the rendering is paused. To continue rendering call the
* function that is the first argument to the callback.
* @type {function}
*/
this.onContinue = null;

if (typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING")) {
// For testing purposes.
Object.defineProperty(this, "getOperatorList", {
Expand Down
67 changes: 67 additions & 0 deletions test/integration/viewer_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1028,5 +1028,72 @@ describe("PDF viewer", () => {
});
});
});

describe("when cancelled and re-rendered after 1 microtick", () => {
const forEachPage = setupPages("100%", 1, {
eventBusSetup: eventBus => {
globalThis.__pageRenderedEvents = [];
eventBus.on("pagerendered", ({ pageNumber, isDetailView }) => {
globalThis.__pageRenderedEvents.push({ pageNumber, isDetailView });
});
},
});

it("propely cleans up old canvases from the dom", async () => {
await forEachPage(async (browserName, page) => {
const waitForPageRenderedEvent = filter =>
page.waitForFunction(
filterStr => {
// eslint-disable-next-line no-eval
if (globalThis.__pageRenderedEvents.some(eval(filterStr))) {
globalThis.__pageRenderedEvents = [];
return true;
}
return false;
},
{ polling: 50 },
String(filter)
);
const getCanvasCount = () =>
page.evaluate(
() =>
document.querySelector("[data-page-number='1'] .canvasWrapper")
.childElementCount
);

await waitForPageRenderedEvent(e => e.pageNumber === 1);

await page.evaluate(() => {
window.PDFViewerApplication.pdfViewer.updateScale({
drawingDelay: -1,
scaleFactor: 4,
});
});
await waitForPageRenderedEvent(
e => e.pageNumber === 1 && e.isDetailView
);
expect(await getCanvasCount())
.withContext(`In ${browserName}, after the first zoom-in`)
.toBe(2);

await page.evaluate(() => {
window.PDFViewerApplication.pdfViewer.updateScale({
drawingDelay: -1,
scaleFactor: 0.75,
});
Promise.resolve().then(() => {
window.PDFViewerApplication.pdfViewer.updateScale({
drawingDelay: -1,
scaleFactor: 0.25,
});
});
});
await waitForPageRenderedEvent(e => e.pageNumber === 1);
expect(await getCanvasCount())
.withContext(`In ${browserName}, after the two zoom-out`)
.toBe(1);
});
});
});
});
});
29 changes: 15 additions & 14 deletions web/base_pdf_page_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,12 @@ class BasePDFPageView {
async _drawCanvas(options, onCancel, onFinish) {
const renderTask = (this.renderTask = this.pdfPage.render(options));
renderTask.onContinue = this.#renderContinueCallback;
renderTask.onError = error => {
if (error instanceof RenderingCancelledException) {
onCancel();
this.#renderError = null;
}
};

let error = null;
try {
Expand All @@ -169,22 +175,17 @@ class BasePDFPageView {
// a black canvas if rendering was cancelled before the `onContinue`-
// callback had been invoked at least once.
if (error instanceof RenderingCancelledException) {
onCancel();
} else {
this.#showCanvas?.(true);
return;
}
}

// The renderTask may have been replaced by a new one, so only remove
// the reference to the renderTask if it matches the one that is
// triggering this callback.
if (renderTask === this.renderTask) {
this.renderTask = null;
}

if (error instanceof RenderingCancelledException) {
this.#renderError = null;
return;
this.#showCanvas?.(true);
} finally {
// The renderTask may have been replaced by a new one, so only remove
// the reference to the renderTask if it matches the one that is
// triggering this callback.
if (renderTask === this.renderTask) {
this.renderTask = null;
}
}
this.#renderError = error;

Expand Down

0 comments on commit 9c2f1fc

Please sign in to comment.