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.

Sign up for GitHub

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

Jump to bottom

CFI range generation with start and end at different nodes level #28

Closed
ClementHard opened this issue Feb 3, 2015 · 3 comments
Closed

CFI range generation with start and end at different nodes level #28

ClementHard opened this issue Feb 3, 2015 · 3 comments

Comments

@ClementHard
Copy link
Contributor

The CFI generated for a selection starting and ending at different node level are incorrect.

For the following document :

<html>
    <body>
        <h1>Title</h1>
        <p>Some <strong>very important</strong> text</p>
    </body>
</html>

The CFI for "important text" should be : /2/4, /2/1:5, /3:5
But the actual CFI generated is : /2/4, /2/1:5, /4/3:5

The CFI for "Some very" should be : /2/4, /1:0, /2/1:4
But the actual CFI generated is : /2/4, /4/1:0, /2/1:4

The extra step come from the parent path.

Spec : http://www.idpf.org/epub/linking/cfi/epub-cfi.html#sec-ranges
@danielweck
Copy link
Member

Thank you for spotting this bug. Would you mind contributing a unit test?
https://github.com/readium/readium-cfi-js/blob/develop/spec/javascripts/models/cfi_generation_spec.js

By the way @Twibit , I assume you are @chardouin as well? :)
Thank you for your helpful contributions!

@ClementHard ClementHard mentioned this issue Feb 3, 2015
Merged
@ClementHard
Copy link
Contributor Author

Here it is !
@danielweck Yes, i'm @chardouin as well. I should get rid of one of my account :)

dmitrym0 pushed a commit to dmitrym0/readium-cfi-js that referenced this issue Feb 19, 2015
[readium-cfi-js readium#28] Don''t include the parent step in a range…
bbccc14 
… CFI if a node is a direct child.
dmitrym0 pushed a commit to dmitrym0/readium-cfi-js that referenced this issue Feb 19, 2015
Add an additional unit test from readium#28
d9a111d
@dmitrym0 dmitrym0 mentioned this issue Feb 19, 2015
Merged
@jccr
Copy link
Member

jccr commented Apr 20, 2015

A fix has been merged in.

@jccr jccr closed this as completed Apr 20, 2015
matwood pushed a commit to bibliolabs/readium-cfi-js that referenced this issue Aug 12, 2015
@ClementHard @matwood
Add a unit test to check CFI range generation when start and end node…
5c295d2 
… don't have the same level

Related to readium#28
matwood pushed a commit to bibliolabs/readium-cfi-js that referenced this issue Aug 12, 2015
@matwood
[readium-cfi-js readium#28] Don''t include the parent step in a range…
d02dfae 
… CFI if a node is a direct child.
matwood pushed a commit to bibliolabs/readium-cfi-js that referenced this issue Aug 12, 2015
@matwood
Add an additional unit test from readium#28
5cf9432
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@danielweck @ClementHard @jccr