openSpineItemElementCfi/openSpineItemElementId opens wrong page if html is already open

[nandita121189]

This issue is coming only in scroll auto mode (reflowable view)

The issue seems to be in function getPageForElementCfi() (cfi_navigation_logic.js) that returns wrong pageIndex. This function internally calls function findPageByRectangles(). The value of "left" in clientRectangles variable comes wrong when html that is already open and user is not on first page.

[nandita121189]

Hello @jccr,

I noticed that in getVisibleContentOffsets() in cfi_navigation_logic.js, the following code was removed in one of the commits 99640b8

 return {
            left: (options.paginationInfo ? options.paginationInfo.pageOffset : 0)
                * (isPageProgressionRightToLeft() ? -1 : 1)
        };

If I replace the last return statement

return {
            top: 0,
            left: 0
        };

in the current function getVisibleContentOffsets(), with the code removed in the above commit

 return {
            left: (options.paginationInfo ? options.paginationInfo.pageOffset : 0)
                * (isPageProgressionRightToLeft() ? -1 : 1)
        };

, then the issue seems to be resolved, but I do not understand why that code was removed in the first place and what will be the regression if I apply the fix I suggested. Could you please guide me with it and make changes in the library itself so that it is resolved for future users as well.

[jccr]

Acknowledged. Thanks for the report.

…

On Wed, Apr 5, 2017 at 11:33 PM nandita121189 ***@***.***> wrote: Hello @jccr <https://github.com/JCCR>, I noticed that in getVisibleContentOffsets() in cfi_navigation_logic.js, the following code was removed in one of the commits 99640b8 <99640b8> return { left: (options.paginationInfo ? options.paginationInfo.pageOffset : 0) * (isPageProgressionRightToLeft() ? -1 : 1) }; If I replace the last return statement return { top: 0, left: 0 }; in the current function getVisibleContentOffsets(), with the code removed in the above commit return { left: (options.paginationInfo ? options.paginationInfo.pageOffset : 0) * (isPageProgressionRightToLeft() ? -1 : 1) }; , then the issue seems to be resolved, but I do not understand why that code was removed in the first place and what will be the regression if I apply the fix I suggested. Could you please guide me with it and make changes in the library itself so that it is resolved for future users as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#384 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE5RbLOG_-gmpg0CkTKcvBewM4JfGF6kks5rtIdOgaJpZM4Mz1GZ> .

[danielweck]

Similar? readium/readium-js-viewer#404

[danielweck]

Related? #261

[nandita121189]

I am not sure about readium/readium-js-viewer#404, but #261 is definitely the same issue as openSpineItemElementCfi() internally calls Reflowable_view.openPage() function.

[danielweck]

Fixed the regression bug ( culprit: 99640b8 ) via: 9322c0a
Please let us know how it goes in your tests.

[nandita121189]

Hi @danielweck ,

I just checked your solution on develop branch, although the bug is fixed now but it has a huge regression issue. If you notice the localstorage(of browser) you will see that the object storing last visited location of book i.e.idref and contentCFI has null contentCFI.
Basically , readium.reader.bookmarkCurrentPage() is the function that has been affected by your above mentioned latest commit.

If I revert only the changes done in above mentioned commit then it behaves well as before.

Can you please verify the same at your end and re open this issue if this problem exists.

Thanks

[danielweck]

Indeed!!

[danielweck]

Thank you very much for bringing this up @nandita121189
I fixed the previous fix using a hacky fix: 701e590

@jccr any idea why "CFI navigation logic" getVisibleContentOffsets() works with some consumer code, but not some other? I temporarily fixed the pageIndex issue by locally patching the usage of getVisibleContentOffsets(), otherwise the change negatively affects other usages (such as bookmarking / "get visible element" functions).

[danielweck]

Related: readium/readium-js-viewer#616

[nandita121189]

Hello @danielweck ,
Is the hacky fix 701e590 applied by you supposed to fix both the original issue and the regression issue ?
Coz I am still getting the above mentioned regression issue !

[danielweck]

No regression bug in the TravisCI-deployed build (from the develop branch):
https://readium.firebaseapp.com/?epub=epub_content%2Faccessible_epub_3&goto=%7B%22idref%22%3A%22id-id2635343%22%2C%22elementCfi%22%3A%22%2F4%2F2%5Bbuilding_a_better_epub%5D%2F10%2F28%2F14%2C%2F1%3A267%2C%2F1%3A268%22%7D
(above link should reference the a character of accessibility word, just before the sentence And if you really don’t know and can’t find an answer, ask.)

[danielweck]

PS: tested on Chrome and Firefox on OSX (Safari kinda works, but has some inconsistencies when refreshing the page).

[jccr]

Sorry about my lack of response @nandita121189 @danielweck
I finally have a chance to look into this

[nandita121189]

@danielweck Yes I just checked again it works for me too. As of now I didn't find the regression issues.

[danielweck]

@nandita121189 Good to know, thanks!

Thank you @jccr for finding time to take a look at this issue.

[nandita121189]

Hello @danielweck ,

I just checked and noticed that although openSpineItemElementId is working fine this issue seems to be partially fixed as openSpineItemElementCfi function is still not working correctly.

Could you please check and reopen the issue.

Thanks.

[sercanl]

Hi,
Is this fix available on version 0.28?

[danielweck]

YEs