Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207 #2641

robertknight · 2020-10-14T13:13:51Z

f96af43 added support for PDF.js >= v2.5.207 by changing an event
listener to use PDF.js's internal event bus rather than DOM events.
However there was another DOM event listener for the pagesloaded event
in src/annotator/anchoring/pdf.js that should also have been updated but was
overlooked. This didn't cause a problem in testing with the dev server because
the test documents load quickly enough that they are already loaded by the time
the client's anchoring logic ran.

This commit updates the way that the client listens for events from
PDF.js to use the event bus where available and only fall back to the
DOM in versions of PDF.js that don't support it.

Use PDF.js's event bus to listen for documentload/documentloaded
and pagesloaded events
Add a fallback method to wait for event bus to become available in
versions of PDF.js which support the eventBus but don't have the
initializedPromise API
Improve the documentation around which versions of PDF.js support
different event types and event dispatch methods
Add tests to cover the behavior from different releases of PDF.js

For an overview of the different versions of PDF.js that the client
needs to support, see #2643.

TODO

Re-test manually in 1) the version of Hypothesis that ships with the dev server, 2) the version that ships with Via and 3) the v1.1.114 release that shipped with https://github.com/hypothesis/pdf.js-hypothes.is until November 2019
Add tests to cover fallback paths that wait for PDF viewer to be initialized

codecov · 2020-10-14T13:16:11Z

Codecov Report

Merging #2641 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2641   +/-   ##
=======================================
  Coverage   97.39%   97.40%           
=======================================
  Files         200      200           
  Lines        7487     7503   +16     
  Branches     1646     1653    +7     
=======================================
+ Hits         7292     7308   +16     
  Misses        195      195

Impacted Files	Coverage Δ
src/annotator/anchoring/pdf.js	`99.41% <100.00%> (+0.01%)`	⬆️
src/annotator/plugin/pdf-metadata.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da788da...5da3feb. Read the comment docs.

lyzadanger

I'm not going to pretend this is straightforward :), but your commenting will go a long ways toward making this feasible to maintain for future devs!

lyzadanger · 2020-10-15T12:59:22Z

src/annotator/plugin/pdf-metadata.js

