readium-js icon indicating copy to clipboard operation
readium-js copied to clipboard

fix for relative mathJaxUrl

Open pmstss opened this issue 10 years ago • 5 comments

mathJaxUrl is described in API.md as "relative url for the MathJax javascript file" but in viewer and other examples absolute url is used. The bad thing is that if relative url is used - it doesn't work correctly. Reason is that the <base> is set for iframe content and it points to epub root, but mathjax url is relative to app, not to epub. Proposed fix "absolutize" mathJaxUrl (using main window location) before usage in iframe script element.

Short URI.js syntax, discussed in readium-shared-js issue 226, is used :)

pmstss avatar Oct 23 '15 13:10 pmstss

Thanks @pmstss In the current build / configuration pattern, the MathJax URL is created absolute on the client side of the Readium API, via build options.

At development time: https://github.com/readium/readium-js-viewer/blob/master/dev/RequireJS_config.js#L49 (may be using single, multiple RequireJS bundle, or no-optimize source tree)

Chrome app distribution: https://github.com/readium/readium-js-viewer/blob/master/src/chrome-app/requirejs-config.js#L37

Cloud reader distribution: https://github.com/readium/readium-js-viewer/blob/master/src/cloud-reader/index.html#L87

The option data is passed via the new Readium() constructor: https://github.com/readium/readium-js-viewer/blob/master/src/js/EpubReader.js#L696

..but note that the Chrome app transforms content in its own way (as the EPUB files are exploded in the HTML5 filesystem): https://github.com/readium/readium-js-viewer/blob/master/src/js/workers/ContentTransformer.js#L65

So to summarize, I am not sure this PR is needed. You should be able to absolutize the URL using window.location on the client side of the Readium API, right?

danielweck avatar Oct 23 '15 13:10 danielweck

I'm using readium-js in custom viewer, not with readium-js-viewer. Sure I can pass absolute mathJaxUrl to Readium constructor, but then mathJaxUrl description in API.md should be fixed from "relative url" to "absolute url"?

..but note that the Chrome app transforms content in its own way

Important remark, thanks. But that ContentTransformer.js is part of readium-js-viewer: I think the same changes are required there to match API description. Given readium-js PR will not break anything. If we will support both relative and absolute urls in mathJaxUrl - why not?

pmstss avatar Oct 23 '15 14:10 pmstss

Right now the MathJax lib ships with the application, and we pass an absolute URL to ensure its location is explicitly known, not obfuscated by an API layer. Imagine that the MathJax.js URL is passed as a relative reference (e.g. /MathJax.js, ./MathJax.js, ../MathJax.js, ../libs/MathJax.js, etc.), the webview will automatically make it absolute via the iframe's base href (which is the absolute URL of the loaded spine item document). Hypothetically, this may actually be a desirable behaviour, assuming for example that content gets deployed to a server in such a way that external "runtime" libraries (e.g. rendering helpers, annotation tools) are copied across into the location of exploded EPUBs (or at least under the same domain), whilst the app lives on another domain. If we hard-code the window.location deep inside the Readium API (forcing the MathJax relative URL to resolve against it, instead of the iframe's base), it breaks the potential use-case (removes the flexibility).

danielweck avatar Oct 23 '15 15:10 danielweck

Described use case is for sure possible, and you are right, such kind of flexibility will be lost with this fix. But definitely it is not what comes to mind first when playing with readium options and see "relative url for the MathJax javascript file".

I think at least explicit mentioning, that mathJaxUrl is expected to be either relative to each spine item document or to be absolute (as it is now in all readium usages), is required in API.md. Second is also important, because current description could be treated as prevention from using absolute url there, which is not true.

pmstss avatar Oct 23 '15 15:10 pmstss

Defer this PR, but we do need to update the documentation.

rkwright avatar Nov 20 '15 17:11 rkwright