This patch does three things:
- Updates the `gulp unittestcli` command, using `gulp lint` as a guide, such that it can be run locally on Windows without any modifications.
- Updates the `gulp lib` command to support disabling of Babel through the `SKIP_BABEL` environment variable. Note that all other build targets support this mode, and there's no good reason for `lib` to be any different here.
- Updates the `npm test` command, used in Node.js/Travis, to test the code as-is test. Since modern Node.js versions seem to have no problems with ES6 compatible code in general, we should just test the source code as-is instead (similar to the tests running on the regular bots).
With the removal of the global `PDFJS` object, in PDF.js version `2.0`, the viewer options are no longer as easily accessible as they previously were (and issues have been filed about this).
In particular, since the viewer files aren't necessarily loaded *immediately*, this means that `PDFViewerApplication`/`PDFViewerApplicationOptions` aren't necessarily available directly. By dispatching an event once all viewer files are loaded but *before* the viewer initialization has run, setting `AppOptions` during load (in custom implementations of the default viewer) should hopefully become a little bit easier[1].
---
[1] In hindsight, this should probably have been implemented when the global `PDFJS` object was removed...
Rather than having a (somewhat) randomly choosen list of Preferences which `AppOptions` are allowed to override, it makes much more sense to simply add an AppOption to allow custom implementations to ignore Preferences altogether (it's also inline with the AppOption that allows the `ViewHistory` to be bypassed on load).
This should have been part of the previous commit that updated the
Wintersmith dependency. Markdown support is no longer included in Pug
itself and should be done by a transformer instead.
The intent of the code, based on existing comments, is to perform a binary search. However, because of what appears to be a typo in the code responsible for computing the current search index, this code is always checking *every* entry (albeit only at the "final" node) starting from the last one.
Update configuration to work with Babel 7
Explicitly require globals - eslint-plugin-mozilla needs it, but doesn't require it on its own.
Fix Regexp to match Babel 7's inlined _interopRequireDefault
According to the specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G6.2384179, the keys of NameTree/NumberTree should be ordered.
For corrupt PDF files, which violate this assumption, we thus need to fallback to an exhaustive search in order to e.g. find all destinations.
*Please note:* Given that this only implements a fallback for the "final" node of the Tree, there's obviously a risk that the patch isn't sufficient for dealing with all kinds of out-of-order corruption. However, this kind of problem should be rare in practice, and without a real-world test-case it's difficult to implement a completely general solution (and there's obviously a question if you'd even want to).
This changes all occurrences of `var` to `let`/`const` in this code, and updates the signature of the constructor to use object destructuring for better readability (and self documentation).
Also, `useRequestAnimationFrame` is changed to a parameter and the `typeof window` check is now done *once* rather than at every `_scheduleNext` call.
This changes all occurrences of `var` to `let`/`const` in this code, and updates the signatures of a couple of methods to use object destructuring.
Finally, when creating `InternalRenderTask` instances *only* the necessary parameter are now provided, since passing through the `RenderParameters` as-is seems completely unnecessary.
Jasmine recommends to use the `configure` method on the environment
during boot. This commit makes the code correspond to how it's done in
Jasmine's default boot file. The options dropdown in the HTML reporter
now works again after these changes, because this broke in the upgrade
to Jasmine 3, and the unit tests are executed in a random order by
default, which is important to make sure the unit tests are
self-contained and don't depend on the result of another unit test.
Rather than closing [bug 1505824](https://bugzilla.mozilla.org/show_bug.cgi?id=1505824) as WONTFIX (which is my preferred solution), given how *minor* this "problem" is, it's still possible to adjust the error messages a bit.
The main point here, which is relevant even if the changes in `BaseViewer` are ultimately rejected during review, is that we'll no longer attempt to call `BaseViewer.currentPageLabel` with an empty string from `webViewerPageNumberChanged` in `app.js`.
The other changes are:
- Stop printing an error in `BaseViewer._setCurrentPageNumber`, and have it return a boolean indicating if the page is within bounds.
- Have the `BaseViewer.{currentPageNumber, currentPageLabel}` setters print their own errors for invalid pages.
- Have the `BaseViewer.currentPageLabel` setter no longer depend, indirectly, on the `BaseViewer.currentPageNumber` setter.
- Improve a couple of other error messages.
When `highlightAll` is *not* set, there's only going to be a single match per page and unconditionally calling `PDFFindController.scrollMatchIntoView` doesn't really matter.
However, when `highlightAll` is set the current code may result in a large number of unnecessary `PDFFindController.scrollMatchIntoView` calls. Since `TextLayerBuilder._renderMatches` already checks if a particular match is the selected one, for highlighting purposes, it's simple enough to also skip scrolling completely for non-selected matches.
Only scroll search results into view as a result of an actual find operation, and not when the user scrolls/zooms/rotates the document (bug 1237076, issue 6746)
Most of the code in `PDFFindController` assumes that a valid `state` always exits, hence it cannot hurt to add a simple check to avoid errors being thrown.
Currently searching, and particularily highlighting of search results, may interfere with subsequent user-interactions such as scrolling/zooming/rotating which can result in a somewhat jarring UX where the document suddenly "jumps" to a previous position.
This is especially annoying in cases where the highlighted search result isn't even visible when a user initiated scrolling/zooming/rotating happens, and there exists a couple of bugs/issues about this behaviour.
It seems reasonable, as far as I'm concerned, to treat searching as one operation and any subsequent non-search user interactions with the viewer as separate and thus not scroll the current search result into view *unless* the user is actually doing another search.
This also seems consistent with general searching in e.g. Firefox and Adobe Reader:
- Compare with "regular" searching of e.g. HTML files in Firefox, where the user scrolling and/or zooming the document will not force a currently highlighted search result to become re-scrolled into view.
- Compare also with Adobe Reader, where the user scrolling, zooming, and/or rotating the document will not force the currently highlighted search result to become re-scrolled into view.
The question is then why search highlighting was implemented this way in PDF.js to begin with. It might be that this wasn't really intended behaviour, but more a consequence of the asynchronous nature of the API. Considering that most operations, such as fetching the page, rendering it and extracting its text-content are all asynchronous; searching and highlighting of matches thus becomes asynchronous too.
However, it should be possible to track when search results have been scrolled into view and highlighted, and thus prevent these wierd "jumps" when the user interacts with the document.
*Please note:* Unfortunately this required moving the scrolling of matches back into `PDFFindController`, since I simply couldn't see any other (reasonable) way of implementing the functionality without tracking the `_shouldScroll` property in only *one* spot.
However, given that the new `PDFFindController.scrollMatchIntoView` method follows a similar pattern as `BaseViewer.scrollPageIntoView` and `PDFThumbnailViewer.scrollThumbnailIntoView`, this is hopefully deemed OK.
