Issue #223 PR

[TomLaggna]

Based on changes made by @ColinT (see ColinT@5cf6bb0)

Scrolling now updates the page binding and doesn't cause a jump to the top of the page when triggered.

Also fixed a bug where the updateSize calculation would compare pdfjs units to css px.

[ColinT]

This change also moves the scroll container of the underlying viewer inside the pdf-viewer component, and applies position: relative correctly to solve styling issues such as #422.

[jiangyh1024]

when to release @ColinT

[ColinT]

@jiangyh1024 Sorry, I'm not a project collaborator. It will probably be released when the review is complete.

[VadimDez]

My biggest concern is that this introduces scrollbar for the pdf-viwer, i don't think having two scrollbars on the same page is a good idea.

[TomLaggna]

I think pdfjs can't emit a pagechange event without having the container itself be scrollable. If you look at the official mozilla implementation for example, they also use a scrollable container for their pdf.
The change certainly moves this viewer a step closer to the 'suite' style we see with other viewers (talking the usual fullscreen viewer with a toolbar and stuff) and I don't know if you want to go towards this direction with the project.
So I figure, if you want this feature in master and still keep the low restraints and lightweight feeling, we could provide some styling options (e.g.: only show the scrollbar if you hover over the pdf).

[ColinT]

@VadimDez Thanks for the review. You raise a valid point and I think there is a solution. It requires passing the scrolling element and viewer element to pdfjs-dist:

@Input() container;
/* ... */
const pdfOptions: PDFViewerParams | any = {
  container: this.container || this.element.nativeElement.querySelector('div'),
  viewer: this.element.nativeElement.querySelector('div').firstElementChild,
  /* ... other options */
};

this.pdfMultiPageViewer = new PDFJSViewer.PDFViewer(pdfOptions);

I got this working but it has one complication:

pdfjs-dist seems to assume the container is a DOM Element. If the container is only a Node and not an Element, many functions will fail. For example, in the demo the window is scrolling. Since it is not an Element, we have to pass the scroll UIEvent through document.body, which is an Element. If anyone can prove me wrong on this point, that would be fantastic, as currently the workaround is a bit ugly:

ugly code

// app.component.ts
constructor(@Inject(DOCUMENT) public document: Document) {
  window.onscroll = (_ev) => {
    const newEvent = new UIEvent('scroll');
    document.body.dispatchEvent(newEvent); // Tell pdfjs that there is a scroll event happening
  };
}

<!-- app.component.html -->
<pdf-viewer [container]="document.body"></pdf-viewer>

Please see the following forked branch for a working demo:
https://github.com/ColinT/ng2-pdf-viewer/tree/issue-223.2
You can see pagechange events in the console. It is currently just a proof of concept, and not ready for PR. I haven't been thorough with looking at what other effects this has on the underlying viewer.

Feel free to incorporate those changes into this PR or otherwise.

[vishnu-dev]

@VadimDez When will we have this release?

[VadimDez]

Released in 6.0.0 https://github.com/VadimDez/ng2-pdf-viewer/blob/master/CHANGELOG.md#600