Fix infinite sidebar loading in Firefox >=130 with PDFs #6541

acelaya · 2024-09-09T08:54:49Z

When using the bookmarklet in Firefox, we use the PDF.js version embedded on it.

Firefox 130 has released with a new version of PDF.js which removes the prop we used to use to know that the PDF was downloaded, cuasing the sidebar to stay in an infinite loading state. See the conversation in slack https://hypothes-is.slack.com/archives/C2BLQDKHA/p1725653987800389

This PR adds a fallback check to PDFViewerApplication.pdfDocument.getDownloadInfo(), which returns a promise when the PDF is downloaded, and is what used to be used to set the removed prop (see https://github.com/mozilla/pdf.js/pull/18463/files#diff-e24e4bd32090b0c400a70f02d1b844df95c9bbe62cd0b280f4397e9f4194fe44L1224), so it should be equivalent.

I did not remove the previous check, but combined both using the new one as a fallback. This will reduce potential regressions in older versions of PDF.js, Firefox, or when using other browsers. However, it seems to be possible to rely only on PDFViewerApplication.pdfDocument.getDownloadInfo(), as it has been around for several versions.

Testing steps

Check out this branch
Make sure you have a locally running version of via

This PR requires mostly sanity checking a few scenarios, being:

Bookmarklet in a page like https://pdfobject.com/pdf/sample.pdf: javascript:(function()%7Bwindow.hypothesisConfig=function()%7Breturn%7BshowHighlights:true,appType:'bookmarklet'%7D;%7D;var d=document,s=d.createElement('script');s.setAttribute('src','http://localhost:5000/embed.js');d.body.appendChild(s)%7D)();
Via: http://localhost:9082/https://pdfobject.com/pdf/sample.pdf
Client (embedded): http://localhost:3000/pdf/nils-olav

These three scenarios should be tested in Firefox 130, to verify it works now, but also Firefox <130 and other browsers (chrome and/or Safari) to verify there are no regressions for those.

acelaya · 2024-09-09T09:20:28Z

src/annotator/integrations/test/pdf-metadata-test.js

@@ -94,6 +94,7 @@ class FakePDFViewerApplication {
    this.dispatchDOMEvents = domEvents;
    this.initialized = false;
    this.newFingerprintAPI = newFingerprintAPI;
+    this.downloadComplete = false;


This was previously not initialized, relying on the "falsy" behavior of undefined.

Now we need to initialize to ensure the code branches that need to run are preserved.

codecov · 2024-09-09T09:22:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.43%. Comparing base (72117eb) to head (12830f4).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6541   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         270      270           
  Lines       10213    10219    +6     
  Branches     2416     2417    +1     
=======================================
+ Hits        10155    10161    +6     
  Misses         58       58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

acelaya · 2024-09-09T09:26:35Z

src/annotator/integrations/pdf-metadata.ts

@@ -70,9 +86,10 @@ export class PDFMetadata {
   * @param app - The `PDFViewerApplication` global from PDF.js
   */
  constructor(app: PDFViewerApplication) {
-    this._loaded = pdfViewerInitialized(app).then(() => {
+    this._loaded = pdfViewerInitialized(app).then(async () => {


I'm not going to change that here, as it may have other implications, but I think that we could simply do this:

this._loaded = pdfViewerInitialized(app) .then(() => app.pdfDocument.getDownloadInfo()) .then(() => app);

But we would probably have to drop support for some older versions of PDF.js

But we would probably have to drop support for some older versions of PDF.js

We need to clarify the supported range of PDF.js versions. I think there are some existing code comments somewhere. The main considerations would be:

The version of PDF.js used in Via and the current extension

Versions of PDF.js used in the last N Firefox releases

Which older versions of https://github.com/hypothesis/pdf.js-hypothes.is we want to support

Of these, (3) is the main potential source of old PDF.js versions still in use. We might need to do some metrics gathering in the client to find out how many requests are coming from old releases, and also add some kind of warning banner if the client is loaded in an old version that is unsupported.

I didn't know https://github.com/hypothesis/pdf.js-hypothes.is existed.

acelaya · 2024-09-09T09:29:17Z

src/types/pdfjs.ts

+  /**
+   * @return A promise that is resolved when the document's data is loaded.
+   *         It is resolved with an {Object} that contains the `length` property
+   *         that indicates size of the PDF data in bytes.


I basically copied the JSDoc from PDF.js repo https://github.com/mozilla/pdf.js/blob/a1b45d6e69d37b76e675a7f247943b70f186baca/src/display/api.js#L1024-L1028

acelaya · 2024-09-09T10:14:24Z

src/annotator/integrations/test/pdf-metadata-test.js

    } = {},
  ) {
    this.url = url;
    this.documentInfo = undefined;
    this.metadata = undefined;
-    this.pdfDocument = null;
+    this.pdfDocument = new FakePDFDocumentProxy();


We need this to be set from the beginning now, so I changed it so that it can be created "empty", but exposes a finishLoading method that is invoked when the fake app's own finishLoading method is called.

Is it possible in PDF.js that pdfDocument is unset either temporarily or permanently when Hypothesis is loaded? Do we handle that gracefully?

According to our type definitions, it is mandatory, but looking at the code, it can be null apparently https://github.com/mozilla/pdf.js/blob/f68310b7b1db39a934124acc90867a949da2d091/web/app.js#L971

Also, if you check that code, the prop is initialized only when load method is called.

I'll create an issue to try to reproduce what happens when it is not set and handle it properly.

marcospri

I can't offer much insight on the code changes themselves but I followed the test instructions in firefox 130.0-2 and 129.0.2-1 and everything works as expected.

robertknight

These changes generally look good in the happy path case. I had a query about whether we gracefully handle cases where the PDF has failed to load, has not yet started loading (is this possible) or the viewer was loaded without specifying a PDF.

robertknight · 2024-09-17T11:22:09Z

src/annotator/integrations/pdf-metadata.ts

+ * For newer PDF.js versions we wait for
+ * `PDFViewerApplication.pdfDocument.getDownloadInfo()` to resolve.
+ */
+async function isPDFDownloaded(app: PDFViewerApplication): Promise<boolean> {


This function has type Promise<boolean> but it only ever returns true. An API like whenPDFDownloaded(app): Promise<void> would probably give a better cue to readers about how it behaves.

This function has type Promise<boolean> but it only ever returns true

Hmm, I'm not sure if this is actually the case.

Previously we were relying in the downloadComplete prop, which could be true or false. Now it can also be undefined, but it can still be false I guess? 🤔

In that case, if the prop is set and is false, we would be falling back to the other logic that was already in place, which listens for documentloaded/documentload on the event bus.

Or am I missing something?

robertknight · 2024-09-17T11:26:42Z

src/annotator/integrations/pdf-metadata.ts

@@ -70,9 +86,10 @@ export class PDFMetadata {
   * @param app - The `PDFViewerApplication` global from PDF.js
   */
  constructor(app: PDFViewerApplication) {
-    this._loaded = pdfViewerInitialized(app).then(() => {
+    this._loaded = pdfViewerInitialized(app).then(async () => {


But we would probably have to drop support for some older versions of PDF.js

We need to clarify the supported range of PDF.js versions. I think there are some existing code comments somewhere. The main considerations would be:

The version of PDF.js used in Via and the current extension

Versions of PDF.js used in the last N Firefox releases

Which older versions of https://github.com/hypothesis/pdf.js-hypothes.is we want to support

Of these, (3) is the main potential source of old PDF.js versions still in use. We might need to do some metrics gathering in the client to find out how many requests are coming from old releases, and also add some kind of warning banner if the client is loaded in an old version that is unsupported.

robertknight · 2024-09-17T11:27:44Z

src/annotator/integrations/test/pdf-metadata-test.js

@@ -1,6 +1,7 @@
 import { delay } from '@hypothesis/frontend-testing';
 import EventEmitter from 'tiny-emitter';

+import { promiseWithResolvers } from '../../../shared/promise-with-resolvers';


We could replace this with Promise.withResolvers going forwards.

robertknight · 2024-09-17T11:33:47Z

src/annotator/integrations/test/pdf-metadata-test.js

    } = {},
  ) {
    this.url = url;
    this.documentInfo = undefined;
    this.metadata = undefined;
-    this.pdfDocument = null;
+    this.pdfDocument = new FakePDFDocumentProxy();


Is it possible in PDF.js that pdfDocument is unset either temporarily or permanently when Hypothesis is loaded? Do we handle that gracefully?

robertknight · 2024-09-17T11:34:43Z

src/types/pdfjs.ts

+  /**
+   * Indicates the download of the PDF has completed.
+   * This prop is not set in PDF.js >=4.5, in which case you should use
+   * `PDFViewerApplication.pdfDocument.getDownloadInfo()` instead.


A link to the commit that removed this property upstream would be useful.

acelaya force-pushed the pdfjs-firefox-fix branch 2 times, most recently from 19e490a to a0fae12 Compare September 9, 2024 09:19

acelaya commented Sep 9, 2024

View reviewed changes

acelaya marked this pull request as draft September 9, 2024 09:33

acelaya force-pushed the pdfjs-firefox-fix branch from a0fae12 to 0c92b50 Compare September 9, 2024 10:12

acelaya commented Sep 9, 2024

View reviewed changes

acelaya requested a review from marcospri September 9, 2024 10:14

acelaya marked this pull request as ready for review September 9, 2024 10:14

acelaya force-pushed the pdfjs-firefox-fix branch from 0c92b50 to 7dedfaf Compare September 9, 2024 10:23

Prevent accidental miss-clicks outside multi-select checkboxes

12830f4

acelaya force-pushed the pdfjs-firefox-fix branch from 7dedfaf to 12830f4 Compare September 9, 2024 10:24

marcospri approved these changes Sep 9, 2024

View reviewed changes

acelaya merged commit 4b60a07 into main Sep 9, 2024
4 checks passed

acelaya deleted the pdfjs-firefox-fix branch September 9, 2024 11:34

acelaya mentioned this pull request Sep 12, 2024

Fix annotations highlights not rendered in Firefox 130 #6548

Merged

robertknight reviewed Sep 17, 2024

View reviewed changes

acelaya mentioned this pull request Sep 17, 2024

Firefox v130+ fixes improvements #6559

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix infinite sidebar loading in Firefox >=130 with PDFs #6541

Fix infinite sidebar loading in Firefox >=130 with PDFs #6541

acelaya commented Sep 9, 2024 •

edited

Loading

acelaya Sep 9, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

acelaya Sep 9, 2024

robertknight Sep 17, 2024

acelaya Sep 17, 2024

acelaya Sep 9, 2024

acelaya Sep 9, 2024 •

edited

Loading

robertknight Sep 17, 2024

acelaya Sep 17, 2024

marcospri left a comment •

edited

Loading

robertknight left a comment

robertknight Sep 17, 2024

acelaya Sep 17, 2024

robertknight Sep 17, 2024

robertknight Sep 17, 2024

robertknight Sep 17, 2024

robertknight Sep 17, 2024

Fix infinite sidebar loading in Firefox >=130 with PDFs #6541

Fix infinite sidebar loading in Firefox >=130 with PDFs #6541

Conversation

acelaya commented Sep 9, 2024 • edited Loading

Testing steps

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri left a comment • edited Loading

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

acelaya Sep 9, 2024 •

edited

Loading

marcospri left a comment •

edited

Loading