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

question about html root element #14

Open
halmos opened this issue Oct 27, 2021 · 5 comments
Open

question about html root element #14

halmos opened this issue Oct 27, 2021 · 5 comments

Comments

@halmos
Copy link
Contributor

halmos commented Oct 27, 2021

This may be somewhat related to issue #10 .

The epub cfi spec does not indicate what the root element of the html portion of the cfi should be. However, in all the examples, the body element is assigned the cfi step reference number of 4.

This would seem to indicate that root html element is not included in the step reference.

That also agrees with your tests found in https://github.com/fread-ink/epub-cfi-resolver/blob/master/tests/simple.js#L120-L251.

In those tests, all the html step references begin with 4/.

The issue is that the cfi generator code behaves differently. It includes the root html element in the step reference and adds the leading 2: 2/4...

For example in the generator.js test you can see that the test string includes the leading 2:

var cfiStr = 'epubcfi(/2/4[body01]/10[para05]/3:5)';

I believe the correct behavior would be for the generator to stop traversing the DOM tree when it reaches the body elements.

If you agree, I would be happy to make a pull request.

@Juul
Copy link
Member

Juul commented Oct 27, 2021

Yes it looks like you are correct. Combining this with issue #10 it seems that maybe the correct behavior is to always ignore the root element, whether it's a html element or a package element or something else. Does that seem right to you? If so then perhaps we can replace your fix for issue #10 with a generic "skip the root element" fix?

@halmos
Copy link
Contributor Author

halmos commented Oct 28, 2021

Yes, that makes sense to me. Would you like me to work on that and make a merge request?

@Juul
Copy link
Member

Juul commented Oct 29, 2021

If you have the spoons that would be great. I'm traveling and might not be very responsive for the next few days.

@halmos
Copy link
Contributor Author

halmos commented Nov 1, 2021

No problem

@halmos
Copy link
Contributor Author

halmos commented Nov 1, 2021

I made the pull request: #15

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

2 participants