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

Add options preview=mathml and preview=altimg to the mml2Jax preprocessor #557

Closed
fred-wang opened this issue Aug 30, 2013 · 39 comments
Closed
Labels
Accepted Issue has been reproduced by MathJax team Feature Request Fixed v2.3
Milestone

Comments

@fred-wang
Copy link
Contributor

By default, the mml2Jax removes the <math> tag and replaces it by the alttext if there is one (preview="altext"). The other known preview is "none". We could add twi other values: preview=mathml (original MathML element) and preview=altimg (altimg tag).

@fred-wang
Copy link
Contributor Author

The behavior is:

none => no preview
mathml => the math tag
alttext (default) => the alttext if there is one and nothing otherwise
altimg => the altimg if there is one and nothing otherwise. (alttext, altimg-width and altimg-height are used to create an img tag with alt, width and height attributes)

I'm wondering if we should change the default to be 'mathml' as it is a better preview for browsers with (even limited) MathML support. Also, the preview for altimg and alttext may be nothing if the attributes are not specified on the math element, which is the case for most MathML content I guess (perhaps they should fallback to the math rather than showing nothing).

I haven't tried to see if the Safe extension can be used to filter altimg.

@ghost ghost assigned fred-wang Aug 30, 2013
@pkra
Copy link
Contributor

pkra commented Aug 30, 2013

I'd be in favor of switching to the default to 'mathml'. Or even better: jqmath (but that's out of scope for v2.3).

@fred-wang
Copy link
Contributor Author

Not sure about how to do jqmath, from my limited experiments I think the work done is significant enough so that we may not want that during the mml2jax parsing (especially if we want to optimize the MathML input steps in the future).

Whether or not 'mathml' becomes the default, what about my comment about falling back to mathml for the altimg/alttext modes when the relevant attributes are missing? Or should we continue to show nothing in these modes?

@fred-wang
Copy link
Contributor Author

OK, I've changed the default to 'mathml'.

=> Ready for review

@dpvc
Copy link
Member

dpvc commented Sep 10, 2013

The idea is a good one, and adding both MathML and altimg previews is great. There is a problem, however, with IE < 9 (or perhaps it is just 8), which is that the math tag being passed to the createPreview function is not the complete tree, but just the <math> tag. That is because IE < 9 didn't handle unknown tags properly, and just made a flat version of the markup, not a proper DOM tree. That is, you got an empty <math> tag followed by an empty <mrow> or whatever, ... followed by an empty </mrow> (yes, a tag with name "/mrow"), followed by an empty </math>. MathJax simply concatenates these into a string that becomes the content of the MathJax <script> tag (this was one reason for the MathML-to-string-back-to-MathML process, because IE didn't have the initial MathML step).

So I guess that means either 1) abandon earlier versions of IE, at least for previews, 2) use DOMParser to parse the MathML string now (at least for IE) when a MathML preview is requested. Thoughts?

@dpvc
Copy link
Member

dpvc commented Sep 10, 2013

Note, this will require an update to the documentation when it is added. (Not sure if that gets added to the milestone or not.)

@fred-wang
Copy link
Contributor Author

Regarding doc: I'm guessing this should be added to #534

I didn't really try with IE but since it does not have MathML support and since MathPlayer is only loaded in the NativeMML output, perhaps we can abandon the mathml previews for IE and just come back to the altext?

@fred-wang
Copy link
Contributor Author

(also when MathPlayer is installed, the preview is not really needed since the rendering will be fast)

@pkra
Copy link
Contributor

pkra commented Sep 10, 2013

Let's open issues on the docs repository and create a milestone there as well.

@dpvc
Copy link
Member

dpvc commented Sep 10, 2013

There is no guarantee that the output will be NativeMML, so the output may be slow. (Not every configuration picks NativeMML for IE, namely TeX-AMS_HTML and TeX-AMS_HTML-full.) The presence of MathML in the page may trigger MathPlayer to load earlier (depending on how the page is set up), but you'r right, it may not happen until the output phase. On the other hand, inserting just the <math> tag (as is currently the case) would need to be altered in any case.

