These callbacks should not be necessary *before* the document has been initialized. Furthermore, move the functionality to a new helper-method since `PDFViewerApplication.load` is already quite large.
Given that this relies on accessing properties on the `PDFDocumentProxy`-instance, it seems more appropriate for this code to live in `PDFViewerApplication`.
It seems that the timeout is way too short in practice, since this new integration-test failed *intermittently* already in PR 12702 (which is where the test was added).
The ideal solution here would be to simply await an event, dispatched by the viewer, however that unfortunately doesn't appear to be supported by Puppeteer.
Instead, the solution implemented here is to add a new method in `PDFViewerApplication` which Puppeteer can query to check if the scripting/sandbox has been fully initialized.
There's really no point, as far as I can tell, to attempt to dispatch an event in a non-existent sandbox. Generally speaking, even trying to do this *could* possibly even lead to errors in some cases.
Furthermore, utilize optional chaining to simplify some `dispatchEventInSandbox` calls throughout the viewer.
Finally, replace superfluous `return` statements with `break` in the switch-statement in the `updateFromSandbox` event-handler.
There's no really compelling reason, as far as I can tell, to introduce the `ENABLE_SCRIPTING` build-target, instead of simply re-using the existing `TESTING` build-target for the new `gulp integrationtest` task.
In general there should be no problem with just always enable scripting in TESTING-builds, and if I were to *guess* the reason that this didn't seem to work was most likely because the Preferences ended up over-writing the `AppOptions`.
As it turns out the GENERIC-viewer has already has built-in support for disabling of Preferences, via the `AppOptions`, and this can be utilized in TESTING-builds as well to ensure that whatever `AppOptions` are set they're always respected.
For DOM events all event names are lower-case, and the newly added PDF.js scripting-events thus "stick out" quite a bit. Even more so, considering that our internal `eventBus`-events follow the same naming convention.
Hence this patch, which changes the "updateFromSandbox"/"dispatchEventInSandbox" events to be lower-case instead.
Furthermore, using DOM events for communication *within* the PDF.js code itself (i.e. between code in `web/app.js` and `src/display/annotation_layer.js/`) feels *really* out of place.
That's exactly the reason that we have the `EventBus` abstraction, since it allowed us to remove prior use of DOM events, and this patch thus re-factors the code to make use of the `EventBus` instead for scripting-related events.
Obviously for events targeting a *specific element* using DOM events is still fine, but the "updatefromsandbox"/"dispatcheventinsandbox" ones should be using the `EventBus` internally.
*Drive-by change:* Use the `BaseViewer.currentScaleValue` setter unconditionally in `PDFViewerApplication._initializeJavaScript`, since it accepts either a string or a number.
- Actually remove the `isDown` property when destroying the scripting-instance.
- Mark all `mouseState` usage as "private" in the various classes.
- Ensure that the `AnnotationLayer` actually treats the parameter as properly *optional*, the same way that the viewer components do.
- For now remove the `mouseState` parameter from the `PDFPageView` class, and keep it only on the `BaseViewer`, since it's questionable if all of the scripting-functionality will work all that well without e.g. a full `BaseViewer`.
- Append the `mouseState` to the JSDoc for the `AnnotationElement` class, and just move its definition into the base-`AnnotationElement` class.
* the goal is to execute actions like Open or OpenAction
* can be tested with issue6106.pdf (auto-print)
* once #12701 is merged, we can add page actions
This new event essentially mirrors the existing "pagesinit" event, and will allow e.g. a custom implementation of the viewer to be notified before the current PDF document is removed from the viewer.
By using this new event, we're thus able to dispatch a "pageclose" event for JavaScript actions when closing the existing document.
Having looked at the Acrobat JavaScript specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/AcrobatDC_js_api_reference.pdf#G5.1963437, I suppose that introducing these two new events is probably the easiest solution overall.
However there's a number of things that, as far as I'm concerned, will help the overall implementation:
- Only dispatch these new events when `enableScripting = true` is set.
- Handle them *separately* from the existing "pagechanging" event dispatching, to avoid too much clutter.
- Don't dispatch either of the events if the page didn't actually change.
- When waiting for pages to render, don't dispatch "pageopen" if the page is no longer active when rendering finishes.
- Ensure that we only use *one* "pagerendered" event listener.
- Ensure that "pageopen" is actually dispatched when the document loads.
I suppose that we *could* avoid adding the "pageclose" event, and use the existing "pagechanging" event instead, however having a separate event might allow more flexibility in the future. (E.g. I don't know if we'll possibly want to dispatch "pageclose" on document close, as mentioned briefly in the specification.)
* move set/clear|Timeout/Interval and crackURL code in pdf.js
* remove the "backdoor" in the proxy (used to dispatch event) and so return the dispatch function in the initializer
* remove listeners if an error occured during sandbox initialization
* add support for alert and prompt in the sandbox
* add a function to eval in the global scope
Given that the GENERIC default viewer supports opening more than one document, and that a unique scripting-instance is now used for each document, the changes made in this patch seem appropriate.
While it's not entirely clear to me that it's ultimately desirable to use the `pdf.sandbox.js` in the Chromium-extension, given that the MOZCENTRAL-build uses `pdf.scripting.js` directly in a *custom* sandbox, the current state isn't that great since setting `enableScripting = true` with the Chromium-extension will currently fail completely.
Hence this patch, which should at least unbreak things for now.
Since the `close` method has become quite large, this small re-factoring shouldn't hurt (and may also be useful with future changes to the `_initializeJavaScript` method).
I completely missed this previously, but we obviously should remove the scriptElement as well to *really* clean-up everything properly.
Given that there's multiple existing usages of `loadScript` in the code-base, the safest/quickest solution seemed to be to have call-sites opt-in to remove the scriptElement using a new parameter.
This patch *attempts* to actually implement what's described for the `Count`-entry in the PDF specification, see https://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G11.2095911, which I mostly ignored back in PR 10890 since it seemed unnecessarily complicated[1].
Besides issue 12704, I've also tested a couple of other documents (e.g. the PDF specification) and these changes don't *seem* to break anything else; additional testing would be helpful though!
---
[1] At the time, all PDF documents that I tested worked even with a very simple approach and I thus hoped that it'd would suffice.
Similar to the previous patch, the GENERIC default viewer is capable of opening more than *one* PDF document and we should ensure that we handle that case correctly.
I was actually quite surprised to find that, despite the various `scripting`-getters implementing `destroySandbox` methods, there were no attempts at actually cleaning-up either the "sandbox" or removing the globally registered event listeners.
This patch also changes the method to skip *all* data fetching when "enableScripting" isn't active. Finally, simplifies some event-data accesses in the "updateFromSandbox" listener.
Another possible option here could be to use the `contentLength`, when it exists, and then using e.g. a custom event to always update the "filesize" in the sandbox "after the fact" with the result of the `getDownloadInfo`-call.
We can easily avoid unnecessary API-calls here, since most of the time the `metadata` will already be available here. In the *rare* case that it's not available, we can simply wait for the existing `getMetadata`-call to resolve.
This will be useful in the following patch, and note that there's also an old issue (see 5765) which asked for such an event. However, given that the use-case wasn't *clearly* specified, and that we didn't have an internal use for it at the time it wasn't implemented.
Also, ensure that all of the metadata-related properties are actually reset when the document is closed.
Compared to the, previously removed, `sandbox`/`watch-sandbox` gulp-tasks, these ones should work even when run against an non-existent/empty `build`-folder.
Also, to ensure that the development viewer actually works out-of-the-box, `gulp server` will now also include `gulp watch-dev-sandbox` to remove the need to *manually* invoke the build-tasks.
Finally, this patch also removes the `web/devcom.js` file since it shouldn't actually be needed, assuming that the "sandbox"-loading code in the `web/genericcom.js` file is actually *correctly* implemented.
Rather than having two slightly different ways of setting the pending/notFound appearance on the "findInput", we can simply use "data-status" in both cases since they're obviously mutually exclusive.
As mentioned in https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support, PDF.js version `2.6.347` is the last release with IE 11/Edge support.
Hence we should now be able to reduce unnecessary duplication in the default viewer image resources, note the files in the `web/images/` folder with a `-dark` suffix, by using only *one* SVG-image for each icon and letting the `background-color` depend on the CSS theme instead.
For the `gulp mozcentral` build-target, the resulting `web/images/` folder is reduced from `43 997` to `28 566` bytes (~35 percent).
*Please note:* I don't really know if this implementation is necessarily the *best* solution, but it seems to work well enough in e.g. Firefox Nightly and Google Chrome Beta as far as my testing goes.
Given that we already include the "Content-Disposition"-header filename, when it exists, it shouldn't hurt to also include the information from the "Content-Length"-header.
For PDF documents opened via a URL, which should be a very common way for the PDF.js library to be used, this will[1] thus provide a way of getting the PDF filesize without having to wait for the `getDownloadInfo`-promise to resolve[2].
With these API improvements, we can also simplify the filesize handling in the `PDFDocumentProperties` class.
---
[1] Assuming that the server is correctly configured, of course.
[2] Since that's not *guaranteed* to happen in general, with e.g. `disableAutoFetch = true` set.
* quickjs-eval.js has been generated using https://github.com/mozilla/pdf.js.quickjs/
* lazy load of sandbox code
* Rewrite tests to use the sandbox
* Add a task `watch-sandbox` which update bundle pdf.sandbox.js on change in the sandbox code
Given that it's generally faster to call *one* function and have it loop through an object, rather than looping through an object and calling a function for every iteration, this patch will reduce the total time spent in `PDFViewerApplication._readPreferences` ever so slightly.
Also, over time we've been adding more and more preferences, rather than removing them, so using the new `AppOptions.setAll` method should be generally beneficial as well.
While the effect of these changes is quite small, it does reduces the time it takes for the preferences to be fully initialized. Given the amount of asynchronous code during viewer initialization, every bit of time that we can save should thus help.
Especially considering the recently added `viewerCssTheme` preference, which needs to be read very early to reduce the risk of the viewer UI "flashing" visibly as the theme changes, I figured that a couple of small patches reducing the time spend reading preferences cannot hurt.
Given that only two debugging hash parameters (i.e. `disableWorker` and `pdfBug`) will make this method asynchronous, we can avoid what's most of the time is an unnecessary `Promise.all` invocation.
While this does work pretty well in my quick testing, it's *very much* a hack since as far as I can tell there's no support in the CSS specification for using e.g. a CSS variable to override a `@media (prefers-color-scheme: dark) {...}` block.
The solution implemented here is thus to *edit* the viewer CSS, by either removing the entire `@media ...` block in light-mode or by ensuring that its rules become *unconditionally* applied in dark-mode.
To simplify the overall implementation, since all of this does seem like somewhat of an edge-case, the `viewerCssTheme` preference will *only* be read during viewer initialization. (Similar to many other existing preferences, a reload is thus required when changing it.)
Originally the default preferences were defined in a JSON-file checked into the repository, which was loaded using SystemJS in development mode.
Over the years a number of changes have been made to this code, most notably:
- The preferences JSON-file is now generated automatically, during building, from the `AppOptions` abstraction.
- All SystemJS usage has been removed from the development viewer.
Hence the default preferences are now available *synchronously* even in the development viewer, and it's thus no longer necessary to defer to the microtask queue (since `getDefaultPreferences` is async) just to get the default preferences.
While the effect of these changes is quite small, it *does* reduces the time it takes for the preferences to be fully initialized. Given the amount of asynchronous code during viewer initialization, every bit of time that we can save should thus help.
- Add support for logical assignment operators, i.e. `&&=`, `||=`, and `??=`, with a Babel-plugin. Given that these required incrementing the ECMAScript version in the ESLint and Acorn configurations, and that platform/browser support is still fairly limited, always transpiling them seems appropriate for now.
- Cache the `hasJSActions` promise in the API, similar to the existing `getAnnotations` caching. With this implemented, the lookup should now be cheap enough that it can be called unconditionally in the viewer.
- Slightly improve cleanup of resources when destroying the `WorkerTransport`.
- Remove the `annotationStorage`-property from the `PDFPageView` constructor, since it's not necessary and also brings it more inline with the `BaseViewer`.
- Update the `BaseViewer.createAnnotationLayerBuilder` method to actaually agree with the `IPDFAnnotationLayerFactory` interface.[1]
- Slightly tweak a couple of JSDoc comments.
---
[1] We probably ought to re-factor both the `IPDFTextLayerFactory` and `IPDFAnnotationLayerFactory` interfaces to take parameter objects instead, since especially the `IPDFAnnotationLayerFactory` one is becoming quite unwieldy. Given that that would likely be a breaking change for any custom viewer-components implementation, this probably requires careful deprecation.
*Note that I wasn't able to reproduce the issue in Firefox, but only in Chromium-browsers.*
The bug, and it's feels almost trivial once you've found it, is that we're not passing the `transform` parameter as intended to `PDFPageProxy.render` when drawing thumbnails on HiDPI displays. Instead the canvas context is, for reasons that I don't even pretent to understand, *manually* scaled in `PDFThumbnailView._getPageDrawContext`, which thus doesn't guarantee that the `baseTransform` property on the `CanvasGraphics`-instances becomes correct.
The solution is really simple though, just handle the `transform` the same way in `PDFThumbnailView.draw` as in `PDFPageView.paintOnCanvas` and things should just work.
*This is a pre-existing issue that I noticed while working on PR 12613, and fixing this also brings the thumbnail code inline with the page code.*
Given the intermittent nature of all of this, it's somewhat difficult to reproduce it consistently; however the following steps should at least provide an outline:
1. Open the sidebar, and the thumbnailView, and start scrolling around.
2. *Quickly* close the sidebar, so that all thumbnails won't have time to finish rendering.
3. Either wait for the cleanup-timeout to occur, or simply run `PDFViewerApplication.cleanup()` in the console.
What *intermittently* happens here is that `WorkerTransport.startCleanup` rejects, and consequently that cleanup doesn't complete as intended, since some of the thumbnails are left in a *pending* renderingState[1].
Fixing this is simple though, and only requires updating `PDFThumbnailViewer.cleanup` along the lines of `BaseViewer.cleanup`.
---
[1] Keep in mind that thumbnails will *only* render when the thumbnailView is visible, to reduce resource usage.
This patch will help reduce memory usage, especially for longer documents, when the user scrolls around in the thumbnailView (in the sidebar).
Note how the `PDFPageProxy.cleanup` method will, assuming it's safe to do so, release main-thread resources associated with the page. These include things such as e.g. image data (which can be arbitrarily large), and also the operatorList (which can also be quite large).
Hence when pages are evicted from the `PDFPageViewBuffer`, on the `BaseViewer`-instance, the `PDFPageView.destroy` method is invoked which will (among other things) call `PDFPageProxy.cleanup` in the API.
However, looking at the `PDFThumbnailViewer`/`PDFThumbnailView` classes you'll notice that there's no attempt to ever call `PDFPageProxy.cleanup`, which implies that in certain circumstances we'll essentially keep all resources allocated permanently on the `PDFPageProxy`-instances in the API.
In particular, this happens when the users opens the sidebar and starts scrolling around in the thumbnails. Generally speaking you obviously need to keep all thumbnail *images* around, since otherwise the thumbnailView is useless, but there's still room for improvement here.
Please note that the case where a *rendered page* is used to create the thumbnail is (obviously) completely unaffected by the issues described above, and this rather only applies to thumbnails being explicitly rendered by the `PDFThumbnailView.draw` method.
For the latter case, we can fix these issues simply by calling `PDFPageProxy.cleanup` once rendering has finished. To prevent *accidentally* pulling the rug out from under `PDFPageViewBuffer` in the viewer, which expects data to be available, this required adding a couple of new methods[1] to enable checking that it's indeed safe to call `PDFPageProxy.cleanup` from the `PDFThumbnailView.draw` method.
It's really quite fascinating that no one has noticed this issue before, since it's been around since basically "forever".
---
[1] While it should be *very* rare for `PDFThumbnailView.draw` to be called for a pageView that's also in the `PDFPageViewBuffer`, given that pages are rendered before thumbnails and that the *rendered page* is used to create the thumbnail, it can still happen since rendering is asynchronous.
Furthermore, it's also possible for `PDFThumbnailView.setImage` to be disabled, in which case checking the `PDFPageViewBuffer` for active pageViews *really* matters.
* When no actions then set it to null instead of empty object
* Even if a field has no actions, it needs to listen to events from the sandbox in order to be updated if an action changes something in it.
Note that a number of these cases are covered by existing unit-tests, and a few others only matter for the development/build scripts.
Furthermore, I've also tried to the best of my ability to test each case *manually* to hopefully further reduce the likelihood of this patch introducing any bugs.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-useless-escape
Given the number of parameters, and the fact that many of them are booleans, the call-sites are no longer particularly easy to read and understand. Furthermore, this slightly improves the formatting of the JSDoc-comment, since it needed updating as part of these changes anyway.
Finally, this removes an unnecessary `numViews === 0` check from `getVisibleElements`, since that should be *very* rare and more importantly that the `binarySearchFirstItem` function already has a fast-path for that particular case.
This patch addresses a review comment, which pointed out that we should *also* handle the pageNumber-input, from PR 12493.
Given that a user *manually* changing pages using the pageNumber-input, on the toolbar, could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this case as well probably won't hurt.
All of these methods will, in one way or another, cause e.g. scrolling or zooming to occur and consequently they don't really make sense unless there's an active PDF document. Especially since all of these methods end up calling into a `BaseViewer`-instance, which already contains similar early returns in essentially all of it's methods and setters.
This fixes only those warnings, as reported by https://lgtm.com/projects/g/mozilla/pdf.js?mode=list, that make sense (as far as I'm concerned).
Hence this patch leaves the following things unaddressed:
- The "recommendation"-category, since it only complains about unused variables. However, note that all of those cases are purposely included and that there's thus ESLint-disable comments added to explictly allow them.
- The "warning"-category, which still contains two complaints. However, as far as I can tell, they are both false positives.
Given first of all the false positives of the LGTM static analyzer, and secondly that we'd need to add (essentially duplicated) disable-comments for the unused variable cases, it's not entirely clear to me if we actually want to work towards including LGTM in the PDF.js project (e.g. running alongside Travis) or if we should just close issue 11965.
Given that we're now accessing certain API-functionality *directly* in this file, e.g. the AnnotationStorage and Optional Content configuration, ensuring that there's not a version mismatch definitely seem like a good idea to prevent any *subtle* future bugs.
Ensure that these tooltip-only Annotations are handled as "internalLink"s, to ensure that they behave as expected in PresentationMode (e.g. they should still use a `pointer`-cursor).
Ensure that `PDFLinkService.getDestinationHash` won't create links with empty hashes, since those don't really make a lot of sense in general (this improves things for tooltip-only Annotations).
This PDF file can be used for testing: http://mirrors.ctan.org/macros/latex/contrib/pdfcomment/doc/pdfcomment.pdf#page=14
- Return early in `PDFViewerApplication._initializeJavaScript` for PDF documents without any `fieldObjects`, which is the vast majority of all documents, to prevent errors when trying to parse a non-existent object.
- Similar to the other `PDFViewerApplication._initialize*` methods, ignore the `fieldObjects` if the document was closed before the data resolved.
- Fix the JSDoc comment for the `generateRandomStringForSandbox` helper function, since there's currently a bit too much copy-and-paste going on :-)
- Change `FirefoxScripting` to a class with static methods, which is consistent with the surrounding code in `web/firefoxcom.js`.
There's no compelling reason to update this property *manually* in multiple places, since that's error-prone with any future code changes, given that `_updateInternalState` is always called just before anyway.
While the referenced issue could very well be seen as an edge-case, this patch adds support for updating of the browser history when interacting with the thumbnails in the sidebar (assuming we want to do this).
The main reason for adding the history implementation in the first place, was to simplify navigating back to a previous position in the document when named/explicit destinations are used (e.g. when clicking on "links" or when using the outline in the sidebar).
As such, it never really crossed by mind to update the browser history when the thumbnails are used. However, a user clicking on thumbnails could be regarded as a pretty strong indication of user-intent w.r.t. navigation in the document, hence I suppose that updating the browser history in this particular case probably won't hurt.
This modernizes and improves the code, by using `async`/`await` and by extracting the helper function to its own method.
To hopefully avoid confusion, given the next patch, the method is also re-named to `goToDestination` to make is slightly clearer what it actually does.
Given that we're no longer using SystemJS to load the `web/` files, see PR 11919, there's nothing that prevents us from using standard `ìmport` statements in this file.
Obviously it's still necessary to load part of the code conditionally on the build type, however this still allows us to clean-up and simplify at least some of this file.
The `debugger`-statement would only, potentially, make sense during development and we thus want to prevent it from being accidentally included when landing code.
The `alert`, `confirm`, and `prompt` functions should generally be avoided, with the few intended cases manually allowed.
Please find additional details about the ESLint rules at:
- https://eslint.org/docs/rules/no-debugger
- https://eslint.org/docs/rules/no-alert
In the rest of the viewer code-base, we purposely don't treat `RenderingCancelledException`s as actual errors (since they aren't) and consequently we never log them.
Hence it makes sense, as far as I'm concerned, to simply treat `RenderingCancelledException`s the same way when printing in Firefox.
While I don't print a whole lot, I cannot remember seeing these "errors" logged when printing until *very* recently[1]. Given that the browser print functionality and UI, in Firefox, is under active development it's certainly possible that there's some recent changes to the related timings which make `RenderingCancelledException`s more likely now.
---
[1] Interestingly, only some PDF documents seem to be affected as well; I'm able to reproduce this pretty consistently by opening https://www.uni-muenster.de/imperia/md/content/ziv/pdf/printpay_flyer.pdf in Firefox and then repeating the following sequence:
Clicking on the PDF.js print button, and then cancelling printing.
This should be helpful to easily determine the *exact* version of the viewer itself, when looking at a *built* `web/viewer.js` file.
Note that we're already including this information in other built files, such as e.g. `pdf.js`, `pdf.worker.js`, `pdf_viewer.js`, and `pdf.image_decoders.js`.
This adds a new `PDFViewerApplication.triggerPrinting` method, which takes care of checking that printing is actually supported before calling `window.print`, to remove the need to duplicate that code in multiple places.
Also, removes the `PDFViewerApplication.printing` getter since it's not really necessary any more.
For now we need to use a Babel-plugin, since part of our build system doesn't support this fully (e.g. Babel-loader, Webpack 4.x, and SystemJS).
While the `?.` operator will thus always be transpiled by Babel, even in modern builds, simply supporting it for development purposes seems like a step in the right direction.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining
Given how those are used, there *should* not be any situation in which e.g. `undefined` is ever returned. However, actually checking that the pageView/thumbnail is defined cannot hurt.
Also, re-factor `webViewerPageRendered` slightly since the `pageView` is no longer unconditionally necessary after the previous patches; note in particular that the thumbnails will only be updated when the sidebar *and* the thumbnailView is visible.
Finally, fixes a bug in `webViewerPageChanging` whereby an empty string would not be treated as a valid pageLabel and instead be replaced by `null`.
Given that the default viewer only uses the "page stats" when debugging is enabled, it seems much simpler and more straightforward to simply query the API *directly* when this information is actually required. That way, there's a bit less information that needs to be stored/updated on each `PDFPageView`-instance.
Finally, since the `EventBus` now exists, we no longer need to handle the "page stats"-case in the regular listeners in `web/app.js`, but can instead add special "page stats"-listeners only when debugging is enabled.
The way that rendering errors are handled in `PDFPageView` is *very* old, and predates e.g. the introduction of the `EventBus` by several years.
Hence we should be able to simplify things a bit here, by including the Error (when it exists) in the "pagerendered" event and thus avoid having to reach into `PDFPageView` for it.
Note that a `RenderingCancelledException` *should* never actually reach this method, but better safe than sorry I suppose, considering that both `PDFPageView` and `PDFThumbnailView` are already catching `RenderingCancelledException`s since those are *not* Errors in the normal sense of the word.
For years the loadingBar and sidebarContainer has had a slightly annoying and unfortunate dependency, since the loadingBar width follows the main toolbar width[1].
To prevent the loadingBar from obscuring part of the sidebarContainer, especially the buttons, the sidebarContainer is thus moved down when the loadingBar is visible. This has always annoyed me[2], since it means that the buttons in the sidebar may thus move vertically which seems bad from a UX perspective.
Now that CSS variables are available in all supported browsers[3] however, fixing the loadingBar/sidebarContainer overlap issues are finally easy. The solution is simply to let the sidebarContainer, when visible, control the loadingBar left position (right in RTL locales) in the same way that the viewerContainer is handled. Hence the sidebarContainer can now have a *consistent* vertical postition, without the loadingBar overlapping it.
---
[1] Obviously the right position (left in RTL locales) of the loadingBar is, potentially, reduced to account for a scrollbar.
[2] I've tried to fix this a few times, but it always seemed like more trouble than it's worth.
[3] https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties#Browser_compatibility
The `getVisibleElements` helper function currently requires the viewerContainer to be absolutely positioned; possibly fixing this is tracked in issue 11626.
Without `position: absolute;` set, in the CSS, there's a number of things that won't work correctly such as e.g.
- Determining which pages are currently visible, thus forcing all of them to render on load and increasing resource usage significantly; note https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#allthepages
- Scrolling pages into view, by using the `BaseViewer.currentPageNumber` setter or similar.
Based on the number of opened issues over the years, the fact that `position: absolute;` is required has shown to be something that users can very easily overlook unless they follow e.g. the `simpleviewer` example to the letter.
Hence, to improve things until such a time that issue 11626 is fixed, we'll now refuse to initialize a `BaseViewer` instance unless the `container` has the required CSS set. (Forcibly setting `position: absolute;` on the viewerContainer element is bound to cause significantly more issues/confusion, hence the current approach of throwing an Error.)
