Skip to content

Commit

Permalink
Ensure that the viewer property, on BaseViewer-instances, is a va…
Browse files Browse the repository at this point in the history 
…lid div-element (issue 12320)

This should help prevent future issues, caused by the user omitting the `viewer` option and/or providing an incorrect `container` option, when initializing a `BaseViewer`-instance.
  • Loading branch information
@Snuffleupagus
Snuffleupagus committed Sep 3, 2020
1 parent 84da13b commit 89f6bb0
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions web/base_viewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,17 @@ class BaseViewer {

this.container = options.container;
this.viewer = options.viewer || options.container.firstElementChild;

if (
(typeof PDFJSDev === "undefined" ||
PDFJSDev.test("!PRODUCTION || GENERIC")) &&
!(
this.container instanceof HTMLDivElement &&
this.viewer instanceof HTMLDivElement
)
) {
throw new Error("Invalid `container` and/or `viewer` option.");
}
this.eventBus = options.eventBus;
this.linkService = options.linkService || new SimpleLinkService();
this.downloadManager = options.downloadManager || null;
Expand Down

4 comments on commit 89f6bb0

@jsg2021
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke rendering to another frame. HTMLDivElement is unique per window so an instance of HtmlDivElement from a subframe/alternate window will fail this test since instanceof only can check the prototype chain.

@jsg2021
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably test the values for the expected interface instead. Something like ?.tagName === 'DIV' && ?.ownerDocument != null ?

@timvandermeij
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a better check that works in both scenarios, then we're certainly open for pull requests to improve this. Thanks.

@jsg2021
Copy link
Contributor

@jsg2021 jsg2021 commented on 89f6bb0 Sep 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How aggressive do you want the check? This is probably the most aggressive:

const getDocument = (elm) => (elm || {}).ownerDocument || document;
const getWindow = (elm) => (getDocument(elm) || {}).defaultView || window;

const isHTMLDivElement = (elm) => elm instanceof getWindow(elm).HTMLDivElement;

Please sign in to comment.