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.

Sign up for GitHub

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

Jump to bottom

Fix page-switching for landscape documents with SpreadModes and PresentationMode (PR 14877 follow-up) #15798

Merged
merged 1 commit into from
Dec 9, 2022
Merged

Fix page-switching for landscape documents with SpreadModes and PresentationMode (PR 14877 follow-up) #15798

merged 1 commit into from
Dec 9, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

In PR #14877 I forgot to update the horizontal padding, used when computing the scale of the pages, for the case where SpreadModes and PresentationMode are being used together.

Steps to reproduce:

  1. Open the viewer with the default tracemonkey.pdf document.
  2. Enable any SpreadMode.
  3. Rotate the document once, either clockwise or counterclockwise.
  4. Enter PresentationMode.
  5. Try swithching page, e.g. by clicking on the document.

Expected result:
The visible pages change as you click.

Actual result:
The visible pages are "stuck" in the current view.

@Snuffleupagus
Copy link
Collaborator Author

This was found during testing of PR #15770.

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Dec 9, 2022

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/7e03c18d054f847/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Dec 9, 2022

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/7e03c18d054f847/output.txt

Total script time: 1.25 mins

Published

@calixteman
Copy link
Contributor

If I understand correctly the original issue:

  • the current page is 1 because it's the first visible one
  • but page 1 is 100% visible when page 2 is 99% visible
  • hence next page will set current page to 2
  • then scroll page 2 into view (which is already there)
  • update the current page in taking the first visible one which is page 1
  • consequently we're stuck on page 1

So your fix is finally just making page 2 100% visible and then next page will be 3 and now everything is working.
I think I now understand better why you "worry" about #15770.

@calixteman
Copy link
Contributor

I wonder if be could use a ResizeObserver and use a flex layout or something to layout the pages.
This way the layout will be done automatically, then the pages will be resized and thanks to the observer we can adjust the scale factor when redrawing the visible pages.

@Snuffleupagus
Fix page-switching for landscape documents with SpreadModes and Prese…
47ac706 
…ntationMode (PR 14877 follow-up)

In PR 14877 I forgot to update the horizontal padding, used when computing the scale of the pages, for the case where SpreadModes and PresentationMode are being used together.

Steps to reproduce:
 1. Open the viewer with the default `tracemonkey.pdf` document.
 1. Enable any SpreadMode.
 2. Rotate the document *once*, either clockwise or counterclockwise.
 3. Enter PresentationMode.
 4. Try swithching page, e.g. by clicking on the document.

Expected result:
 The visible pages change as you click.

Actual result:
 The visible pages are "stuck" in the current view.
@Snuffleupagus Snuffleupagus force-pushed the PresentationMode-spreadMode-hPadding branch from f558ea1 to 47ac706 Compare December 9, 2022 13:10
@Snuffleupagus Snuffleupagus merged commit 3155e2a into mozilla:master Dec 9, 2022
@Snuffleupagus Snuffleupagus deleted the PresentationMode-spreadMode-hPadding branch December 9, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Snuffleupagus @pdfjsbot @calixteman