This is relevant for e.g. `PDFSinglePageViewer`, and `PDFViewer` with Presentation Mode active.
By moving this code to a helper method in `BaseViewer`, it's thus possible to reduce the amount of duplicate code that currently needed in `PDFViewer` and `PDFSinglePageViewer`.
*This patch is based on something that I noticed while working on PR 10126.*
The recent re-factoring of `PDFFindController` brought many improvements, among those the fact that access to `BaseViewer` is no longer required. However, with these changes there's one thing which now strikes me as not particularly user-friendly[1]: The fact that in order for searching to actually work, `PDFFindController.setDocument` must be called *and* a 'pagesinit' event must be dispatched (from somewhere).
For all other viewer components, calling the `setDocument` method[2] is enough in order for the component to actually be usable.
The `PDFFindController` thus stands out quite a bit, and it also becomes difficult to work with in any sort of custom implementation. For example: Imagine someone trying to use `PDFFindController` separately from the viewer[3], which *should* now be relatively simple given the re-factoring, and thus having to (somehow) figure out that they'll also need to manually dispatch a 'pagesinit' event for searching to work.
Note that the above even affects the unit-tests, where an out-of-place 'pagesinit' event is being used.
To attempt to address these problems, I'm thus suggesting that *only* `setDocument` should be used to indicate that searching may start. For the default viewer and/or the viewer components, `BaseViewer.setDocument` will now call `PDFFindController.setDocument` when the document is ready, thus requiring no outside configuration anymore[4]. For custom implementation, and the unit-tests, it's now as simple as just calling `PDFFindController.setDocument` to allow searching to start.
---
[1] I should have caught this during review of PR 10099, but unfortunately it's sometimes not until you actually work with the code in question that things like these become clear.
[2] Assuming, obviously, that the viewer component in question actually implements such a method :-)
[3] There's even a very recent issue, filed by someone trying to do just that.
[4] Short of providing a `PDFFindController` instance when creating a `BaseViewer` instance, of course.
With `PDFFindController` instances no longer (directly) depending on
`BaseViewer` instances, we can pass a single `findController` when
initializing a viewer, similar to other components.
Now it follows the same pattern as e.g., the document properties
component, which allows us to have one instance of the find controller
and set a new document to search upon switching documents.
Moreover, this allows us to get rid of the dependency on `pdfViewer` in
order to fetch the text content for a page. This is working towards
getting rid of the `pdfViewer` dependency upon initializing the
component entirely in future commits.
Finally, we make the `reset` method private since it's not supposed to
be used from the outside anymore now that `setDocument` takes care of
this, similar to other components.
If the current viewer is a `PDFSinglePageViewer` instance the Scroll/Spread modes are no-ops, hence displaying buttons that do *nothing* when clicked will probably do very little besides confuse users.
Rather than having to manually call a method on `PDFFindController` instances from `BaseViewer.setDocument`, thus essentially having to resolve the private `_firstPagePromise` from the "outside", this can be done easily with the 'pagesinit' event dispatched on the `eventBus` instead.
Please note this particular `PDFFindController` code pre-dates the `eventBus` by almost three years, which should explain why the code looks the way it does.
Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.
---
[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to https://github.com/mozilla/pdf.js/pull/8539#issuecomment-309706629 for additional details.
Since all the other viewer methods use the getter/setter pattern, e.g. for setting page/scale/rotation, the way that the Scroll/Spread modes are set thus stands out. For consistency, this really ought to use the same pattern as the rest of the `BaseViewer`. (To avoid breaking third-party implementations, the old methods are kept around as aliases.)
Note how in `BaseViewer.forceRendering` the Scroll mode is used to determine how pre-rendering will work. Currently this is broken in Presentation Mode, if horizontal scrolling was enabled prior to entering fullscreen.
Furthermore, there's a few additional cases where the `this.scrollMode === ScrollMode.HORIZONTAL` check is pointless either in Presentation Mode or when a `PDFSinglePageViewer` instance is used.
Since the Scroll/Spread modes doesn't make (any) sense in `PDFSinglePageViewer` instances, the general structure of these methods can be improved to reflect that.
Given that this method is a no-op in `PDFSinglePageViewer`, similar to `_regroupSpreads`, let's improve the general code structure by simply moving the method.
The Secondary Toolbar buttons for, not to mention the actual toggling of, Scroll/Spread modes are currently completely broken in older browsers (such as IE11).
As a follow-up, it'd probably be a good idea to try and find a *feature complete* `classList` polyfill that could be used instead, but this patch at least addresses the immediate regression.
Please refer to the compatibility information in https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_compatibility
This builds on the scrolling mode work to add three buttons for joining
page spreads together: one for the default view, with no page spreads,
and two for spreads starting on odd-numbered or even-numbered pages.
A couple of basic unit-tests are added, and a manual `isLandscape` check (in `web/base_viewer.js`) is also converted to use the helper function instead.
One additional complication with removing this option from the global `PDFJS` object, is that the viewer currently needs to check `disableAutoFetch` in a couple of places. To address this I'm thus proposing adding a getter in `PDFDocumentProxy`, to allow checking the *actually* used values for a particular `getDocument` invocation.
Rather than having two different (but connected) options for the textLayer, I think that it makes sense to try and unify this. For example: currently if `disableTextLayer === true`, then the value of `enhanceTextSelection` is simply ignored.
Since PDF.js version `2.0` already won't be backwards compatible in lots of ways, I don't think that we need to worry about migrating existing preferences here.
This removes the `PDFJS.imageResourcesPath` dependency from the viewer components and the test-suite, but please note that as a *temporary* solution the default viewer still uses it.
This removes the `PDFJS.maxCanvasPixels` dependency from the viewer components, but please note that as a *temporary* solution the default viewer still uses it.
This removes the `PDFJS.useOnlyCssZoom` dependency from the viewer components, but please note that as a *temporary* solution the default viewer still uses it.
The only reason for adding this parameter in the first place, all the way back in PR 4074, was that the "maintain document position on zooming" feature was landed and backed out a couple of times before it finally stuck.
Hence it seemed, at the time, like a good idea to have a simple way to disable that behaviour. However, that was almost four years ago, and it's just not likely that we'd want/need to ever disable it now.
Furthermore I really cannot imagine why anyone would actually *want* to reset the position whenever zooming occurs, since it results in a quite annoying UX.
*So, to summarize:* Based on the above, I think that we should try to remove this parameter now. On the off chance that anyone complains, re-adding it shouldn't be difficult.
This patch introduces an abstract `BaseViewer` class, that the existing `PDFViewer` then extends. *Please note:* This lays the necessary foundation for the next patch.
