Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify PDF.js version support policy #2643

Open
robertknight opened this issue Oct 15, 2020 · 0 comments
Open

Clarify PDF.js version support policy #2643

robertknight opened this issue Oct 15, 2020 · 0 comments

Comments

@robertknight
Copy link
Member

robertknight commented Oct 15, 2020 •
edited
Loading

The client internally has code paths to support different versions of PDF.js. We don't have a documented policy about which versions are supported nor do we know which versions of PDF.js are actually still in use with Hypothesis.

Versions of PDF.js we need to support:

  1. The versions that ship with the browser extension and Via. We can update these ourselves.
  2. The version of PDF.js that ships with the current version of Firefox. Mozilla updates this often - I'm not sure quite how often.
  3. Versions of PDF.js that are used by publishers embedding PDFs with Hypothesis on their websites. These will often be copies of https://github.com/hypothesis/pdf.js-hypothes.is.

(1) is fairly easy to handle as we can control when the extension/Via ship a new version of PDF.js and make sure it is supported. (2) has occasionally caught us out due to breaking API changes in a new PDF.js release appearing in Firefox's internal viewer first. (3) might be the reason that we have to keep support for older PDF.js versions around longer than we'd like. It was unfortunately based on PDF.js v1.1.114 from June 2015 until November 2019. That version is probably embedded on various websites which are not often updated, though we don't yet have metrics on this.

On the basis of the above, I propose that the oldest version of PDF.js that we support is v1.1.114. As long as it isn't especially painful to do so, and as long as there are still users of it, we should keep supporting that version for a while.

This task is done when it has been discussed internally and we have documented somewhere obvious in the code (and possibly user-facing documentation) what the supported versions of PDF.js are and how to test them.
@robertknight robertknight mentioned this issue Oct 15, 2020
Open
robertknight added a commit that referenced this issue Oct 15, 2020
@robertknight
Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207
d01872f 
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 robertknight mentioned this issue Oct 15, 2020
Merged
2 tasks
robertknight added a commit that referenced this issue Oct 15, 2020
@robertknight
Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207
20c5d97 
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 added a commit that referenced this issue Oct 16, 2020
@robertknight
Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207
5da3feb 
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 added a commit that referenced this issue Oct 16, 2020
@robertknight
Make logic for waiting for PDFs to load work in PDF.js >= 2.5.207
77d3023 
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 robertknight mentioned this issue May 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@robertknight @leedenison