Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a number of regressions/inefficiencies introduced by adding Scroll/Spread modes to the viewer (PR 9208 follow-up) #9832

Merged
merged 8 commits into from
Jun 23, 2018

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Jun 21, 2018

This PR contains a couple of very quick patches, to address a number of regressions/inefficiencies introduced in the viewer by the Scroll/Spread modes PR.[1]
In addition, most of the new code isn't needed (at all) in PDFSinglePageView instances, and this PR also tries to clean that up as well.

Given the regressions, the 2.0.550 pre-release might need to be replaced once this PR lands.

Please refer to the individual commit messages for additional details.

Edit: Please note that the viewer is currently unusable in older browsers (such as IE11), refer to the first commit.


[1] There's probably room for additional clean-up, but someone else is more than welcome to do that :-)

@Snuffleupagus Snuffleupagus force-pushed the scrollMode-fixes branch 7 times, most recently from abe988a to 550538d Compare June 22, 2018 08:35
…wo* parameters, since browser support is limited

The Secondary Toolbar buttons for, not to mention the actual toggling of, Scroll/Spread modes are currently completely broken in older browsers (such as IE11).

As a follow-up, it'd probably be a good idea to try and find a *feature complete* `classList` polyfill that could be used instead, but this patch at least addresses the immediate regression.

Please refer to the compatibility information in https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_compatibility
…PDFViewer._regroupSpreads`

There's no good reason to iterate through an arbitrary number of DOM elements this way, since a document could contain thousands of pages, when everything can be easily removed at once; compare with e.g. `BaseViewer._resetView` and `PDFThumbnailViewer._resetView`.

Furthermore given that it's a `PDFViewer` instance, the `this.viewer` property can be accessed directly. Besides, `_setDocumentViewerElement` only exists as a helper method for `setDocument` in the base class and none of this code applies for `PDFSinglePageViewer` instances either.
…SpreadMode}` methods

Since all the other "public" methods validate the arguments, these (new) ones really ought to do the same.
Given that this method is a no-op in `PDFSinglePageViewer`, similar to `_regroupSpreads`, let's improve the general code structure by simply moving the method.
… that they are no-ops in `PDFSinglePageViewer` instances

Since the Scroll/Spread modes doesn't make (any) sense in `PDFSinglePageViewer` instances, the general structure of these methods can be improved to reflect that.
…s correctly in Presentation Mode, when horizontal scrolling was enabled

Note how in `BaseViewer.forceRendering` the Scroll mode is used to determine how pre-rendering will work. Currently this is broken in Presentation Mode, if horizontal scrolling was enabled prior to entering fullscreen.

Furthermore, there's a few additional cases where the `this.scrollMode === ScrollMode.HORIZONTAL` check is pointless either in Presentation Mode or when a `PDFSinglePageViewer` instance is used.
…g Scroll/Spread modes without a PDF document being loaded
@Snuffleupagus Snuffleupagus force-pushed the scrollMode-fixes branch 2 times, most recently from 044c8ea to 2f1ca98 Compare June 23, 2018 09:45
Since the current page will be explicitly scrolled into view *directly* afterwards anyway (compare with e.g. the `pagesRotation` code), trying to maintain the current position when re-applying the zoom level during changing of Scroll modes is redundant.
@mozilla mozilla deleted a comment from pdfjsbot Jun 23, 2018
@mozilla mozilla deleted a comment from pdfjsbot Jun 23, 2018
@mozilla mozilla deleted a comment from pdfjsbot Jun 23, 2018
@mozilla mozilla deleted a comment from pdfjsbot Jun 23, 2018
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/bc2aa6f64be3a0e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/bc2aa6f64be3a0e/output.txt

Total script time: 8.36 mins

Published

@timvandermeij timvandermeij merged commit e8b5088 into mozilla:master Jun 23, 2018
@timvandermeij
Copy link
Contributor

Thank you for improving this! I have updated the project board so we don't forget to replace the pre-release.

@Snuffleupagus Snuffleupagus deleted the scrollMode-fixes branch June 23, 2018 20:10
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Fix a number of regressions/inefficiencies introduced by adding Scroll/Spread modes to the viewer (PR 9208 follow-up)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants