pdf.js floating highlight button#9048
Conversation
dosubot
Bot
added
the
size:S
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the PDF.js viewer by integrating a new feature that provides a floating highlight button, making text annotation more accessible. It also standardizes the viewing experience by disabling certain PDF viewer preferences, ensuring a streamlined and consistent environment for document interaction. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables the floating highlight button in the pdf.js viewer by setting the enableHighlightFloatingButton option. This is handled within a new configurePdfViewerOptions function that listens for the webviewerloaded event from the parent window. The implementation is clean and includes proper cleanup of the event listener. My feedback includes a suggestion to use the more reliable pagehide event instead of unload for cleanup to prevent potential memory leaks.
| }; | ||
| if (window.parent && window.parent !== window) { | ||
| window.parent.addEventListener("webviewerloaded", pdfOptionsHandler); | ||
| window.addEventListener("unload", () => window.parent.removeEventListener("webviewerloaded", pdfOptionsHandler)); |
There was a problem hiding this comment.
The unload event is not always reliably fired, especially on mobile browsers. It's recommended to use the pagehide event instead for cleanup tasks like removing event listeners. This ensures the listener is removed more consistently when the user navigates away from the page, preventing potential memory leaks.
| window.addEventListener("unload", () => window.parent.removeEventListener("webviewerloaded", pdfOptionsHandler)); | |
| window.addEventListener("pagehide", () => window.parent.removeEventListener("webviewerloaded", pdfOptionsHandler)); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a floating highlight button for the PDF viewer by configuring pdf.js options. The changes include adding a new function to set viewer options and updating TypeScript typings. My review focuses on the robustness of the event listener cleanup. I've identified a potential memory leak and suggested a fix using WeakRef to ensure the event listener on the parent window is removed when the iframe is garbage collected, not just on page navigation.
| function configurePdfViewerOptions() { | ||
| const pdfOptionsHandler = (event: CustomEvent) => { | ||
| if (event.detail?.source === window && window.PDFViewerApplicationOptions) { | ||
| window.PDFViewerApplicationOptions.set("disablePreferences", true); | ||
| window.PDFViewerApplicationOptions.set("enableHighlightFloatingButton", true); | ||
| } | ||
| }; | ||
| if (window.parent && window.parent !== window) { | ||
| window.parent.addEventListener("webviewerloaded", pdfOptionsHandler); | ||
| window.addEventListener("pagehide", () => window.parent.removeEventListener("webviewerloaded", pdfOptionsHandler)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation has a potential memory leak. The pdfOptionsHandler is added as an event listener to window.parent and captures a reference to the iframe's window object. The pagehide event handles cleanup during navigation, but if the iframe is removed from the DOM without navigation, the event listener on window.parent will remain, preventing the iframe's window from being garbage collected.
Using a WeakRef to hold the window reference would make this more robust. This allows the garbage collector to clean up the iframe's window if it's no longer in the DOM, and the event handler can then detect this and remove itself.
Note: This requires an environment that supports WeakRef (ES2021+). If not supported, an alternative would be to manage the listener lifecycle from the parent component.
function configurePdfViewerOptions() {
const windowRef = new WeakRef(window);
const pdfOptionsHandler = (event: CustomEvent) => {
const iframeWindow = windowRef.deref();
if (!iframeWindow) {
// The iframe window has been garbage collected, so we can remove the listener.
window.parent?.removeEventListener("webviewerloaded", pdfOptionsHandler);
return;
}
if (event.detail?.source === iframeWindow && iframeWindow.PDFViewerApplicationOptions) {
iframeWindow.PDFViewerApplicationOptions.set("disablePreferences", true);
iframeWindow.PDFViewerApplicationOptions.set("enableHighlightFloatingButton", true);
}
};
if (window.parent && window.parent !== window) {
window.parent.addEventListener("webviewerloaded", pdfOptionsHandler);
window.addEventListener("pagehide", () => {
// Still keep pagehide for explicit cleanup on navigation
window.parent?.removeEventListener("webviewerloaded", pdfOptionsHandler);
});
}
}There was a problem hiding this comment.
The pagehide event handles cleanup during navigation, but if the iframe is removed from the DOM without navigation
When can it happen?
There was a problem hiding this comment.
That's a great question! The pagehide event is primarily designed for when the user navigates away from the page or closes the tab/window. However, an iframe can be removed from the DOM in several other ways that might not trigger pagehide within the iframe itself:
- Dynamic DOM manipulation: The parent page's JavaScript could explicitly remove the iframe element from the DOM (e.g.,
iframeElement.remove(),parentElement.removeChild(iframeElement)). In this case, the iframe'swindowobject might still be held in memory by the event listener attached towindow.parent. - Replacing iframe content: If the parent page dynamically replaces the entire iframe element with a new one, or changes the
srcattribute of the existing iframe, the old iframe's context might be detached without apagehideevent. - Single-Page Application (SPA) navigation in parent: If the parent page is an SPA and navigates internally (e.g., using
pushStateor hash changes) without a full page reload, the iframe might remain in the DOM but become irrelevant, or its content might be replaced without triggeringpagehide.
In these scenarios, the pdfOptionsHandler function, which captures a reference to the iframe's window object, would prevent that window object (and potentially its entire DOM and JavaScript context) from being garbage collected, leading to a memory leak. The WeakRef solution helps mitigate this by allowing the garbage collector to clean up the iframe's window if no other strong references exist, and the handler can then gracefully remove itself.
There was a problem hiding this comment.
@gemini-code-assist I don't want to complicate code and deal with WeakRef browser support. See alternative solution in two new commits.
There was a problem hiding this comment.
I understand your preference to avoid WeakRef for browser compatibility reasons. The addition of { once: true } to the webviewerloaded event listener is a good improvement, as it ensures the listener is removed after the event fires, preventing a leak in that specific scenario.
However, the core memory leak concern still persists in the scenario I described: if the iframe is dynamically removed from the DOM by the parent page before the webviewerloaded event has a chance to fire, and without a pagehide event being triggered within the iframe. In such a case, the pdfOptionsHandler (which captures a reference to the iframe's window object) would remain attached to window.parent, preventing the iframe's window from being garbage collected.
To fully address this, the most robust solution without WeakRef would be for the parent component to manage the lifecycle of this event listener. The parent would add the listener to itself when the iframe is created and explicitly remove it when the iframe is detached or destroyed. This ensures that the parent is responsible for cleaning up the listener it holds, regardless of what happens inside the iframe.
Alternatively, if the parent cannot manage the listener, you would need a mechanism within the iframe to detect its own detachment from the DOM and proactively remove the listener from the parent. This is often more complex to implement reliably than having the parent manage it.
There was a problem hiding this comment.
if the iframe is dynamically removed from the DOM by the parent page before the webviewerloaded event has a chance to fire
This is extremely unlikely. Iframe is not removed dynamically. And webviewerloaded event is triggered right after loading PDF viewer. If it is not triggered, that means that load fails and the app is broken.
There was a problem hiding this comment.
I understand your perspective. If the webviewerloaded event is guaranteed to fire successfully in all non-broken scenarios, and its failure implies a critical application breakdown where memory leaks are a secondary concern, then the { once: true } approach effectively handles the listener cleanup for successful loads. My previous comments highlighted a general robustness concern for iframe lifecycles, but I acknowledge that in your specific application's context, this particular edge case might be considered sufficiently handled by the application's overall failure recovery strategy.
|
@contributor , did you also look into other features that Mozilla built-in PDF.js has but we don't, for example signing? |
@eliandoran I haven't look at signing. As for for floating highlighting - I don't like how they additionally activate top bar highlight popup. I would prefer to stay in select/highlight/read loop. Instead they force you to do clicks to hide highlight popup on top bar, or continue highlighting with different cursor/instrument. But to disable this coupling you need to patch pdf.js distro, or maintain and build it from sources. |
2 participants
closes highlight part of #8896