Implement a Two Page View Mode

[Snuffleupagus]

This PR is the continuation of the work that @Moistly and @waddlesplash started in PRs #2395 and #2660.
Please note that I got permission from @waddlesplash on IRC to take over the implementation of this feature.

Given that the last actual coding in #2660 was done six months ago, it was easier for me to start fresh rather that trying to patch the old code. Compared to the old implementation, this PR tries to add as much of the code as possible in separate files to avoid cluttering viewer.js too much.

Based on a recent IRC discussion, this PR is on hold until we have implemented testing that covers the viewer.

Things left to do in this PR:

• Once we have a testing framework in place: Implement the necessary tests for this feature.
• Ask Stephen Horlander for icons that fit the new style.
• Revert to the initial horizontal scrolling behaviour. (From working on e.g. Fix horizontal scroll issue when zooming #4188, I've found that there are lots of issues with horizontal scrolling in a cross-browser environment.)

Fixes #590.
Fixes #1475.
Fixes #2376.
Fixes #3658.
Fixes bug 786602.
Fixes bug 864440.

[Snuffleupagus]

Compared to the previous PRs, this implementation of the Two Page View Mode differs slightly.
The reason for this is that the previous versions suffered from a couple of smallish, but annoying, alignment issues which turned out to be difficult to solve in a straightforward way. It might be possible to find a solution, but I fear that the it would make the CSS rules unnecessarily complex and thus difficult to both understand and maintain.
Among these issues: controlling the vertical distance between pages became more involved; the pages are shifted slightly off-centre horizontally (even in one page mode); if a document with different page sizes is displayed in two page mode, there are issues when trying to change the vertical alignment of the pages.

[waddlesplash]

I think that now that the items are in a menu with separators, you can change "Show Cover Page in Two Page View" to simply "Show Cover Page". It'll be less redundant.

Other than that, nice job!

[Snuffleupagus]

@waddlesplash Thanks for the feedback!
I think you have a good point about "Show Cover Page", lets see if others agree.

[timvandermeij]

@Snuffleupagus Two things:

• I must say I'm a bit confused by the text "Show Cover Page in Two Page View", or "Show Cover Page" for that matter. I think it implies that the cover page would be hidden otherwise, which isn't the case. It's just that it can be viewed separately from the other pages. I would lean more towards something like "Show Cover Page saperately", but that's just my two cents!
• If you go to two page view and reload the page, the secondary toolbar is opened on each refresh. I don't know if that was done intentionally, but I can imagine that users are annoyed by that because they have to close it everytime.

The functionality in this PR is really nice. I'll have to take a closer look at the code, but it appears to be working really good. I'll also take a shot at making some descriptive icons.

[Snuffleupagus]

@timvandermeij Regarding your comments:

1. The text "Show Cover Page in Two Page View" is taken from the previous PRs. I believe the reason for using that particular string was that it is used in Adobe Reader. This was discussed previously, see: Two-Up View Redux #2660 (comment).
   2. I forgot to remove that before pushing the code. (There actually was a reason for implementing it this way, but that isn't relevant currently so no need for it.) Fixed in new commit.

[timvandermeij]

@Snuffleupagus I see, Adobe Reader does it too. Strange choice of words in my opinion, but okay then :)

I agree with @waddlesplash that 'Show Cover Page' would be good enough as it is really just an extension of the two page view option and not a separate option.

[timvandermeij]

I have added some review comments. I think what remains for this PR is:

• Address review comments.
• Does the 'show cover page' icon need to be flipped for RTL languages, @ebraminio?
• I also suggest changing Show Cover Page in Two Page View to Show Cover Page.

[ebraminio]

No it doesn't need but I suggest using a language independent icon for cover icon as not all languages are using Arabic digits (1234...), something like a simplistic nature view http://openclipart.org/people/Machovka/Machovka_cloudy.svg or something more applicable

[Snuffleupagus]

The review comments have been addressed and the l10n strings are changed slightly. The last commit also contains a change that prevents issues, if the user tries to change the page mode before the document has begun to load.

/botio-linux preview

[timvandermeij]

I found the comment from @ebraminio very useful and decided to design another icon for the 'show cover' functionality. Take a look at the new icon below. It no longer contains a digit or other language-specific items, it's literally what I could think of as a cover page: a big title on top and lines of text below it. I have tested it for this PR using github-try and it looks good to me. Since the icon is symmetric, there is also no need to mirror it. Thanks for the suggestion, @ebraminio!

@Snuffleupagus What do you think of this new icon? If you like it, could you add it to the PR?

[pjaytycy]

What about something like this?

big version:

[Snuffleupagus]

@timvandermeij I don't dislike it. ;-)
That said, when I think of a cover page, I associate it more with a big title on top and an illustration beneath. Cover pages, in my experience, are rarely covered with lots of text.

@pjaytycy I'm not exactly sure if you meant including all of your illustration as an icon, or just the top "page"!?
But the issue is that whatever we use, the icon size is limited to 16x16 pixels.

[timvandermeij]

@Snuffleupagus I was inspired by the Tracemonkey PDF, but perhaps that's not the best example of a real cover page. I can try adding a simplistic image (barely noticable because of the 16x16 format, but still...) later and see if that works out better.

[Snuffleupagus]

I can try adding a simplistic image (barely noticable because of the 16x16 format, but still...) later and see if that works out better.

What about a title (like you already did) and then just a square, suggesting an "image", below it?

[timvandermeij]

This should be more like it. It has a title, a subtitle and an image. I have made the lines transparent so it blends better with the PDF.js UI (that's why it's barely visible here below).

[timvandermeij]

@Snuffleupagus Could you fix the merge conflicts? Also, is something still blocking this patch?

[Snuffleupagus]

Also, is something still blocking this patch?

Yes, the fact that I've been unable to get the toggling of page mode working as it should from the context menu in Presentation Mode. At this point, I leaning towards just scrapping this part of the patch. That would of course mean that the only way to change page mode is from the Secondary Toolbar, before entering Presentation Mode.
A related question: Should it even be possible to use the Two Page View Mode in Presentation Mode!?

[waddlesplash]

A related question: Should it even be possible to use the Two Page
View Mode in Presentation Mode!?

Yes. It's useful.

[waddlesplash]

@Snuffleupagus what else needs to be done here?

[Snuffleupagus]

@waddlesplash I don't think that there is much else, so reviewing I guess.

[waddlesplash]

Awesome. I say it's ready to be merged, but I'm not a reviewer 😄

[timvandermeij]

Let's get this PR rolling again. It seems like a really useful feature to me and I think it's practically ready.

@Snuffleupagus: Could you resolve the merge conflicts?
@brendandahl, @yurydelendik: Could either one of you review this?

[Snuffleupagus]

Let's get this PR rolling again. It seems like a really useful feature to me and I think it's practically ready.

I would like the functionality of PR #3967 to land first, to be able to add code to scroll pages into view properly (horizontally) in two page view mode.

[timvandermeij]

@Snuffleupagus Could you rebase this? Also, is there anything that needs to be done for this PR according to you? If not, we should review this.

[Snuffleupagus]

Also, is there anything that needs to be done for this PR according to you?

I don't believe there is. I've made a number of small improvements compared to the first version, but at this point I don't think that I can do much more.

/botio-windows preview

[pdfjsbot]

From: Bot.io (Windows)

---

Received

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

Live output at: http://107.22.172.223:8877/902365025dcf953/output.txt

[yurydelendik]

https://bugzilla.mozilla.org/show_bug.cgi?id=1009576

[avih]

Is there still a plan to add two-pages display?

It's quite an important feature of a PDF viewer IMO, and content which was meant to be displayed as two-pages, like magazines, really suffers if it can't.

[toerb]

The PR is now open for over 600 days. What is holding it back?

[timvandermeij]

The main problem is that pages can be of different sizes and that the current implementation might not deal with that properly. There is also the problem of testing this: it has to be done manually as there are no browser tests for this yet.

[micsanbr]

Is there a document that explains how to implement the changes from @Snuffleupagus (a few posts above this) in the latest release of pdf.js ?
Is this feature still going to be implemented in the future ?

[timvandermeij]

@micsan13br You would have to clone the branch and rebase it onto the current master branch. After that you should be able to work with it using node make server.

[thomasflad]

I've tried to rebase the changes from @Snuffleupagus but the changes are complexer then I thought. Maybe someone can help here?

[brendandahl]

I'd be more comfortable with landing this feature with more tests. In the mean time I'll close it until we have done something about #6505.

[avih]

In the mean time I'll close it until we have done something about #6505.

TBH this doesn't sound like a great reason to close this PR.

Admittedly the patch is very bitrotted, but it's probably the place where most discussions of this feature happened, and closing it would hide this discussion (e.g. while searching within open issues).

[timvandermeij]

Discussions about the issue should take place in #590. Discussions here should only concern this particular implementation. Since this patch is needs a lot of work and there are no tests yet, it is better to close it and to revisit it later on with a fresh implementation rather than leaving it open and keeping it in the queue for several years.

By the way, I think that this patch can be simplified quite a bit given the current code base, so revisiting this is not only a good idea for the tests, but also for the code quality. Keep in mind that this patch was written two years ago.

[Snuffleupagus]

By the way, I think that this patch can be simplified quite a bit given the current code base, so revisiting this is not only a good idea for the tests, but also for the code quality. Keep in mind that this patch was written two years ago.

I completely agree with the above!

I'd be happy to try and re-implement this, once we have the necessary testing framework in place.

(One of the reasons that I stopped working on it, is that it wasn't clear to me if we really wanted to introduce all this complexity in the viewer.)

[brendandahl]

TBH this doesn't sound like a great reason to close this PR.

Yes, sorry there's a bit more context. We're are trying to clean up the pull request queue and only have actively worked on(within the last month) things there. This will hopefully leave less pulls in an unresolved limbo like we currently have.

[avih]

Yes, sorry there's a bit more context

Thanks. Appreciate the info.

[jepz88]

@Snuffleupagus i know this has been discussed, but will this be implemented in the soon future? :) Saw the demo of this by you, is it possible to use that solution until the official release? :)
/Jesper

