readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
550 stars 186 forks source link

All Highlights are on the first page #645

Closed ar2code closed 7 years ago

ar2code commented 7 years ago

This issue is a Bug

Hello! Thank you for such great library, but I think I have an issue with highlights.

When we have EPUB with several virtual pages (in one html), highlights are correct redered only on a first page. When we go to next virtual page and select text, highlight is not rendered. Than we go back to first virtual page and see rendered highlight in a wrong place.

I attached screenshoots: First: select and highlight on a first page - OK

h1

Second: select and NOT highlight on a second page - WRONG

h2

Thired: go back to the first page and see wrong highlights - WRONG

h3

Product

Readium JS viewer with highlight plugin.

Maybe I do something wrong... Can you help me please? Thank you!

danielweck commented 7 years ago

Hello, thank you for your feedback! What revision / branch are you using? What web browser?

ar2code commented 7 years ago

Hello, Daniel Thank you for quick answer! I'm using the last revision, master branch. Labels: 0.27.0. Your last commit is [921cd45] from 12/06/2017. The same behaviour for Chrome, IE and all mobile browsers. Chrome version 60.0.3112.113 (64-bit).

ar2code commented 7 years ago

I figured out what has happened.

When highlight renders it is got wrong pageOffset.

In highlight controller I found this line:

offset = this.context.paginationInfo.pageOffset;

And offset is undefined.

But when context is created I found a function but not a property. Highlight manager.js -> this.attachAnnotations = function

paginationInfo: function() { return spineItem.paginationInfo; }

I replaced it with:

paginationInfo: spineItem.paginationInfo

Now it is OK. But maybe I don't understand the whole thing. Maybe you will check it. :) Thank you!

danielweck commented 7 years ago

Hello @JCCR this looks like a regression bug from: https://github.com/readium/readium-shared-js/commit/9967855c46802672bedd57d3c12056683705075c#diff-6d1424ffdf1e073d51d72f6fc7de7464R556

(note that all other usage of paginationInfo in the source tree is correct, i.e. as a property, not a getter function)

https://github.com/readium/readium-shared-js/blob/05be48df7021bda38147ba38e9855c2ab2965ebe/plugins/highlights/controller.js#L557-L561

https://github.com/readium/readium-shared-js/blob/05be48df7021bda38147ba38e9855c2ab2965ebe/plugins/highlights/manager.js#L117

danielweck commented 7 years ago

Fixed via https://github.com/readium/readium-shared-js/commit/efa5ea5ed4e848b761774172ce6cd95cb2d82d82

Thank you very much @AlexeyRozhkov !!