Currently we'll only attempt to start from the current page when a new search is done, however for 'findagain' operations we'll always continue from the last match position.
This could easily lead to confusing behaviour if the user has scrolled to a completely different part of the document. In an attempt to improve this somewhat, for repeated 'findagain' operations, we'll instead reset the position to the current page when it's *absolutely* certain that the user has scrolled.
Note that this required adding a new `BaseViewer` method, and exposing that through `PDFLinkService`, in order to check if a given page is visible.
In an attempt to avoid issues, in custom implementations of `PDFFindController`, the code checks for the existence of the `PDFLinkService.isPageVisible` method *before* using it.
Unfortunately the `PDFFindController.executeCommand` method has now become a bit more complicated than one would like, but hopefully this small change will improve the structure somewhat (especially for subsequent patches).
With only *one* event now being dispatched when the scale changes, in combination with there only being two call-sites, it doesn't seem necessary to keep the helper method for dispatching the 'scalechanging' event.
Currently `PDFFindController._calculateMatch` is (indirectly) dispatching an `updatetextlayermatches` event for every *single* page of the document. For short documents, such as the `tracemonkey` file, this probably doesn't matter too much, but for documents with a couple of thousand pages it seems unfortunate.
It shouldn't be necessary, in general, to dispatch `updatetextlayermatches` events here, since that's already being taken care of in `PDFFindController._updateMatch` which is always called when a match has been found.
However, when `highlightAll` is set we still need to ensure that pages which finished rendered *before* searching begun are updated correctly.
**STR:**
1. Open the default viewer, with the `tracemonkey` file.
2. Open the findbar, and search for "trace".
3. Enable the "Highlight All" option.
4. Close the findbar.
5. Re-open the findbar, and click on the "findNext" button.
6. Scroll down to the *second* page of the document.
**ER:**
Since "Highlight All" is active, all matches on the *second* page should be highlighted.
**AR:**
No matches are highlighted on the *second* page.
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`.
For a short document, such as e.g. the `tracemonkey` file, this repeated normalization won't matter much, but for documents with a couple of thousand pages it seems completely unnecessary (and wasteful) to keep repeating the normalization whenever for every single page.
Currently the text-content is normalized every time that a new search operation is started, which seems completely useless considering that the "raw" text-content is never used for anything.
For a short document, such as e.g. the `tracemonkey` file, this repeated normalization won't matter much, but for documents with a couple of thousand pages it seems completely unnecessary (and wasteful) to keep repeating the normalization whenever e.g. a new search operation starts.
In the event that multiple instances of `PDFFindController` ever exists simultaneously, they will all be able to share just one `normalize` function in this way. Furthermore, the regular expression is now created lazily rather than at class construction time.
Despite all highlighted matches being removed in response to the 'findbarclose' event, there's a risk that a match could still be scrolled into view *after* the findbar has been closed[1].
Hence we need to ensure that long running searches, particularily those happening in large and/or slow loading documents[2], are ignored as well.
---
[1] The match is hidden, as expected, but the document could still scroll unexpectedly.
[2] Large documents loaded with `disableAutoFetch = true` and `disableStream = true` set are particularily susceptible to this issue.
Given that dispatching the 'updatetextlayermatches' event with `pageIndex = -1` set is now used to target the textLayers of *all* pages, there's no need to send individual events to every single page during `_nextMatch`. Since there can be an arbitrary number of pages in a document, this small/simple optimization seems too easy to ignore.
This patch does four things:
- Change the search related methods in `TextLayerBuilder` to be "private", since there're only called from within the class itself now.
- Use `const` for local variables not intended to change in the search related methods in `TextLayerBuilder`.
- Finally, removes most `this.findController` checks since they are redundant. Note how both `this._convertMatches` and `this._renderMatches` are *only* ever called, from `this._updateMatches`, when `this.findController` is actually defined. Hence there's really no need to repeat those checks all over the place, especially with all the relevant methods now being marked as "private".
- Always initialize the `this._pageMatchesLength` property with an empty array, to simplify the code in `TextLayerBuilder`.
This should, hopefully, cover all the possible ways[1] in which "fake workers" are loaded. Given the different code-paths, adding unit-tests might not be that simple.
Note that in order to make this work, the various `fakeWorkerFilesLoader` functions were converted to return `Promises`.
---
[1] Unfortunately there's lots of them, for various build targets and configurations.
Rather than having every invocation of `Toolbar._updateUIState` compute a valid `pageScaleValue`, it seems easier to simply ensure that it happens when the value is actually updated.
Looking at the history of this code, this parameter has never been used.
I'm guessing that most likely the code in `web/toolbar.js` began life as a copy of `web/secondary_toolbar.js`, which would probably explain why that parameter exists.
*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.
Since searching itself is an asynchronous operation, removal of highlights needs to be asynchronous too since otherwise there's a risk that the events happen in the wrong order and find highlights thus remain visible.
Also, this patch will now ensure that only 'findbarclose' events for the *current* document is handled since other ones doesn't really matter. Note in particular that when no document is loaded text-layers are, obviously, not present and subsequently it's unnecessary to attempt to hide non-existent find highlights.
For many years it's been possible to enter a search term into the findbar(s) before the document has finised loading, such that searching starts immediately once it has loaded.
PR 10099 accidentally broke that, which I unfortunately missed during reviewing.
Since searching is asynchronous you cannot directly check in `executeCommand` if the document is loaded/current, but need to wait until searching is actually enabled first.
Furthermore this patch also ensures that the `_findTimeout` is always correctly cleared given that it adds further asynchronous behaviour to searching, since you obviously only want to deal with searches relevant to the current document.
This is similar to the format used by a number of other viewer components, and should simplify the `PDFSidebar` initialization slightly.
Furthermore, by using the `eventBus` it's no longer necessary for `PDFSidebar` to have a direct dependency on `PDFOutlineViewer`.
There's still room for improvement here, but this patch is at least a start (since it's not clear to me how best to handle the viewers).
This attempts to reduced the level of indirection, and the amount of code, when dispatching `fileattachmentannotation` events, by removing the `PDFLinkService.onFileAttachmentAnnotation` method and just accessing `PDFLinkService.eventBus` directly in the `FileAttachmentAnnotationElement` constructor.
Given that other properties, such as `externalLinkTarget`/`externalLinkRel`, are already being accessed directly this pattern seems fine here as well.
This commit shows that we can now unit test the find controller and
that executing regular queries works. Note that this is only a first
step and not a complete suite of unit tests for all possible options
of the find controller.
While writing this unit test, I found two smaller issues that I
addressed directly. The first one is that in the previous find
controller refactoring I forgot to rename some occurrences of a now
private member variable. Fortunately this did not cause any bugs since
we did have a public getter and the fetched value may be changed by
reference, but it's nevertheless good to fix. The second issue is that
some entries in the `test/unit/clitests.json` file were not correct,
resulting in these tests not being executed on e.g., Travis CI.
With `PDFFindController` instances no longer (directly) depending on
`BaseViewer` instances, we can pass a single `findController` when
initializing a viewer, similar to other components.
This removes the dependency on a `PDFViewer` instance from the find
controller, which makes it more similar to other components and makes it
easier to unit test with a mock link service.
Finally, we remove the search capabilities from the SVG example since it
doesn't work there because there is no separate text layer.
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.
This way the resetting of `PDFLinkService`/`PDFDocumentProperties` instances, as is done in `PDFViewerApplication.close`, only requires passing in *one* `null` argument instead of two.
The built-in PDF Viewer (in Firefox) cannot use the browser findbar when PDF files are embedded in e.g. iframe/object tags, and the PDF.js findbar (i.e. `PDFFindBar`) will thus be used instead in those cases.
This is slightly problematic, since the `MOZCENTRAL` version of the viewer uses a special, slimmed down, version of the `l10n.js` file that doesn't (currently) support plural forms. To prevent the matchesCounter from breaking completely in this edge-case, temporarily hard-code the plural form to use the default `[other]` version of the locale strings.
This prevents the findbar from intermittently displaying `0 of {number} matches`, which *could* theoretically happen for large and/or slow loading documents.
