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

fix for relative mathJaxUrl #121

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

pmstss
Copy link

@pmstss pmstss commented Oct 23, 2015

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 :)

@danielweck
Copy link
Member

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?

@pmstss
Copy link
Author

pmstss commented Oct 23, 2015

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?

@danielweck
Copy link
Member

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).

@pmstss
Copy link
Author

pmstss commented Oct 23, 2015

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 added a commit to pmstss/readium-js that referenced this pull request Oct 23, 2015
… baseHref issue"

This reverts commit 718b96f.

As discussed in readium#121, relativity
to spine item document is expected feature. Converting url to absolute is
more safe to be done earlier, before passing mathJaxUrl to readium.
@rkwright
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants