Fix: Sets the pdf scale after page render rather than on resize #178
Conversation
src/lib/viewers/doc/DocBaseViewer.js
Outdated
| @@ -1038,6 +1039,7 @@ const MOBILE_MAX_CANVAS_SIZE = 2949120; // ~3MP 1920x1536 | |||
| if (pageNumber) { | |||
| // Page rendered event | |||
| this.emit('pagerender', pageNumber); | |||
| this.setScale(this.pdfViewer.currentScale); | |||
There was a problem hiding this comment.
Which event listeners don't get triggered? Do some not fire if the scale is auto?
There was a problem hiding this comment.
The click handlers for all point annotations. I'm still looking into why exactly the scale affects the listeners because the listeners aren't being removed at any point...
There was a problem hiding this comment.
It sounds like this solution is more of a stopgap than an actual fix. Am I wrong in thinking this?
There was a problem hiding this comment.
So I spent a while messing with this and when the device orientation changes, this handler is called and so therefore so is renderAnnotations(). The listeners are never unbound and I even went and looked at some of the point annotation icons and they still have the listeners attached to each button.
My conclusion is basically that when the page is re-rendered, the page listeners are attached on top of the annotations. So rather than triggering any point annotation listeners, it registers as a general page click. I'll update this accordingly
There was a problem hiding this comment.
The reason setScale was fixing the issue was that it forced a re-rendering of annotations after the page was rendered.
src/lib/viewers/doc/DocBaseViewer.js
Outdated
| @@ -1038,6 +1038,7 @@ const MOBILE_MAX_CANVAS_SIZE = 2949120; // ~3MP 1920x1536 | |||
| if (pageNumber) { | |||
| // Page rendered event | |||
| this.emit('pagerender', pageNumber); | |||
| this.setScale(this.pdfViewer.currentScale); // Set scale to current numerical scale | |||
There was a problem hiding this comment.
Moving setScale() to after hte page is rendered ensures that the annotations listeners will be on top of the document listeners.
There was a problem hiding this comment.
Can annotations listen to the pagerender event and then re render based on that? Calling setScale again could introduce unnecessary work.
There was a problem hiding this comment.
It shouldn't be called any extra times because I moved it from the resize() method to after the the page is actually done rendering
There was a problem hiding this comment.
pagerendered gets called when scrolling to pages, not always resize/zoom related
There was a problem hiding this comment.
Ah forgot it's also called on scroll. But would re-rendering the annotations on 'pagerender' be any different than setting the scale itself in the pageRenderedHandler()? That would still get triggered on scroll right?
There was a problem hiding this comment.
I have it calling renderAnnotationsOnPage() based on which pages have emitted the 'pagerender' event in the document viewer (in the second commit)
pramodsum
changed the title
pramodsum
changed the title
- Waits for the page to be fully rendered before updating the annotations scale. This ensures that the annotations listeners are on top of the document and it's associated listeners. - Fixes issue where the first device orientation change would result in some listeners being hidden underneath the document listeners, resulting in point annotations not triggering on mobile.
|
@jeremypress let me know if this looks good! :) |
- renderAnnotationsOnPage is triggered on the 'pagerender' event based on which pages have been rendered - If no specific page has been rendered (or is an image file), renders all annotations on the file
event listeners due to the scale not being set