fix: styles and scripts not injected into epub navigator#749
fix: styles and scripts not injected into epub navigator#749m-abs wants to merge 15 commits intoreadium:developfrom
Conversation
readium/navigator/src/main/java/org/readium/r2/navigator/epub/WebViewServer.kt
Outdated
Show resolved
Hide resolved
|
As usual, I've changed my mind on the way, this might roughly be the way to go. I'd like to clarify the concepts we're working with and check them against both the expectations and the implementation, but this is a headache. I'll get back to it later. |
|
I think I've got my ideas clear.
Do you share my analysis @mickael-menu? |
|
@qnga I think your approach is sound and seems like the way to go. I like the idea of separating the host for the publication container from the one for the Readium static assets. |
|
@m-abs You know what to do if you want to proceed?
I reckon the minimum would be to solve the issue I pointed above and prevent injection in Webpubs. |
I'm not entirely sure how to do all that, but I'm willing to try as this is very important for us at Nota. |
|
Actually I was too quick regarding relative URLs in non-packaged publications. We can resolve them to base URLs regardless of the publication profile. That's why I suggested the |
…ed resources from remote host
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where styles and scripts were not being injected into the EPUB navigator for streaming publications (EPUBs served over HTTP). The problem was that the WebViewServer was only checking for the hardcoded https://readium/ hostname, which didn't match streaming EPUBs that use different base URLs.
Changes:
- Added
hostproperty toAbsoluteUrlfor extracting hostname from URLs - Updated
WebViewServerto use hostname-based routing with distinct hostnames for publications (readium_package) and assets (readium_assets) - Added fallback logic to handle streaming publications with arbitrary hostnames by resolving URLs against the publication's base URL
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| readium/shared/src/main/java/org/readium/r2/shared/util/Url.kt | Added host property to AbsoluteUrl class to enable hostname extraction |
| readium/shared/src/main/java/org/readium/r2/shared/publication/Publication.kt | Added imports for URL conversion utilities (unused) |
| readium/navigator/src/main/java/org/readium/r2/navigator/epub/WebViewServer.kt | Refactored request handling to support hostname-based routing and added fallback for streaming publications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import org.readium.r2.shared.util.toUri | ||
| import org.readium.r2.shared.util.toUrl |
There was a problem hiding this comment.
These imports are unused in this file. The toUrl and toUri functions are not referenced anywhere in Publication.kt.
| import org.readium.r2.shared.util.toUri | |
| import org.readium.r2.shared.util.toUrl |
readium/navigator/src/main/java/org/readium/r2/navigator/epub/WebViewServer.kt
Show resolved
Hide resolved
| ?: return null | ||
|
|
||
| servePublicationResource( | ||
| return servePublicationResource( |
There was a problem hiding this comment.
Redundant return statement. The when expression already returns the result, so the explicit return on line 74 is unnecessary. This should just be the value expression without the return keyword.
| return servePublicationResource( | |
| servePublicationResource( |
| else -> { | ||
| // Request is for streaming a resource, if the baseUrl an AbsoluteUrl and hostname | ||
| // is not ASSETS_HOSTNAME or READIUM_PACKAGE_HOSTNAME | ||
| val baseUrl = publication.baseUrl as? AbsoluteUrl ?: return null |
There was a problem hiding this comment.
@qnga I'm unsure, if I should return null here and let the platform handle the request or if it would be better to return an errorResource() or still call servePublicationResource(...)?
There was a problem hiding this comment.
If there is a baseUrl it must be absolute (I wonder why it's not in the signature, we should check). So we're dealing here with the case when there is no baseUrl. Each resource either comes from a package or has got an absolute URL as href. You've already dealt with the package case above, so I guess you should deal with absolute hrefs here.
Shortly, if the request URL is equivalent to some resource href (and the profile is EPUB), then serve the resource from the publication with injection. If it is not, serve an error resource.
There was a problem hiding this comment.
If there is a baseUrl it must be absolute (I wonder why it's not in the signature, we should check).
The baseUrl is aRelativeUrlwhen it is a packaged publication and anAbsoluteUrlwhen it is a streamed publication, which was why I added that constraint.
But I think I need to remove it again, because a packaged publication might still use an external resource. Unless we want to prevent that.
I think in our (Nota's) case, this would only happen with media-overlay/guided-navigation books which isn't affected by this.
If it is not, serve an error resource.
I'm not so sure about this.
Could there be a case where the resource url is not listed in any of the links in the publication? For instance if an image is used in the HTML but not listed in resources.
About the profile being EPUB , currently the epub/WebViewServer doesn't check for it but I'll add it.
There was a problem hiding this comment.
A packaged publication should have no baseUrl because it should have no self link. I checked with Mickaël, you can do an optional cast in Publication.baseUrl to get an absolute URL or nothing. That's already like this in the Swift toolkit.
Regarding external resources, I believe they should be served from the publication, because they are part of it. An HTTP client should probably be added to the container chain when it is built if some resources are remote.
Could there be a case where the resource url is not listed in any of the links in the publication?
In theory that's impossible, it's the point of having a manifest listing the resources. In practice, if you use publication.get you'll get a resource if the href is matching something in the package (not the manifest) and null if it's not.
There was a problem hiding this comment.
In practice, if you use publication.get you'll get a resource if the href is matching something in the package (not the manifest) and null if it's not.
We might need to double check this in depth but if it is not the case I think that's the soundest approach anyway. I remember me doing something similar in a different case.
There was a problem hiding this comment.
I got baseUrl that were a RelativeUrl in my packaged, but might be because our converter always adds a self-link to our RWPM.
According to this a self-link with an absolute URL is required:
https://readium.org/webpub-manifest/#23-links:~:text=Test%20Publication%22%0A%7D-,2.3.%20Links,is%20an%20absolute%20URI%20to%20the%20canonical%20location%20of%20the%20manifest.,-Example%203%3A%20Link
If it can be left our in packaged publications, I think the documentation should be updated :) and we (Nota) should fix our self link to be absolute for streaming.
There was a problem hiding this comment.
See readium/webpub-manifest#119
We decided this a while back but the spec often lags behind.
There was a problem hiding this comment.
I have changed this back to checking for baseUrl being an AbsoluteUrl.
Is there anything more I need to do? We would really like to be able to stream publications :)
|
There is another I'm not sure if that one can be used for streaming too, I don't see how to open one via the |
|
Yes, we should change the server in the new navigator too. I can replicate your changes by myself though, if it makes your work easier. |
I don't mind making the changes there too, when we have agreed on the changes in the |
|
|
||
| val href = | ||
| // Look up the link in the publication to make sure we have the right resource. | ||
| publicationLinkFromHref(baseUrl.resolve(requestUrl))?.href?.resolve() |
There was a problem hiding this comment.
Why do you resolve requestUrl? I don't think it can be relative.
| val baseUrl = publication.baseUrl as? AbsoluteUrl ?: run { | ||
| val error = ReadError.Decoding( | ||
| "baseUrl is not an AbsoluteUrl, cannot load remote resource from $requestUrl" | ||
| ) | ||
| onResourceLoadFailed(requestUrl, error) | ||
| return serveErrorResponse() | ||
| } |
There was a problem hiding this comment.
We don't need baseUrl if href is absolute. It could be safer to keep going in that case.
Streaming epub profile didn't inject styles and scripts into the XHTML file because the base url isn't
https://readium/This fixes the issue I had with #734, I'm not sure if it was the same problem with #718.