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

The MathJax class forces ltr direction on messages #627

Closed
amire80 opened this issue Oct 17, 2013 · 24 comments

Comments

Projects
None yet
3 participants
@amire80
Copy link

commented Oct 17, 2013

As I look at http://devel.mathjax.org/testing/mathjax/MathJax-issue530/test/localization.html?locale=he , I see that all the error messages (yellow background, red text and border) are shown with direction: ltr in the CSS. User interface messages in right-to-left languages must have direction: rtl.

(W3C's best practices for bidirectional HTML suggest to use HTML dir="rtl" and not CSS direction: rtl. If this be fixed using the HTML dir, it is preferable, but that is secondary.)

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

As Davide said elsewhere these yellow messages are actually MathML merror elements and are then converted into SVG, HTML-CSS or MathML (according to the selected output mode).

It's possible to use a direction: rtl CSS property on the MathML token elements, but that won't work on non-browser MathML rendering (i.e. MathPlayer here). MathML has a dir attribute that could be used for that purpose and should work in Firefox native MathML and (I guess) MathPlayer.

MathML bidi is not implemented yet in MathJax and we need that for the SVG/HTML-CSS output modes. Actually, here only the dir attributes on the MathML token element is necessary. I guess we can use the dir attribute for the HTML-CSS element. For SVG, I suspect that would be a bit more complicated since we use <path> not <text> elements. Probably we can rely on the mtextInherit work, here.

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

(See also #610 and #474)

@ghost ghost assigned fred-wang Oct 18, 2013

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

I've created an experimental bidi branch that just implements basic features for MathML bidi that are needed here. This should work in MathML and HTML-CSS output modes, not SVG yet.

@amire80 Can you try http://devel.mathjax.org/testing/mathjax/MathJax-bidi/test/localization.html?locale=he and see if that gives better result? The default output mode is HTML-CSS, you may switch to the MathML in Firefox and see if that works too.

Known issues:

  • some messages don't have RTL yet (e.g. the "fake" messages (in bold) in the test or other errors created in the HTML-CSS/SVG output)
  • does not work in SVG output mode
@amire80

This comment has been minimized.

Copy link
Author

commented Oct 18, 2013

It is not better. The parent <span>s of the error messages carry style="direction: ltr" which is applied to the error messages, so they are still wrong.

How can I switch to MathML?

@amire80

This comment has been minimized.

Copy link
Author

commented Oct 18, 2013

... Although the MathJax class doesn't have direction: ltr any more.

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Use the MathJax menu: right click on any equation and choose Math Settings → Math Renderer → MathML

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

(look at the "TeX" item of the test page, as the "MathML" one has probably issues)

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

@amire80: Which span carries style="direction: ltr"? I see that the default is direction=ltr until we arrive at the span with class="mstyle", where it becomes rtl until the text node. Here is what I get in HTML-CSS output mode:

hebrew

I think I have fixed the remaining MathML messages. However two messages on the test page (the bold ones) are not generated by the normal code path and are just "faked", so they won't render correctly (however they will render correctly in the rare cases when they happen by the normal way).

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

@amire80 I've tried something for the SVG output too. Can you please try again with the three output modes (HTML, SVG and MathML)? http://devel.mathjax.org/testing/mathjax/MathJax-bidi/test/localization.html?locale=he (be sure to clear the cache)

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

=> Setting ready for review, just to get preliminary feedback.

https://github.com/fred-wang/MathJax/compare/mathjax:develop...bidi

The idea is to implement MathML bidi just for what is necessary for the merror messages. For the MathML output, it's just setting the correct dir attribute. For the SVG, I only considered the cases where <text> is used. For HTML, I just apply the css direction=rtl property on the span ; this is probably wrong in general but will work for the error messages and might help people wanting simple support like in #610 (if that hack is not wanted, I can remove that). I only considered dir on math/mstyle/tokens not mrow.

@amire80

This comment has been minimized.

Copy link
Author

commented Oct 18, 2013

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Great, thanks for the feedback :-)

@dpvc

This comment has been minimized.

Copy link
Member

commented Oct 19, 2013

@fred-wang, I've taken a look at the code and have the following recommendations:

Since you need to check the fontDirection() in several locations, make a common routine in the mml Element Jax to handle the attributes. Here's what I'd recommend: add

  Error: function (message,def) {
    var mml = this.merror(message),
        dir = MathJax.Localization.fontDirection(),
        font = MathJax.Localization.fontFamily();
    if (def) {mml = mml.With(def)}
    if (dir || font) {
      mml = this.mstyle(mml);
      if (dir) {mml.dir = dir}
      if (font) {mml.style.fontFamily = "font-family: "+font}
    }
    return mml;
  }

after the copyAttributeNames at line 227 of jax/element/mml/jax.js. This creates the <merror> element, looks up the direction and font from the localization data, and adds an mstyle for the changes if there is a need for one. It also accepts an optional def parameter that allows additional attributes for the <merror>, if needed (you can use this in mglyph.js for example).

Then, in the AsciiMath, MathML, and TeX input jax, you can use things like

return MML.Error(message);

rather than

var mml = MML.merror(message);
return (MathJax.Localization.fontDirection() ? MML.mstyle(mml).With({dir: "rtl"}) : mml);

Note also that your version forces "rtl" direction, even if the direction specified by the localization file is "ltr" (though I agree that is unlikely).

In autoload/mglyph.js,

err = MML.merror(values.alt).With({mathsize:"75%"});
if (LOCALE.fontDirection()) {err = MML.mstyle(err).With({dir: "rtl"})};

can become

err = MML.Error(values.alt,{mathsize:"75%"});

instead. The second error message can be treated similarly. Note that there is also a corresponding error in the jax/output/SVG/autoload/mglyph.js file that you missed.

I did some reading about the direction CSS, and (as the OP suggested), it might be better to use dir="rtl" attributes rather than style CSS. One issue is that the direction CSS applies to block-level elements (see the last sentence of the Firefox documentation, and to make it work with in-line elements, you also need unicode-bidi: embed. I tried that, and it does work, but the dir attribute seems simpler.

So I'd recommend changing HTMLhandleDir() to be

     HTMLhandleDir: function (span) {
       var dir = this.Get("dir",true); // only get value if not the default
       if (dir) {span.dir = dir}
       return span;
     },

Note that this also only sets the value when it is actually needed (i.e., when it has been specified as something other than the default), whereas the current implementation alway adds the attribute to every token element, whether it is needed or not.

Finally, because the token elements will inherit the value from <mstyle> or the base <math> element, there is no need to use HTMLhandleDir() in the MML.mstyle and MML.math objects. Indeed, I think it is probably a mistake to do so, as the MathML3 spec indicates that "the bidirectional algorithm is applied independently to the contents of each token element", so I think you want to specify this for the individual spans for those token elements rather than on the container span for the mstyle or math element, as that would mean that the bidirectional algorithm would treat the child spans of those element as part of a larger run of text, so neighboring token elements would interact rather than be separate.

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2013

Finally, because the token elements will inherit the value from or the base element, there is no need to use HTMLhandleDir() in the MML.mstyle and MML.math objects.

The idea was to try to implement an experimental and rudimentary support for the "overall direction bidi" in the HTML output as asked in #610 instead of just the bidi for token elements that is needed here. It seems that might work for simple equations, but I suspect that in general this is incorrect and would need more careful work. However, since that was asked by @mohsen-ghaly, I thought adding this experimental hack would be useful for some people. That won't be problematic for the error messages, since there is essentially only one single merror element here. However as I said, I can remove that if you prefer.

@dpvc

This comment has been minimized.

Copy link
Member

commented Oct 20, 2013

I think it would be best to wait with that until we can do it properly, rather than have people start using something that will likely change substantially in the future. I can see them doing things to get around the problems that will just get them into trouble later. Fixing the error messages is an important step, but the bidi control for the rest of the math probably should wait.

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

Yes, that makes sense. I'll update the branch today.

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

https://github.com/fred-wang/MathJax/compare/mathjax:develop...bidi

=> Ready For Review

@dpvc could you please add a look again today, so that we can merge this and start the beta release process?

@dpvc

This comment has been minimized.

Copy link
Member

commented Oct 21, 2013

I had added the def parameter to MML.Error() so that things like

err = MML.Error(values.alt).With({mathsize:"75%"});

could be handled as

err = MML.Error(values.alt,{mathsize:"75%"});

putting the mathsize on the original merror rather than on the mstyle, but it works either ways, so I suppose it is not a problem to do it this way.

=> Ready for Release

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

Oops, sorry I missed that change. I'll do it when I do the pull request.

@dpvc

This comment has been minimized.

Copy link
Member

commented Oct 21, 2013

Thanks. I just noticed that putting the mathsize on the mstyle does cause problems because both the merror and the mtext that it contains get mathize:75%, and so the messages are too small.

@dpvc

This comment has been minimized.

Copy link
Member

commented Oct 21, 2013

There are two occurrences in the HTML-CSS glyph.js and one in the SVG glyph.js.

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

@dpvc I've created pull request #634

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

=> Merged

@fred-wang fred-wang closed this Oct 21, 2013

fred-wang added a commit that referenced this issue Oct 21, 2013

@fred-wang

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2013

This adds an extra comma causing a crash on old versions of IE. I've removed the comma from the beta branch, but this should be fixed on develop.

fred-wang added a commit that referenced this issue Oct 22, 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.