readium-shared-js
readium-shared-js copied to clipboard
readium
Fix/viewport clipped
Fixes #181 :
Reflowable document viewport gets clipped when enlarged (CSS columns) because the page offset is computed with rounded values.
To fix that, we do not round the column width value, but we round the page offset value instead. The page offset value is used to display the current content.
I had a look at possible regression bugs by hunting down usages of columnWidth (now a number with floating point precision / many decimal places), and as long as CSS is happy with long numbers in px values ; in all web browsers ; then this code change should be alright. Overall, this looks like a relatively safe PR. Note that as I have not been able to reproduce the original problem (inaccurate page offset), I have not actually tested this proposed fix. It does make a lot of sense though.
PS: as Johan said, only the "Files changed" section is relevant, let's not merge the actual commit diff (instead: let's just cherry-pick the code diff).
I don't think that columnWidth is directly used in CSS, but the pageOffset value is. That's why I added the Math.round in the onPaginationChanged function.
I was thinking of: https://github.com/readium/readium-shared-js/blob/develop/js/views/reflowable_view.js#L630
_$epubHtml.css("width", (_htmlBodyIsVerticalWritingMode ? _lastViewPortSize.width : _paginationInfo.columnWidth) + "px");
_$epubHtml.css("column-width", _paginationInfo.columnWidth + "px");
We can add Math.round for those lines or just let browsers deal with decimal pixels.
I just tested my solution on popular browsers and it works fine (Chrome, Firefox, Safari, IE10 & IE11). I should test it on Android and iOS as well to be sure.
That's great @johanpoirier thank you very much for your time on this! :)
I ran some tests on Android and iOS and it works fine.
@danielweck, will this PR be merged?
@johanpoirier we're waiting to hear more about another CSS column-pagination bug (missing reflowable page), so we can see the "big picture". This PR just needs to be updated with git merge develop in the TEA-ebook:fix/viewport-clipped branch (recent AMD-RequireJS refactoring).