[Snuffleupagus]

I know this has been discussed, but will this be implemented in the soon future?

As outlined in #3723 (comment), further work on this is blocked on test coverage for the viewer. Hence it's currently not possible to provide any time-line for this.
Once the necessary testing framework is in place, the functionality in this PR would have to be (at least partially) re-implemented from scratch, since the default viewer has changed significantly since this code was written.

[...] is it possible to use that solution until the official release?

I suppose that it's technically possible. However, in its current state, the PR will most likely not work with an up-to-date version of PDF.js. (Please note that it doesn't make sense for me to continue working on this, until such a time as it's actually possible for this feature to land.)

[michal-drozd]

@Snuffleupagus: Is there any chance, that Your PR d81c30b would be working on actual version of pdf.js ? Disregarding tests my team is desperately need this feature.

[prohtex]

@Snuffleupagus @michal-drozd I also have a need for this feature. Is there any hope of getting the code working on a current version of pdfjs?

[timvandermeij]

This is already answered in #3723 (comment).

[prohtex]

@timvandermeij Its too bad development of such a crucial feature is hinged on #6505 which seems to be stuck. Until then, I guess I am using 0.8.852 on a production site. Aside from performance drawbacks, the 2 page view implemented in this PR by @Snuffleupagus is excellent.

[nikhilweee]

Any chances of this being accepted anytime soon?

[timvandermeij]

Another pull request is currently in the review queue that implements this in an easier way.