-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add scrolling and spread modes to web viewer #9208
Conversation
5525050
to
2e6654c
Compare
2e6654c
to
7ccdf60
Compare
Is there a way to get this as a viewer option, similar to setting up the zoom or page? Having the ability to do |
/botio-linux preview |
It looks like it works quite well, so nice work! Since this is quite a big viewer change, we do need some time to play with this to make sure there are no issues, but at first glance the solution looks pretty clean. I did find one issue with horizontal scrolling so far. Open the viewer generated above, go a few pages further (for example page 6) and then select horizontal scrolling. Notice that you can scroll all the way to the right, but on the left you cannot scroll farther back than page 6, i.e., pages 1-5 are not reachable anymore. |
Thanks for taking a look! I see the problem and I'll touch up the PR later tonight. |
7ccdf60
to
8b4dc92
Compare
Okay, that issue should be fixed. @jlac1024, in principle I don't see why not but what I think is the relevant code doesn't look trivially extensible. I doubt it'd be a lot of work to do, but probably someone who understands why the existing code is written the way it is should do it. |
/botio-linux preview |
Hi guys! Are there any news on this? Thanks! |
This works great, many thanks. Testing: #590 (comment) |
Many, many thanks to @rhendric for this very badass piece of code! Please see my amended comments below. |
@prohtex, I'm quite confident that a small extension to this PR could achieve what you're looking for. My main reservation is that I'm not sure how best to manage the proliferation of possible options, and I didn't want a merge to be blocked on a UX debate for how to manage the choice between vertical, horizontal, two column, two column with cover page, fit-to-width columns (what I ended up calling ‘grid’), and fit-to-width columns with cover page. I ended up implementing what I thought was the best balance between giving people the ability to get the view they want, perhaps with a little extra viewport fiddling, and keeping the number of new UI elements low. But I can try adding a two column mode if @timvandermeij, or whoever is the gatekeeper for not letting UI elements get out of hand, approves two more menu items for it (or suggests I try a different interface). This PR has already been sitting around for almost two months, though, so if there's any additional merge risk at all to trying more experiments, I'd rather get this merged first and then play around with more modes afterwards. |
@prohtex keep in mind that implementation shall work with multi-size page PDF documents as well. Can you verify your solution for https://github.com/mozilla/pdf.js/raw/master/test/pdfs/sizes.pdf and https://github.com/mozilla/pdf.js/files/703007/PeakLoadGrowth_Redacted.xlsx.pdf ? |
@rhendric, thanks for your reply. I hope this PR gets approved as soon as possible. Maybe it would be possible down the line to add a flag somewhere in the code that would allow the user to "lock" grid view to a max number of pages (be it 2 or more). It would also be great if there were a way to activate these modes from the URL or with a cookie. In my use case, I need to provide the client with a button in the toolbar they can click to quickly enable and disable 2 page mode. @yurydelendik, I'm not exactly sure what you mean. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't intend for this to come off as negative, given the amount effort that was very clearly put into this PR, but unfortunately I think that there's a couple of issues with this PR as it currently stands.
Please note that I haven't done an in-depth review of the code, but the following is a couple of bigger picture things that I've found thus far. The central one is that there's a number of assumptions made in existing code, assumptions that will no longer hold with the changes proposed in this PR.
-
First, and foremost, please note that the last attempt to add a TwoPageView mode was ultimately rejected in no small part because of missing tests for the viewer UI.
Even though the current implementation is smaller, w.r.t. the amount of added code, it still seem like a somewhat risky change without any tests. There's always a risk of bugs in edge-cases, see e.g. (the now fixed) Add scrolling and spread modes to web viewer #9208 (comment); refer also to some of the points mentioned below.
It's rather unfortunate that we still don't have any tests for this part of the code-base, but I'm not
sure where that leaves us here!? -
Pre-rendering of the next/previous page, i.e. the one after/before the visible ones, depending on the scroll direction is currently not working correctly in the "Horizontal Scrolling" mode; see
PDFRenderingQueue.getHighestPriority
.
Currently thewatchScroll
method only tracks vertical scroll, since that's the only thing that mattered, however that's no longer correct here. While we might be able to hack around that here, I cannot help wonder ifwatchScroll
shouldn't be re-factored to be more generic instead!? -
In order to reduce the size/scope of this PR, should we perhaps not implement the "Horizontal Scrolling" mode initially, since that isn't a commonly requested feature compared to the TwoPageView mode!?
(Please see the point above as well) -
The single most serious problem with this PR, is that the
getVisibleElements
function will unfortunately not work in either the "Horizontal Scrolling" or "Grid Scrolling" modes for PDF documents with varying page sizes; e.g. https://github.com/mozilla/pdf.js/files/1513951/PageSizes_output.pdfWhat can happen here, is that
getVisibleElements
will miss pages that are actually visible, leading to some pages failing to render; note this condition. I'm just not sure how we'll be able to address that, with only limited changes togetVisibleElements
, such that we won't end up regressing performance for the regular (vertical) scrolling mode.
Given that it's imperative thatgetVisibleElements
works correctly, in order for the viewer not to break in "interesting" ways, e.g. a complete re-write ofgetVisibleElements
could pose some risk with only manual testing available (since it's difficult to cover all cases). -
The "Grid Scrolling" mode, as currently implemented, will most likely be quite confusing to users since its behaviour is highly dependent on a number of factors. E.g. the width of the browser window, the current zoom level, the size of the pages in a particular PDF document, and so on...
I think that Add scrolling and spread modes to web viewer #9208 (comment) summarizes this quite well, and I'd worry that we'd get loads of questions from users that are confused about this.From a quick look at all the issues filed in both GitHub and Bugzilla, it seems to me that what people generally are asking for (with few exceptions) is a "TwoPageView" mode similar to what most other PDF viewers implement.
Please also keep in mind that the "TwoPageView" mode is a concept defined in the PDF specification itself, see e.g. http://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G6.2393749.
Hence it's possible for a PDF document to specify that it should be displayed with a layout of two pages side-by-side, which we wouldn't really be able to support correctly with the "Grid Scrolling" mode as implemented here.So my suggestion would be that we should, at least initially, only implement a "TwoPageView" mode rather than a generic "Grid Scrolling" mode.
-
While this implementation obviously only makes sense for the
PDFViewer
, thePDFSinglePageViewer
(or more likelyBaseViewer
) must still implement the same exact interface (with no-op methods) to prevent errors. -
Regarding the requests for a hash parameter to control the view mode, given that no such thing exists in the specification: http://www.adobe.com/content/dam/acom/en/devnet/acrobat/pdfs/pdf_open_parameters.pdf, I'd suggest that we probably should avoid doing that since it would be non-standard.
I'm really sorry to say that I don't have good answers to some of the issues/questions raised above, which are mostly caused be the state of already existing PDF.js viewer code :-(
Also, I'd assume that we'd want to try and get PDF.js version 2.0
officially released first given its scope, before this PR can land (to reduce risk).
@Snuffleupagus, ❤️! You aren't coming off as negative, just as a good reviewer doing your job. I see two broad classes of issues that you've raised: technical-level (a thing doesn't work correctly) and design-level (we shouldn't do this for reasons). I appreciate both, and I'll look into the technical issues myself shortly. I would like to say a few words in defense of the design. First of all, although it may not be apparent from a cursory glance, this implementation is almost entirely CSS (this might change after I address the technical issues you've brought up, so maybe this point won't be as strong in the near future), unlike previous PRs implementing two-up views by you and others. There is no modification to how HTML gets generated or to JavaScript class structure. All but eight lines in the diff of JavaScript files are boilerplate responsible for handling click events from the new menu items, toggling the selected state of those menu items, and adding and removing CSS classes from the single PDF viewer element. The remaining eight lines are: five lines in I know that the new scrolling modes offered here are not common (if not entirely unavailable) in other PDF readers, and I appreciate that as the nth implementor of a PDF reader, you're more concerned with catching up to where the other implementations are than with innovating. But I think the fact that this is all done with CSS is an important argument in favor of innovating here—other PDF readers don't have the competitive advantage of a full CSS layout engine automatically available to them, and so for them, supporting multiple new scrolling modes involves answering all of those obnoxious questions about what to do with oddly-sized pages and how to scroll to pages and so on. For them, the barrier to this kind of innovation is much higher. I also think that, as is the case with most innovations, we can't rely on what users have asked for as an accurate signal of true demand. Users who have seen two-up views in other PDF readers, and who want to see as many pages as possible on their screens, will naturally ask for two-up views here instead of ‘as many pages as will fit’ views, despite the latter being a better satisfier of their needs. Now, I do hear loud and clear that there are use cases, particularly revolving around reviewing print media, where two-up is superior to grid, and I'm not trying to minimize that. I'm just arguing that not hearing many people ask for horizontal or grid scrolling does not necessarily mean that people don't want those modes. Who knows, once they've seen it in Firefox, maybe they'll demand it elsewhere? Personally, I have been searching for years for a PDF reader that does horizontal scrolling—it makes reading academic and technical papers, which frequently refer back a few pages to tables and diagrams, so much easier—and before I implemented this, I couldn't find any that worked for me. If this innovation is going to take root anywhere, it's going to take root here, because the barriers elsewhere are much higher and nobody has a particularly strong incentive to buck the consensus. I'm asking you to please take that into consideration when reviewing this PR—yes, you have the health and stability of your project to think of, but you also have a unique opportunity to serve an unaddressed need in the broader PDF reading community due to building on the shoulders of browser layout engines, and I hope that as a community-driven project, you see the potential value there as partially offsetting some amount of risk of bugs and user confusion. (On the topic of user confusion, I get that people are surprised by the unfamiliar ‘Grid’ mode and I'm eager to alleviate that as much as possible. We could rename ‘Grid’ to ‘Wrap to Fit’, perhaps? Different icons? Move the scrolling modes into their own door-hanger popup where we have more room to explain things? If this becomes the major outstanding concern, I'm prepared to submit a few different design ideas and I'm confident that we can find something that most people will find clear.) |
I need to chime in to say that I really hope this PR doesn't get mired in the kind of philosophical debate that has prevented every other workable two-page view from making its way into the base branch. I haven't contributed a line of code to this project, but I've implemented pdfjs for several web projects and I am extraordinarily grateful for all the hard work of those who have. I'm going to ask a question because I honestly don't know the answer at this point: is it a priority at all to see wider use and adoption of pdfjs? If so, it seems it is critical to support a working two-page view. Not tomorrow, but now. I'll admit, when I first saw @rhendric's solution I thought maybe it was boondoggle. Now after playing it with a few days I think it may actually have a lot of practical applications. There are plenty of printed materials that benefit from being viewed as a long row of horizontal pages (graphic novels, for one), or in a 4-up or 6-up view. The real issue here is lack of a feature I will call "spread lock." While it is true that not all PDFs are reproductions of bound printed material, this is my use case, and so this is what I will discuss. Since Gutenberg, most bound printed material relies on the concept of spread-based layout, eg a content area that stretches across two pages. Not every spread employs elements that cross the gutter, but all books use spreads. A PDF of a book is a single cover page followed by a series of spreads. What grid scrolling needs is a "spread lock", that will prevent these spreads from being broken up. With spread lock enabled, it would be possible to view 2 pages, 4 pages, 6 pages, and so on, but not 3, 5 etc. This is good: This is bad: Spread lock could simply be an additional option that is toggled on and off. I understand there are a few kinks to work out, but the code looks good, and ready to be tested and supported. Not tested with hypothetical viewer testing methods someone may develop down the line, but tested ad hoc, right now. I suppose the final concern about this PR is additional UI clutter. Perhaps it would be possible to have a menu option that would turn grid scrolling on and off, and then a submenu or dialog with the additional options, such as horizontal, spreads, etc. @Snuffleupagus, I reviewed the linked Adobe spec. True, Adobe does not provide a spec for this kind of parameter. I don't know enough about this project to know why it would be important to adhere to this particular Adobe spec, but if it is, perhaps cookies or another solution could be used. I'd really like to be able to send users to a PDF viewer that has 2 page "spread lock" view enabled by default. That's it. |
It would be nice to actually have something that's going to fill the niche at this point. Semantics about some esoteric use because 0.01% of people who use PDF.js might not be able to use the new feature (but can still use it normally) doesn't seem like a worthy justification of not incorporating it. Lopsided and odd/random page sizes is something to be concerned about, but I imagine someone who this impacts will just not use the feature. |
I love the idea of spread lock. It strikes at the heart of what that view is for. When I revisit this (soon, but not right now), I'm going to aim to replace the current four-state toggle with two three-state toggles: vertical, horizontal, and wrap; and no spreads, even spreads, odd spreads. The spread toggle will have no effect in horizontal mode but will keep the appropriate pages adjacent in two columns (in vertical mode) or 2n columns (for wrap mode). |
@rhendric Thank you, thank you, thank you! I can't wait to test it out. |
@Rob--W, previous review comments addressed. The expected behavior now is that a document will take the scroll mode from the user's preferences if and only if the user has never explicitly changed the scroll mode for that document (and same for spread mode). |
This looks great. Thanks for your work, responsiveness and patience throughout this whole PR! |
This landed in mozilla-central a few days ago, and the strings are likely going to create problems for translation. For example, there are basically no references for "Wrapped scrolling" in Google (or, at least, I couldn't find them). Would there be any alternative phrasing to describe these features? I'm not asking to change them in the prod, just to provide an alternative, using common language. For example: "page spreads" -> "group of pages"? No clue about the wrapped scrolling. |
@rhendric @prohtex Can you reply to #9208 (comment) ? @flodolo There has been some previous discussion about the spreads terminology from #9208 (comment) onwards. |
Thanks for the link. I was indeed able to find some references to "page spreads" in Microsoft before, even if they usually refer to either "one-page spread" or "two-pages spread".
Right now, the biggest problem is "wrapped scrolling". |
Describing the scrolling modes is probably easiest in comparison with each other:
I'm not sure what the best way to express that idea in other languages is (I'm not even sure ‘wrapped scrolling’ is the best way to express it in English), but ‘wrap’ here was chosen in the sense of line wrap, and maybe the translation of that concept into other languages would be a good starting point. |
I'm confident that after the rigorous debate we had on the matter, the current terminology is both accurate, specific, and understandable. The problem of translating a label into another language is just that -- a problem for someone who understands both English, and that other language, well. In my experience it is fairly common for something to be succinct in one language and somewhat more verbose in another. I don't think it makes sense to choose something less precise, less descriptive, and more awkward, just because it will translate more readily. Surely there are accepted terms in other languages for "Spread." Furthermore, simply Googling the names of these terms won't necessarily yield anything useful about their fitness for this given featureset (Although in my case, the first result I found searching for "two page layout" referenced "spreads": https://support.office.com/en-us/article/view-a-two-page-spread-3d58c31a-5269-4a18-915b-2a2c559494a9#bmtps_is). If someone wants to come forward with a widely understood, common practice term for "grouping of two opposing pages meant to be experienced as a single layout unit" I'm eager to hear it. Until then, I still feel "spread" is the correct term. As for "wrapped scrolling," as @rhendric has mentioned, "wrapping" is a widely understood print and web term that seems, to me, to be well-suited here. It very simply describes what is happening -- the pages are being wrapped. Yes, "wrapping" could be broken out into a toggle, but it would only be useful in vertical mode. An alternative label name then might be the clunky "Vertical scrolling with wrapping," although I can't see what additional clarity that would provide. At this point, unless real bugs or issues are discovered with this PR, maybe the best course of action would be to table the debate about terminology in favor of letting things move forward. I'd encourage anyone who wonders how these particular features and labels were settled upon to scroll up and avail themselves of the extensive discussion on the matter. |
I think it would be "Vertical scrolling with wrapping", right? |
Please excuse me, I have corrected and amended my statement above. |
When I put "two-page spread" into Google translate for Spanish, for example, I get this: propagación de dos páginas I do not know Spanish but this appears to be something like "propagates across two pages." When I enter "spread her legs," I get this: extiende sus piernas This indicates to me there might be a useful distinction here. |
This entry in Microsoft Terminology may also be helpful:
|
@flodolo @rhendric about wrapping: the problem is not that we don't know what wrapping means. It's just that in German "wrap-around" doesn't exist at all. And we can't make something up (if we want people to understand it). @collinanderson "Vertical scrolling with wrapping" is too wide for that menu, I think (especially in German) @rhendric But there are other terms known to users of widely used software (international terms), that describe the same even better. |
@Djfe I'm not sure I understand why "Double Page Layout/Design" is any more succinct, specific, or widely understood than "Spread." Your proposal is to change No Spreads to No Double Page Layout/Design ? My feeling is still that it is not unusual for text to be longer in translation than it is in English. Often a product will be labelled "Door Handle" in English and something like "Lo que una persona se convertiría con el propósito de abrir una puerta" in Spanish. Not to be ethno-centric, but I think the challenge is to come up with a set of terms for each localization that is true to that language, even if it must be a bit longer. Translating "wrapping" to "grid" makes a lot of sense to me, as that is what this feature was called to begin with (grid scrolling). |
Translations like "gerades/ungerades Seitenlayout" (even/odd page layout) would work, but would leave users confused. (except if your job is publishing books/pdfs) Unlike "Show cover page separately" (which equals even page spread) I definitely have ideas for translations (like Seitenlayout), if you don't like my suggestions. |
That's interesting because I know only a few phrases and art history terms in German and yet "Seitenlayout" is something that is immediately clear. Page layout? That's confusing? How about "One Page Layout" and "Two Page Layout," or "Single Pages" and "Double Pages"? |
My last comment, wasn't a reply to you (ups) This will be my reply to your first post: probably some specific German term from the publishing domain exists, that I can't find with Google right now.
No my actual proposal is in #9813 The rest were just ideas floating in my head, from the time before I started looking at other software to get some inspiration.
yeah I just discovered that term, while trying to read all posts from this discussion (I'm still not finished, yet) hehe for the second post of yours:
exactly, but noone ever heard of a page layout that could be even or odd
you should take a look at #9813 :) |
In german when you talk about even or odd, people tend to think: Another way to look at it: It doesn't tell you right away, that it refers to the first page of the duo. |
Desktop Publishing Software seems to refer to page spread in the following way: |
@Djfe, I took a look at #9813. It seems like you've put a lot of thought into this. If this were something that was just introduced, I'd say it is worthy of debate. But I just took 20 minutes and scrolled up on this thread. #9208 (comment) As you can see there were quite a few other suggestions for how these menus might be organized. There was discussion of a submenu. At one point the spread lock was a 2-way toggle with no explicit "off" state. In short, quite a few options were considered. I'm pretty confident what we arrived at is both a good compromise, and quite clear from a UX standpoint. |
|
@Djfe, @prohtex, I propose moving this conversation to #9813, despite there already being a lot of prior discussion on the topic of UX and names here. This PR's conversation thread is already enormous, and moving to #9813 would make it clearer when (and if) there is a resolution satisfactory to all. |
@rhendric, thanks for your comments on #9813. When I have a chance I will review more fully and chime in. I just wanted to say that I think there is a little more than can be done to make "spread view" (or whatever you want to call it) useable, apart from renaming the various options. The main improvement from my perspective would be adjusting the page margins. Currently, with "even spreads" enabled, a zoom level of 50% or so looks great, with the cover page on its own row and subsequent pages side by side. Zoom out further however and things get strange. This was my comment here: #9208 (comment). In any case, thanks as always for your dedication and I'm thrilled things are moving forward at long last. |
I think it would be best to discuss further improvements in their own issues. Mention me in those issues and if they cover topics already suggested (like squeezing the intraspread margin) I'll add a link in the corresponding item in the to-do list at the top. |
Add scrolling and spread modes to web viewer
This PR adds two new features to the web viewer: scrolling modes and spread modes.
With scrolling modes, horizontal scrolling and wrapped scrolling are added as alternatives to the default, vertical scrolling. (This layout is done entirely with CSS, although some JavaScript functions needed to be rewritten to work with the more flexible layout possibilities.)
With spread modes, two-page spreads can be joined side-by-side, starting on either odd- or even-numbered pages.
Here is a visual tour of the new features!
The default view:
Horizontal scrolling:
And wrapped scrolling:
Wrapped scrolling is adaptive to the size of the pages and the window:
Wrapped scrolling is good for fitting more pages on your screen at once, but if what you really want is to be able to see printable materials in the two-up view presented by other PDF readers, you want to enable spreads:
Oops! In that view, the page numbers are on the insides of the spreads. That must mean that this document's spreads begin on even-numbered pages:
That's better!
Unlike wrapped scrolling, spreads make pages stick together at any zoom level:
But you can still use spreads and wrapped scrolling together! Here's the document with odd spreads:
And here it is with the correct spread mode for this document, even spreads:
Notice how you still fit the same number of pages on the screen, but choosing the right spread mode prevents spreads from being broken.
Finally, because the layout is being handled by CSS and the browser's layout engine, unusually sized pages are fully supported by all of these options:
Now try it yourself!
Closes #590, closes #2545, opens the door to your heart.
P.S. Here is a list of suggestions compiled from comments and reviews for further improving these features (in later PRs, possibly by other people)
Set default scroll and spread modes as preferences(Added to this PR)Rewrite nearby(TheclassList.add
/.remove
calls to be consistent with addedclassList.toggle
code.toggle
calls were backed out in Fix a number of regressions/inefficiencies introduced by adding Scroll/Spread modes to the viewer (PR 9208 follow-up) #9832.)