search

readium / readium-sdk

A C++ ePub renderer SDK
BSD 3-Clause "New" or "Revised" License
383 stars 164 forks source link

Spine items rejecting property attributes #318

Closed ghost closed 10 months ago

ghost commented 5 years ago

Related to: 9a9c8d84 where the 'rendition' prefix was disabled The XML parsing runs fine, extracting the property string, but when creating the IRI, there's a whitelist for acceptable IRI prefixes it seems and the default has 'rendition' commented out. The proposed solution is to specifically enable this for SpineItem since it seems there are previous issues with enabling it in the default whitelist.

danielweck commented 5 years ago

Thank you for the PR. I am trying to understand what problem this solves. Could you please provide an EPUB that exhibits a bug?

It has been a very long time since the commit you referenced, but ... ... if I remember correctly, the rendition prefix could not be included in the ReservedVocabularies (all GitHub permalinks reference the develop branch):

https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/property_holder.cpp#L38 https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/property_holder.cpp#L73

... because of the InstallPrefixesFromAttributeValue() function potentially picking-up the rendition URI specified via the prefix attribute of the package OPF element:

https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/package.cpp#L681 ==> https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/package.cpp#L1183-L1215

... which invokes RegisterPrefixIRIStem() (which in turn potentially registers the rendition URI):

https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/property_holder.cpp#L306-L313

There are indeed direct calls to RegisterPrefixIRIStem() (special built-in cases) in order to force the rendition URI registration (when not explicit in package@prefix), so that MakePropertyIRI() can correctly handle the registered _vocabularyLookup:

https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/package.cpp#L443 https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/package.cpp#L460 ==> https://github.com/readium/readium-sdk/blob/a32d900ce337d9ca46dbb029a92f581184ee224c/ePub3/ePub/property_holder.cpp#L314-L326

So, it seems you are trying to solve / have solved a similar issue for SpineItem:

https://github.com/kyles-ep/readium-sdk/blob/fac65be973beb86e179a7b9ab464150ce71dcece/ePub3/ePub/spine.cpp#L39-L42 ( from your forked master branch https://github.com/kyles-ep/readium-sdk/blob/master/ePub3/ePub/spine.cpp )

Can you please explain why SpineItem in particular? Do you think the problem exists somewhere else? (bearing in mind, I am not sure what the "problem" is exactly :)

Thank you for clarifying, and sorry if I am missing an obvious point (it has been a long time since I looked at this C++ codebase)

PS: https://idpf.github.io/epub-vocabs/rendition/#sec-ref

danielweck commented 5 years ago

PS: is @evidentpoint using / planning to use a modified version of readium-sdk, with bug fixes, improvements, API changes, etc.?

ghost commented 5 years ago

Apologies for not elaborating on the problem. I don't know if I can share the EPUB itself, so I'll provide the relevant bits (please let me know if it's sufficient).

The problem is that this document has spine items with "rendition" properties:

<itemref idref="asdf-1" properties="rendition:layout-pre-paginated rendition:spread-landscape rendition:page-spread-center"/>

and the package metadata element has the following children: reflowable auto

From what I understand, spine items are allowed to override the rendition properties specified in the package metadata, and as it is, there's a bug that prevents this property from being picked up.
I admit I'm not the most familiar with EPUB, so perhaps there are other places where such a property needs to be picked up?

jccr commented 5 years ago

PS: is @evidentpoint using / planning to use a modified version of readium-sdk, with bug fixes, improvements, API changes, etc.?

A request came our way that asked us to look into fixing a bug with the readium-sdk. My colleague Kyle is investigating. Evident Point doesn't have plans that go beyond helping out with maintenance and bug fixing.

ghost commented 5 years ago

Sorry. I thought I could change both source and destination of the PR. @jccr Suggests this should be merged into develop instead of master. Perhaps we should close this and open a new one with proper targeting?

danielweck commented 5 years ago

I took the liberty to change the PR target branch from master to develop.

danielweck commented 5 years ago

You explanation makes sense Kyle, thanks. I am surprised this bug hasn't surfaced before. <itemref properties="rendition:xxx" ... /> is not an uncommon construct in the EPUB test books typically used to validate Readium implementations.

Wouldn't it make more sense to invoke this->RegisterPrefixIRIStem("rendition", "http://www.idpf.org/vocab/rendition/#"); immediately after InstallPrefixesFromAttributeValue() is called? (see my full description above) This way, if the OPF prefix definition already contains "rendition", then RegisterPrefixIRIStem() simply overrides it with the default URI defined in the EPUB3 specification ( https://idpf.github.io/epub-vocabs/rendition/#sec-ref ). What do you think? Could you please give this a try, by also removing the two other instances in readium-sdk/ePub3/ePub/package.cpp where this->RegisterPrefixIRIStem("rendition", "http://www.idpf.org/vocab/rendition/#"); is invoked directly (again, details in my comment above). Thanks!