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 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.
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 prevents the findbar from intermittently displaying `0 of {number} matches`, which *could* theoretically happen for large and/or slow loading documents.
The find controller should only coordinate finding a string in the
document and should not be responsible for presenting the matches to the
user. The text layer builder already contains the logic to render the
matches in the viewer, so it should also take care of scrolling the
selected match into view.
The find controller already has quite a lot of state to maintain. We can
avoid keeping track of this member variable because when the find
controller is reset, so is the extract text promises array. Therefore,
we can just check if that array contains items or not to determine if
text extraction already started.
Moreover, there is no need to reset the `pageContents` array since the
`reset` method already takes care of that.
This is an unfortunate oversight on my part, which I stumbled upon when (locally) testing the `mozilla-central` follow-up patch necessary to enable the matches counter in the built-in PDF viewer.
As outlined in https://bugzilla.mozilla.org/show_bug.cgi?id=1282759 the internal Firefox name for the feature is `entireWord`, hence that name is used here as well for consistency (with "Whole words" being limited to the UI).
Given existing limitations of the PDF.js search functionality, e.g. the existing problems of searching across "new lines", there's some edge-cases where "Whole words" searching will ignore (valid) results.
However, considering that this is a pre-existing issue related to the way that the find controller joins text-content together, that shouldn't have to block this new feature in my opionion.
*Please note:* In order to enable this feature in the `MOZCENTRAL` version, a small follow-up patch for [PdfjsChromeUtils.jsm](https://hg.mozilla.org/mozilla-central/file/tip/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm) will be required once this has landed in `mozilla-central`.
For the `PDFFindBar` implementation, similar to the native Firefox findbar, the matches count displayed is now limited to a (hopefully) reasonable value.
*Please note:* In order to enable this feature in the `MOZCENTRAL` version, a follow-up patch will be required once this has landed in `mozilla-central`.
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.
Moreover, rename `FindStates` to `FindState` since enumeration names are
usually not in plural, for readability and consistency with the ones in
`src/shared/util.js`.
Currently this method first uses a loop to build a temporary array to hold Promises, which are then resolved from a recursive helper function once the textContent is fetched for each page.
To me, this is unncessarily complicated, since we can do everything within one loop by simply chaining the asynchronous calls to retrieve the textContent. (Note that this guarantees that the textContent of the pages is still fetched sequentially.)
In various viewer files, there's a number of cases where we basically duplicate the functionality of `createPromiseCapability` manually.
As far as I can tell, a couple of these cases have existed for a very long time, and notable even before the `createPromiseCapability` utility function existed.
Also, since we can write ES6 code now, the patch also replaces a couple of `bind` usages with arrow functions in code that's touched in the patch.
Note that as discussed on IRC, this makes the viewer slightly slower to load *only* in `gulp server` mode, however the difference seem slight enough that I think it will be fine.
Using `else` after `return` is not necessary, and can often lead to unnecessarily cluttered code. By using the `no-else-return` rule in ESLint we can avoid this pattern, see http://eslint.org/docs/rules/no-else-return.
There are PDF generators which create destinations with e.g. too large top values, which cause the wrong page to be scrolled into view because the offset becomes negative.
By ignoring negative offsets, we can prevent this issue, and get a similar behaviour as in Adobe Reader.
However, since we're also using `PDFViewer_scrollPageIntoView` in more cases than just when links (in the document/outline) are clicked, the patch adds a way to allow the caller to opt-out of this behaviour.
In e.g. the following situations, I think that we still want to be able to allow negative offsets: when restoring a position from the `ViewHistory`, when the `viewBookmark` button is used to obtain a link to the current position, or when maintaining the current position on zooming.
Rather than adding another parameter to `PDFViewer_scrollPageIntoView`, I've changed the signature to take an parameter object instead. To maintain backwards compatibility, I've added fallback code enclosed in a `GENERIC` preprocessor tag.
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=874482.
These have been found using `gulp lint` in combination with the `unused:
true` parameter for JSHint. Unfortunately there are too many false
positives to enable this feature, but now that most globals have been
removed because of the conversion to UMD the results are much more
useful than before.
To reduce code duplication, the initialization code now uses the `reset` method.
Also, this patch moves `charactersToNormalize` out of `PDFFindController`, since it seemed better suited to be a "constant".
This patch goes a bit further than issue 6612 requires, and replaces all kinds of whitespace with standard spaces.
When testing this locally, it actually seemed to slightly improve two existing test-cases (`tracemonkey-text` and `taro-text`).
Fixes 6612.
*This regressed in PR 5356.*
Rather than just backing out the offending code, this patch restores scrolling in PresentationMode by making the `overflow: hidden;` check optional and letting the callers that need it (e.g. `PDFFindController`) opt-in to use it.
