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

RTL documents that are reflowable lay out the pages in the wrong order #181

Closed
rkwright opened this issue Aug 27, 2014 · 16 comments
Closed
Assignees

Comments

@rkwright
Copy link
Member

If a RTL document is reflowable and is displayed in two-up layout, the pages are rendered in the wrong order, i.e. page one will be on the left and page two on the right, which is backwards.
A simple test file is here:
https://readiumfoundation.box.com/s/a2cf4jlr17zbl001ma5n
Note that the first page has some explanatory text in English. The is page one and it should be on the right of a two-up spread.

@rkwright rkwright added this to the v1 milestone Aug 27, 2014
@dmitrym0
Copy link

I checked out f49d5aa which was head prior to the #180 merge, and I'm seeing the same behavior. On the last page, the text "This document contains" is in the right hand column, whereas iBooks renders it in the left hand column. This is not a regression due to #180.

@rkwright
Copy link
Member Author

Thank you, Dmitry, for checking that out.

ryanackley added a commit that referenced this issue Aug 28, 2014
@ryanackley
Copy link
Contributor

Fixed by this commit in readium-shared-js

@danielweck
Copy link
Member

@ryanackley regression: the Arabic pages from Ric's EPUB do not turn anymore (they shift in the wrong direction so the viewport is blank). What ebook did you use to test your changes? (maybe it is EPUB-dependent)

@danielweck danielweck reopened this Aug 31, 2014
danielweck added a commit to readium/readium-shared-js that referenced this issue Aug 31, 2014
(missing explicit content direction when page progression direction is explicit to RTL)
@danielweck
Copy link
Member

Here is a working fix tested with Nav-Button-RTL (Ric's Arabic sample ebook with missing content direction and explicit page-progression-direction), regime-anticancer-arabic-20130327 and kusamakura-japanese-vertical-writing-20121124 (from the Google Code EPUB3 samples), tiny3-J (Ric's sample)

readium/readium-shared-js@a5ec2ad

Let me know if it now works. Thanks!

@danielweck
Copy link
Member

Follow-up (sorry, I didn't have time to finish this last night):

Although my workaround addresses Ric's bug and fixes a category of edge-cases (i.e. probably a good number of "real world" EPUB3 publications), not all possibilities are covered (i.e. all possible combinations of html@dir, CSS#direction, CSS#writing-mode, and spine@page-progression-direction). I suggest that ; just as we did for rendition-layout and page-spread ; we derive non-ambiguous behaviours from the specification(s) using a "decision grid / matrix".

I logged a new issue in readium-shared-js (the present one is in readium-js-viewer):

readium/readium-shared-js#102

@ryanackley
Copy link
Contributor

@rkwright and I tested this change with his book and I tested with other RTL documents and the pages turned fine. Are you sure you had everything up to date when you tested this? The behavior your describing was happening in an earlier version (chrome web store) independent of this change.

My fix addressed your last comment. Not sure why you just threw it all away and took a very naive approach.

@danielweck
Copy link
Member

@ryanackley yes, I had a fresh build (I will try again though). Have you looked at the new issue (the shared Google Doc)?

@danielweck
Copy link
Member

@ryanackley tested again with Ric's Nav-Button-RTL.epub and with your code => first two-page spread okay, next pages are blank. Can you please verify on your end?

@ryanackley
Copy link
Contributor

Retested on my end and it works fine. Can you go to chrome://extensions and reload your extension. Then send me a screenshot of your about dialog since it now contains the commit hash of each project you're running.

@ryanackley
Copy link
Contributor

Here is mine
image

@danielweck
Copy link
Member

@ryanackley Ah! There is yet another WebKit discrepancy between Safari and Chrome (handling of writing mode values, I think). Your code works only in Chrome, mine in both. Note that both our solutions are "naive" (and therefore only temporary workarounds), please refer to the new issue and shared Google doc.

@ryanackley
Copy link
Contributor

@danielweck next time can you please assume it was tested by someone and then discuss when everyone is back online? It's a holiday here but I feel like I am forced to respond when you back out one of my changes and question its validity. It's a little stressful. I know you meant no harm and this is why I'm pointing it out to you.

I disagree about my solution being naive. My change would have taken into account css rules for direction. We could have added to it for writing-mode. It's most likely broken in Safari because of the computedStyle not being calculated in time which is probably fixable.

@danielweck
Copy link
Member

@ryanackley I think that for milestone v1 we are better off with a solution that also works in the cloud reader, otherwise it is going to create a lot of confusion amongst those who do not use/develop the Chrome extension. Thus why I deemed appropriate to push a "fix" that works across the board (and tested against Japanese books too, not just Arabic).
I logged a separate issue in milestone v1+ to address this problem properly, as it is not as trivial as adding support for CSS writing-mode in your code: there are other critical points in the reflowable_view code where CSS "columnisation" depends on the combination of both writing-mode and content direction, and we also need to update the paginationInfo appropriately to reflect the overrides of spine-level page progression direction (this has a knock-on effect on cgi navigation logic, canGoLeft/Right, etc.). Thus why I consider both our implementations "naive".

@danielweck
Copy link
Member

Closing this issue now, but please bare in mind that Readium will need a proper fix for this (including changes in several key JS classes), see:

readium/readium-shared-js#102

@danielweck
Copy link
Member

@ryanackley I apologize for overwriting your work without prior consultation. I restored your original code and entered a new issue in shared-js:

readium/readium-shared-js#106

I made a mess, sorry for polluting the history (in chronological order):

readium/readium-shared-js@e41a3cc
readium/readium-shared-js@a5ec2ad
readium/readium-shared-js@65cf5f8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants