Conversion of NCX toc navPoints to <ol> produces different structure

[chine]

I would appear as though addNavPointElements() in package_document.js is producing output that doesn't match the structure of the original navPoints.

For example:

<navPoint>
    <navLabel></navLabel>
    <content></content>
    <navPoint></navPoint>
    <navPoint></navPoint>
    ...etc.
</navPoint>

becomes:

<li>
    <a></a>
</li>
<li>
    <ol>
        <li></li>
        <li></li>
        ...etc.
    </ol>
</li>

Unless there's something of which I'm not aware, shouldn't it instead be:

<li>
    <a></a>
    <ol>
        <li></li>
        <li></li>
        ...etc.
    </ol>
</li>

The offender appears to be here

I'd be happy to submit a pull request if this is a desired change.

[danielweck]

Thanks for reporting this! (most of my test ebooks are EPUB3 with HTML5 NavDoc, not NCX)
Would you mind submitting a pull request, please?

[chine]

Alright, got there, finally.

I also noticed that this line in readium-viewer-js causes the viewer not to consume the resulting dom in a nice way. The last condition (|| $(dom).html()) is dropping the outer <ol> and sending an orphaned list of <il>s

We'd cribbed that portion of the viewer and were still getting an undesirably rendered toc. Also a bug? (don't want to presume it isn't intended for some reason of which I'm not aware.)

[danielweck]

Thank you for the pull request @chine https://github.com/readium/readium-js/pull/98/files

Based on your last comment #97 (comment) is looks like your proposed bugfix in readium-js would still require further investigation in readium-js-viewer, before we can claim that this TOC issue is completely solved. Right?

Do you have any EPUB2 (NCX) documents to share? (so we can add them to test for regression bugs)

[chine]

No problem.

Yes, if you consider both working properly in tandem to be 'completely fixed'. As it stands the API of readium-js would appear to be that, for NCX/EPUB2 TOCs, the result if of the form <ol><li></li>...</ol> which the viewer consumes by .html()-ing and throwing an un-wrapped list of list items right to the UI. So, assuming readium-js is in the right to return the result in that form, then the viewer ought not .html() it. At least, that's my assessment based on us having based our code which consumes the TOC on what we saw in the viewer.

Unfortunately the (single) book we have which caused us to notice this is provided by our partners and isn't ours to share. All say what it is though: The Essentials of Computer Organization and Architecture (4th Ed.) from Jones & Bartlett Learning; ISBN 978-1-284-04561-1