I'm OK with not having MathML previews for IE, but if not having support for MathML is a reason not to use the mathml preview, are we now really talking about a conditional preview that is mathml when native MathML is available and alttext/altimg otherwise? The logic becomes a little more complicated.

I'm wondering if preview might want to be an ordered list of versions to try, like "altimg, alttext, mathml" rather than just one?

@fred-wang
Copy link
Contributor Author

alttext/altimg might not be available either (actually I think that's the case for most MathML content). I think mathml preview is what we want in most cases. That at least give the opportunity to compare MathML support and encourage browser developments, so Gecko/WebKit users could get an acceptable preview (this seems particularly important on mobile devices that are using WebKit) and Chrome/Opera users a bad one (but I hope this will improve). IE is particular, so let's ignore the mathml preview. Also people creating MathML input and intentionally creating alt-content will probably want to configure MathJax themselves. In general I think people creating MathML input are more tolerant regarding the quality of the native MathML, so a mathml preview is best for them.

@dpvc
Copy link
Member

dpvc commented Sep 10, 2013

alttext/altimg might not be available either

Right, which is why I was wondering about an ordered list of preview options, rather than just one.

Also people creating MathML input and intentionally creating alt-content will probably want to configure MathJax themselves.

They may not want to, but they certainly can in order to override the default. In any case, you are correct that that gives them what they need.

Code up what you think it should be. It sounds reasonable.

@fred-wang
Copy link
Contributor Author

Yes, I meant, in general we don't have anything but the math tag and otherwise people intentionally creating alt content do the same for all math and know whether they want to use one alt content or the math tag. So it's not really necessary to create an orderered list in my opinion, they just need to specify the one they want.

@dpvc
Copy link
Member

dpvc commented Sep 10, 2013

OK. That does makes things easier for MathJax.

@dpvc
Copy link
Member

dpvc commented Sep 12, 2013

OK, this takes care of IE, but I'm afraid I found another problem. The preview can no longer be an HTML snippet (as it could originally). That is because you have changed the HTML.Element call to remove preview and use appendChild(preview) instead. This works for the mathml, altimg, and alttext cases (the latter because you have used HTML.TextNode() explicitly), but not for the HTML-snippet case. Because this is one of the documented uses, it will need to be fixed.

Also, I'd move line 192 into the if (preview === "alttext") block, since that is the only place where the alttext variable is used.

@fred-wang
Copy link
Contributor Author

The preview can no longer be an HTML snippet (as it could originally).

argh, I thought I had taken care of this case :( I'll fix that.

@fred-wang
Copy link
Contributor Author

OK, I've commited a change to fix the HTML snippet issue.

Also, I'd move line 192 into the if (preview === "alttext") block, since that is the only place where the alttext variable is used.

altext is use in the img alt text of the altimg preview.

@dpvc
Copy link
Member

dpvc commented Sep 17, 2013

Looks good.

=> Ready for Release

@fred-wang
Copy link
Contributor Author

=> Merged

@fred-wang
Copy link
Contributor Author

preview=mathml seems to double the number of "New Math" signal for one MathML input. See for example

http://devel.mathjax.org/testing/testsuite/API/Hub/messagehook-1.html?&mathJaxPath=http://devel.mathjax.org/mathjax/MathJax-develop/unpacked/&font=STIX&outputJax=HTML-CSS

@fred-wang
Copy link
Contributor Author

Configuration/mml2jax/preview-1b.html

must be updated now that preview=alttext is not the default.

Probably need more tests for the different options.

=> Unit test wanted

@fred-wang
Copy link
Contributor Author

This seems to be the reason why Configuration/MMLorHTML/prefer-2.html fails too:

http://devel.mathjax.org/testing/testsuite/Configuration/MMLorHTML/prefer-2.html?&mathJaxPath=http://devel.mathjax.org/testing/mathjax/MathJax-develop/unpacked/&font=STIX&outputJax=HTML-CSS

the MathML preview is parsed and removed so MathJax-Span-1 no longer exists.

@fred-wang
Copy link
Contributor Author

This also makes the tests for
MathMLToDisplay/Topics/StretchyChars/horizontal.html and MathMLToDisplay/Topics/StretchyChars/vertical.html fail.

@dpvc
Copy link
Member

dpvc commented Oct 9, 2013

It would probably be a good idea to remove id's from the MathML before inserting it into the preview, or perhaps prefix them with something like "MJX-preview-". The only place I can think of where the id's would be needed is in the line breaking where you use breaking at the same place as an element given by an id. But MathJax doesn't yet support that (and I don't think FF does either), so it is not an immediate concern., but may be in the future.

@fred-wang
Copy link
Contributor Author

So before handling duplicate id's, the big problem here is that the <math> preview seems to be sent to the output Jax, while it should not. Not sure why, but perhaps inserting a math element modifies a dynamic getElementByTagName('math') list somewhere. If the preview only exists at the same time as the script tag and if only the script tag is considered by the output Jax, I suspect this would fix all the problems.

@dpvc
Copy link
Member

dpvc commented Oct 10, 2013

I haven't had the chance to look at the double signal issue, but commented on the id issue since I had something to say about that. Your suggestion is a plausible one, as the mml2jax preprocessor uses getElementByTagName('math').

@fred-wang
Copy link
Contributor Author

I'm looking into it right now. Indeed, that seems to happen in the mml2jax preprocessor, the double <script> being created. That's actually slightly more complex because the mml2jax does several calls to getElementByTagName to handle the different cases. I'm trying to create a single array before preprocessing the math elements.

@dpvc
Copy link
Member

dpvc commented Oct 10, 2013

It may also be a good idea to check the class of the parent element and skip any that are the MathJax_Preview class. That way, if a MathML preview is set up and someone calls the preprocessor again before typesetting (when the previews are removed), it will not get picked up.

@fred-wang
Copy link
Contributor Author

OK, I've commited something that seems to fix the issues on Chrome & Firefox. I'll try to do more testing.

@dpvc
Copy link
Member

dpvc commented Oct 10, 2013

Looks good over all. A couple of comments:

The AppendMathElements function could be more efficient as

AppendMathElements: function(mathArray, math) {
  mathArray.push.apply(mathArray,math);
},

This also keeps the order in the array rather than reversing it, so if you want to process the math top down rather than bottom up as it was originally, you could also change the ProcessMathArray function to have var i, m; and use looks like for (i = 0, m = math.length; i < m; i++) {...} instead.

The test for the math preview should use MathJax.Hub.config.preRemoveClass rather than the literal "MathJax_Preivew" since the class nameis configurable.

Finally, shouldn't preview = math.cloneNode(false) be preview = math.cloneNode(true) to get a deep copy? Otherwise you just get the empty <math> element.

@fred-wang
Copy link
Contributor Author

Finally, shouldn't preview = math.cloneNode(false) be preview = math.cloneNode(true) to get a deep copy? Otherwise you just get the empty element.

I'm not really sure to be honest, I tried true, false and no cloneNode at all. Only false worked on Chrome & Firefox.

@fred-wang
Copy link
Contributor Author

I updated the branch. That seems to work on Firefox, Opera, IE (all modes) and Chrome.

There is one issue with skipping the MathJax.Hub.config.preRemoveClass: this will skip everything if the user set that to "". This was done in some tests to prevent the preview removal. However, the solution is to modify the tests to set them to null instead.

@fred-wang
Copy link
Contributor Author

Davide, can you check that one so that I can launch the tests again?

@dpvc
Copy link
Member

dpvc commented Oct 11, 2013

I don't think I'm going to be able to work on this again until Monday or Tuesday, but will as soon as I can.

@dpvc
Copy link
Member

dpvc commented Oct 15, 2013

That looks OK. Technically, we should check for parent.className containing the MathJax preview class name (as one of the space-separated values) rather than actually equal to it, but that's probably not going to come into play.

@fred-wang
Copy link
Contributor Author

OK, should I update the branch or can we just go with that?

@dpvc
Copy link
Member

dpvc commented Oct 15, 2013

=> Ready to Release

@dpvc
Copy link
Member

dpvc commented Oct 15, 2013

WE can let it go for now. But keep in mind for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Issue has been reproduced by MathJax team Feature Request Fixed v2.3
Projects
None yet
Development

No branches or pull requests

3 participants