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

[Math Processing Error] with framespacing #367

Closed
fred-wang opened this issue Dec 21, 2012 · 14 comments

Comments

Projects
None yet
3 participants
@fred-wang
Copy link
Contributor

commented Dec 21, 2012

The following code

 <math xmlns="http://www.w3.org/1998/Math/MathML"  frame="solid" framespacing=" 1em 1em">
      <mtable>
    <mtr>
      <mtd/>
  <mtd><mtext>text</mtext></mtd>
</mtr>
    </mtable>
    </math>

generates a [Math Processing Error]

error: "m.toFixed is not a function"
file: "http://cdn.mathjax.org/mathjax/latest/unpacked/jax/output/HTML-CSS/jax.js"
line: 810

I assume the leading space in the framespacing attribute value is not trimmed.

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2012

The syntax of framespacing is given in chapter 3 by "length, length" where the comma means

"one instance of each form fi, in sequence, with separating whitespace characters (but no commas)"

(http://www.w3.org/Math/draft-spec/chapter2-d.html#id.2.1.5.1)

In autoload/mtable.js, framespacing is parsed that way:

values.framespacing.split(/[, ]+/)[0]
values.framespacing.split(/[, ]+/)[0]

that seems to split the framespacing by either a space or a comma (or a repetition of these characters). There are two issues here:

  • the leading/trailing whitespaces are not trimmed.
  • only actual space can be used, not general whitespaces. Also, comma should not be accepted here.

The REC suggests "Since some applications are inconsistent about normalization of whitespace, for maximum interoperability it is advisable to use only a single whitespace character for separating parts of a value. Moreover, leading and trailing whitespace in attribute values should be avoided." but the most general syntax is still allowed (in particular by the RelaxNG schema), though.

I have proposed a fix on my issue367 branch:

https://github.com/fred-wang/MathJax/compare/master...issue367

@ghost ghost assigned fred-wang Dec 24, 2012

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2013

Crashtests/issue367.html

=> In testsuite

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 20, 2013

I see you have used the String trim() method. Unfortunately, it is not available in some supported browsers (e.g., IE < 9). So I think it would be better to replace trim() by our own copy. It could call the built-in if available, otherwise do the replace by hand. I suppose the check could be made only once by making the definition of the method being dependent on the availability of String.prototype.trim.

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2013

According to http://stackoverflow.com/questions/2308134/trim-in-javascript-not-working-in-ie, this would work:

if(typeof String.prototype.trim !== "function") {
  String.prototype.trim = function() {
    return this.replace("^\s+|\s+$/g", ""); 
  }
}

where could I place that, somewhere in MathJax.js?

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 21, 2013

It's true that this would work, but it is not what we should do. A library like MathJax should not alter the base classes of the browser. That would be an unexpected side-effect of loading MathJax, and one that we should avoid. If you were just writing your own web page, that would be fine, but since MathJax is used in other people's web pages, possibly with many other libraries, we should not alter the basic functionality of the base classes. It is a slippery slope. (We once had a user report a problem with MathJax that was actually due to some other library adding new prototype functions which were not fully compatible with the documented behavior of the functions.)

So I'd recommend using our own function, but not adding it to the String object.

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2013

OK, that makes sense but that's a bit a shame to do that just to support old versions of IE. In this particular case, the behavior of trim is well documented, so that should be safe to define https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/Trim#Compatibility

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 21, 2013

I understand the desire, but I really don't think MathJax should be altering the base classes when it is not absolutely necessary. I'm sure this would be safe, but it is a slippery slope, and not one I want to start down. Loading MathJax should not cause changes to the methods in the browser's implementation of javascript. That is not MathJax's job. If the page author wants that, they should use one of the compatibility libraries specifically for that purpose. MathJax shouldn't do it for them without their consent.

Perhaps Peter will have an opinion on this.

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2013

I agree with you in general that we should not define our custom functions on the core Javascript objects and I'm not suggesting that we start doing that. Here that's not changing the core methods, it's just adding a workaround for a function defined in the Ecmascript standard and not available in old versions of browsers. MathJax has many workarounds for browser bugs and this one is not worse than the others: here we can exactly reproduce the definition of the trim function and that won't affect most recent versions of browsers...

As an example, in the past Gecko source contained C++ definitions like PR_TRUE, PR_MAX, etc to workaround incompatibilities between various C++ compilers and platforms that didn't support well the standards. That was a legacy of the very old source and people now prefer to have something clean and with the usual C++ features: true, std:max etc that everybody even new contributors are familiar with. So they removed these old macros and I think they are still continuing that work ; that's tedious and that make conflicts with others' work. I also recently saw we had a similar issue with custom TRUE/FALSE in 568b919. MathJax code base is not that large and we are not a lot to work on it at the moment, but if in the future we want to remove the workaround for trim, I would prefer to just remove a single place with the definition than to modify every places where it is used...

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 21, 2013

Here that's not changing the core methods, it's just adding a workaround for a function defined in the Ecmascript standard and not available in old versions of browsers.

That's not our job, and not something that people loading MathJax would expect to happen.

MathJax has many workarounds for browser bugs and this one is not worse than the others

The work arounds are internal to MathJax, not external, as your suggestion is. Those work arounds do not change the browser or is capabilities, as this change does. To date, the only global change that MathJax makes to the javascript area is adding the MathJax variable (of course it changed the DOM, but that's its job, whereas changing the functionality available in the String object is not). This would mark a significant change to what MathJax does, and it is not at all like the other bug work-arounds that MathJax currently has.

Your Gecko example misses the point. It is not about the people writing MathJax that I'm concerned about, it is the people who are combining MathJax with other libraries and their own code. The definitions in the C++ code in Gecko does not interact with the C++ code in other applications that you are running, so whether Gecko developers use workarounds for C libraries is a choice that in no way affects the people using Gecko. Those choices do not interact with the outside world.

But MathJax is not isolated from its surroundings, and it works within a shared environment. This change to String.prototype does affect the rest of the code that runs in the browser, and we don't control that code. It is not right for MathJax to make changes to the core environment in which all other javascript code has to run. I'm sorry that that makes the maintenance of MathJax a little more complicated.

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2013

Well, I guess that's where we disagree: in my opinion the source code of free software program is one of the most important interaction with the outside world. That's an important difference with proprietary software: not only we care about the quality of the programs but also about the quality of the source. Developers willing to contribute should be able to quickly understand the code (using standard Javascript features helps) and their patches should not conflict just because we decide to rename everything at one point. At the moment we mainly have a user community and you've essentially been alone to work on the code, but I'm still hoping volunteers could help in the future. So I understand your point very well and I agree with you in general, I'm just giving another perspective from a bigger open source project. I maintain that in this really particular case, that won't be a problem: if people don't use trim they won't see a difference, if they use trim they certainly expect it to work in all browsers as defined by the Ecmascript standard. However, I you really prefer that, I don't mind adding our own function or repeating Regexp+replace everywhere as in the present code.

@dpvc

This comment has been minimized.

Copy link
Member

commented Mar 22, 2013

How about doing something like this in MathJax.Hub:

SplitList: ("trim" in String.prototype ?
                function (list) {return list.trim().split(/\s+/)} :
                function (list) {return list.replace(/^\s+/,'').replace(/\s+$/,'').split(/\s+/)})

and then in the mtable code (and other places), use:

var SPLIT = MathJax.Hub.SplitList;
var CSPACE = SPLIT(values.columnspacing),
    RSPACE = SPLIT(values.rowspacing),
    ...

Perhaps that will be more palatable.

@pkra

This comment has been minimized.

Copy link
Member

commented Mar 22, 2013

Do we know how other frameworks handle the trim problem (e.g. jquery)?

@fred-wang

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2013

OK, I've rewritten the changes in a separate branch. I've also fix a potential math processing error with invalid framespacing attribute. I'll also update the crashtest to include this case.

dpvc pushed a commit to dpvc/MathJax that referenced this issue Apr 1, 2013

@dpvc

This comment has been minimized.

Copy link
Member

commented Apr 1, 2013

Thanks. I've merged the issue367-bis branch into develop.

@dpvc dpvc closed this May 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.