Disable pagechange event if page is not changing. #7289
Conversation
|
Let's see if it breaks our viewer (wondering about presentation mode enter/exit and pdf opening) |
|
|
This seems to cause some glitches related to Presentation Mode, e.g
Note how the top of the first page is now scrolled down a bit, when compared with the same steps in http://mozilla.github.io/pdf.js/web/viewer.html. |
|
The title of the PR is "Disable pagechange event if page is not changing.", but the code actually goes a bit further than that. If the intent of the patch is simply to not fire the event unnecessarily, then we probably still need https://github.com/mozilla/pdf.js/blob/master/web/pdf_viewer.js#L192-L196 even in the |
That what I was afraid of. We need to remove this logic from currentPageNumber somehow. |
yurydelendik
force-pushed
the
double-pagechange
branch
2 times, most recently
from
adbb41f to
3c1832d
Compare
|
I was not able to reproduce to STR above. However I agree that not calling this.scrollPageIntoView might break things. I refactored it further to remove updateInProgress and extracted _resetCurrentPageView -- now the logic must be preserved. |
| @@ -673,13 +676,11 @@ var PDFViewer = (function pdfViewer() { | |||
| } | |||
|
|
|||
| if (!this.isInPresentationMode) { | |||
| this.currentPageNumber = currentId; | |||
| this._setCurrentPageNumber(currentId, true); | |||
There was a problem hiding this comment.
this might cause the confusion -- we need to rethink this logic in the future
yurydelendik
force-pushed
the
double-pagechange
branch
from
3c1832d to
b4d5558
Compare
|
|
||
| _setCurrentPageNumber: | ||
| function pdfViewer_setCurrentPageNumber(val, updateInProgress) { |
There was a problem hiding this comment.
I'm wondering if we actually still need the updateInProgress parameter here, and in the events?
Previously we called scrollPageIntoView from the event handler in app.js (or viewer.js as it were), which seemed bad for e.g. the viewer components. In PR #6140, I removed some dead code, and also refactored this. With the changes in this patch, it seems to me at least, that updateInProgress is now pointless.
(Obviously there might be someone relying on it, but given that we've not really made any promises about the stability of the viewer components, I think we can remove it.)
yurydelendik
force-pushed
the
double-pagechange
branch
from
b4d5558 to
ce2e31a
Compare
yurydelendik
force-pushed
the
double-pagechange
branch
from
ce2e31a to
9f68e3f
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/870fb4223fab35d/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/870fb4223fab35d/output.txt Total script time: 1.06 mins Published |
|
I can confirm that issue I reported in #7289 (comment) is now fixed. Looks good, thank you for the patch! |
Fixes #7288