-        app.eventBus?.off('documentloaded', finish);
+    this._loaded = pdfViewerInitialized(app).then(() => {
+      // Check if document has already loaded.
+      if (app.downloadComplete) {


Is there any version difference here? Or is downloadComplete relevant to all of our supported versions of PDFjs?

I'm not aware of any version differences with downloadComplete, but I also haven't looked closely. This code was just taken from the previous version of this file.

lyzadanger · 2020-10-15T12:59:57Z

src/annotator/plugin/pdf-metadata.js

+
+          // `documentloaded` is the preferred event in PDF.js >= v2.0.943.
+          // See https://github.com/mozilla/pdf.js/commit/7bc4bfcc8b7f52b14107f0a551becdf01643c5c2
+          app.eventBus.on('documentloaded', finish);


These two event names make me cringe...

lyzadanger · 2020-10-15T13:00:07Z

src/annotator/plugin/pdf-metadata.js

-        window.addEventListener('documentload', finish);
-      }
+        if (app.eventBus) {
+          // PDF.js >= v1.6.210 dispatch events via an internal event bus.


Excellent commenting, thank you.

lyzadanger · 2020-10-15T13:01:49Z

src/annotator/plugin/pdf-metadata.js

+          // See https://github.com/mozilla/pdf.js/commit/7bc4bfcc8b7f52b14107f0a551becdf01643c5c2
+          app.eventBus.on('documentloaded', finish);
+
+          // `documentload` is dispatched by PDF.js < v2.1.266.


Is its meaning different from documentloaded? Or was it literally just Very Slightly Renamed (arghghgh)?

It is just renamed AFAIK. I haven't checked what the payloads of the events are, but we don't use them anyway.

lyzadanger · 2020-10-15T13:02:35Z

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

@@ -42,23 +42,42 @@ class FakePDFViewerApplication {
  /**
   * Initialize the "PDF viewer" as it would be when loading a document or
   * when a document fails to load.
+   *
+   * @param {string} url - Fake PDF URL


Thank you very much for adding these thorough comments. This could be crazymaking in the future without them.

lyzadanger · 2020-10-15T13:03:47Z

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

@@ -110,22 +129,32 @@ function delay(ms) {
 describe('annotator/plugin/pdf-metadata', function () {
  [
    {
-      // Oldest PDF.js versions (pre-2.x)
+      // PDF.js < 1.6.210: `documentload` event dispatched via DOM.


Again, we'd be at sea later on without these comments, so thank you!

lyzadanger · 2020-10-15T13:04:44Z

src/annotator/anchoring/pdf.js

@@ -75,10 +75,20 @@ async function getPageView(pageIndex) {
    // a "pdfPage" property.
    pageView = await new Promise(resolve => {
      const onPagesLoaded = () => {
-        document.removeEventListener('pagesloaded', onPagesLoaded);
+        if (pdfViewer.eventBus) {


Perhaps drop some of those snazzy comments about versions in here for later reference?

f96af43 added support for PDF.js >= v2.5.207 by changing an event listener to use PDF.js's internal event bus rather than DOM events. However there was another DOM event listener for the `pagesloaded` event in `src/annotator/anchoring/pdf.js` that should also have been updated but was overlooked. This didn't cause a problem in testing with the dev server because the test documents load quickly enough that they are already loaded by the time the client's anchoring logic ran. This commit updates the way that the client listens for events from PDF.js to use the event bus where available and only fall back to the DOM in versions of PDF.js that don't support it. - Use PDF.js's event bus to listen for `documentload`/`documentloaded` and `pagesloaded` events - Add a fallback method to wait for event bus to become available in versions of PDF.js which support the eventBus but don't have the `initializedPromise` API - Improve the documentation around which versions of PDF.js support different event types and event dispatch methods - Add tests to cover the behavior from different releases of PDF.js For an overview of the different versions of PDF.js that the client needs to support, see #2643.

robertknight · 2020-10-16T13:22:14Z

I agreed with Lyza yesterday that this was good to merge once the missing tests were added.

- Remove the `eventBusDispatchToDOM` option, as the client now watches for these events on the event bus. See [1]. - Use `PDFViewerApplication.initializedPromise` to simplify waiting for app to load. This was added in PDF.js v2.10.377. See [2]. [1] hypothesis/client#2641 [2] mozilla/pdf.js#11607

robertknight force-pushed the fix-pagesloaded-event-handler branch 2 times, most recently from d01872f to 20c5d97 Compare October 15, 2020 10:32

robertknight requested a review from lyzadanger October 15, 2020 10:41

lyzadanger approved these changes Oct 15, 2020

View reviewed changes

robertknight force-pushed the fix-pagesloaded-event-handler branch from 20c5d97 to 5da3feb Compare October 16, 2020 13:13

robertknight marked this pull request as ready for review October 16, 2020 13:21

robertknight merged commit 77d3023 into master Oct 16, 2020

robertknight deleted the fix-pagesloaded-event-handler branch October 16, 2020 13:23

robertknight mentioned this pull request Sep 17, 2021

Remove obsolete logic from pdfjs-init hypothesis/browser-extension#658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207 #2641

Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207 #2641

robertknight commented Oct 14, 2020 •

edited

Loading

codecov bot commented Oct 14, 2020 •

edited

Loading

lyzadanger left a comment

lyzadanger Oct 15, 2020

robertknight Oct 15, 2020

lyzadanger Oct 15, 2020

lyzadanger Oct 15, 2020

lyzadanger Oct 15, 2020

robertknight Oct 15, 2020

lyzadanger Oct 15, 2020

lyzadanger Oct 15, 2020

lyzadanger Oct 15, 2020

robertknight commented Oct 16, 2020

Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207 #2641

Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207 #2641

Conversation

robertknight commented Oct 14, 2020 • edited Loading

codecov bot commented Oct 14, 2020 • edited Loading

Codecov Report

lyzadanger 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Oct 16, 2020

robertknight commented Oct 14, 2020 •

edited

Loading

codecov bot commented Oct 14, 2020 •

edited

Loading