Skip to content

Native MathML: add a workaround for <mlabeledtr>, rowspacing/columnspacing #356

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

Closed
fred-wang opened this issue Nov 27, 2012 · 20 comments
Closed
Labels
Accepted Issue has been reproduced by MathJax team Browser Bug Fixed Test Not Needed v2.2

Comments

@fred-wang
Copy link
Contributor

is used in the code generated by MathJax from LaTeX (for equation labeling) but browsers like Webkit, Gecko or Opera do not support this element:

https://bugzilla.mozilla.org/show_bug.cgi?id=689641
https://bugs.webkit.org/show_bug.cgi?id=85732
http://www.w3.org/TR/mathml-for-css/#d2e2181

cf the discussion on mathjax-dev

@fred-wang
Copy link
Contributor Author

One of the most important missing features in Gecko/Webkit is mtable@rowspacing/mtable@columnspacing. These attributes are used a lot by the TeX input Jax so I'm stretching a bit this issue to include them.

https://bugzilla.mozilla.org/show_bug.cgi?id=330964
https://bugs.webkit.org/show_bug.cgi?id=85731

I suspect this could be implemented by editing CSS properties on the table cells. The browser's default rules are:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/mathml.css#301
http://trac.webkit.org/browser/trunk/Source/WebCore/css/mathml.css#L187

and be dynamically updated. The MathML CSS profile used by Opera contains mtd { display:table-cell; padding:0 0.5ex; }.

@fred-wang
Copy link
Contributor Author

@dpvc You said that you did some experiments with mabeledtr. Can you publish your work? I think I can take this issue and try something for rowspacing/columnspacing too.

@dpvc
Copy link
Member

dpvc commented Jan 31, 2013

I had one small piece to add before committing it. I'll see if I can get to that today.

@dpvc
Copy link
Member

dpvc commented Jan 31, 2013

OK, my issue356 branch has the fix to this point.

@fred-wang
Copy link
Contributor Author

Thanks! I didn't look the code in details, but that looks great. I've verified that it works as expected for the basic tests from MathMLToDisplay/Presentation/TablesAndMatrices/mlabeledtr/ (essentially those of the MathML Test suite). I'll see if I can do something for rowspacing/columnspacing.

@dpvc
Copy link
Member

dpvc commented Feb 4, 2013

Note that this commit also include adding the <mrow> to <mtd>, and an attempt to fix <mspace> by setting the style to the proper width. I had hoped to be able to handle negative widths by using a negative margin-right, but this doesn't work (FF's width computations are still wrong for this). I may try to do more to work around that, but I'm not sure if it can be done.

@fred-wang
Copy link
Contributor Author

OK, thanks. The bug with mspace/mpadded in table will be fixed, so no need to attach a style attribute for Gecko >= 20. I didn't get any update yet regarding the patch I submitted last November, to implement negative mspace@width (or at least to implement it partially, but I think it is enough for MathJax use cases, there are experimental builds here: http://www.wg9s.com/mozilla/firefox/)

@fred-wang
Copy link
Contributor Author

I've created a fork of your issue356 branch and commited some changes to implement rowspacing/columnspacing.

I realized that your commit says it implements malebeledtr for "Firefox and other browsers" but there is still a "HUB.Browser.isFirefox" around the section you added. I don't know if that's on purpose...

@dpvc
Copy link
Member

dpvc commented Feb 6, 2013

Well, I hadn't completely finished the code, and committed it so you could start to work with it. I guess that needs to be something like nMML.mlabeledtrBug instead, and the mtd and mspace code need their own bug values as well.

@fred-wang
Copy link
Contributor Author

OK, I can try to fix this and finish the work. But the code to set the padding on the cell didn't seem to work very well with Chrome, at least when I tried this morning.

@fred-wang
Copy link
Contributor Author

It turned out that my code used the DOM element interface but for some reason I have to use the DOM node interface to make it work with Chrome.

I updated my branch with these changes as well as those mentioned in the review. I also added the nMML bug properties.

It does not seem possible to make this work on Opera, so let's not do it.

@ghost ghost assigned fred-wang Feb 6, 2013
@dpvc
Copy link
Member

dpvc commented Feb 6, 2013

OK, good to know about Chrome.

About Opera, I'm not surprised. The question is whether the original rendering of <mlabeledtr> is better or worse than our modified version. Even though the modified one doesn't do exactly what we want, it might still be better than the original. What do you think?

@fred-wang
Copy link
Contributor Author

Actually, I think mlabeledtr is not implemented at all in Opera:

http://www.opera.com/docs/specs/presto2.12/mathml/

so I guess we can enable our workaround for mlabeledtr Opera too (the one for rowspacing/columnspacing does not change anything)

@dpvc
Copy link
Member

dpvc commented Feb 6, 2013

Sounds good. I thought that might be the case.

@fred-wang
Copy link
Contributor Author

It turned out that my code used the DOM element interface but for some reason I have to use the DOM node interface to make it work with Chrome.

I have reported this bug to Webkit:
https://bugs.webkit.org/show_bug.cgi?id=109556

I don't think the code will append non-element nodes like comments or text as children of mtable or mtr, so that should be safe to keep the node interface. But perhaps it may be better to use firstElementChild/nextElementSibling as this seems to work in Chrome.

@fred-wang
Copy link
Contributor Author

@fred-wang
Copy link
Contributor Author

I updated my branch again to modify your variables and follow the coding style. I think the code should be ready now.

@dpvc
Copy link
Member

dpvc commented Apr 26, 2013

Since this is merged into the branch for issue #359, it will be merged into develop when issue359 is.

=> Ready to Release

@fred-wang
Copy link
Contributor Author

And thus you can merge this too.

@dpvc
Copy link
Member

dpvc commented Apr 29, 2013

=> Merged as 0891402

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 Browser Bug Fixed Test Not Needed v2.2
Projects
None yet
Development

No branches or pull requests

2 participants