Upgrade MathJax support from v2/v3 to v3/v4#7898
Conversation
…cript to reference mathjax v3/v4 instead of v2/v3
…roach, and migrate to v3/v4
…t of render, make sure raw input text is hidden at end of MathJax render, start render chain with Promise.resolve()
656d638 to
9c0b33c
Compare
9c0b33c to
83bb307
Compare
| // Strip $…$ delimiters and clean up escape charaters, | ||
| // then pass the result to MathDocument.convert() to get an SVG element back | ||
| const texMath = cleanEscapesForTex(_texString).replace(/^\$+|\$+$/g, ''); | ||
| return MathJax._.mathjax.mathjax.handleRetriesFor(function() { |
There was a problem hiding this comment.
Why is this call to handleRetriesFor required?
There was a problem hiding this comment.
Found a page in the docs which explains it: https://docs.mathjax.org/en/v4.0/web/retry.html
From what I understand, it sounds like MathJax dynamically loads some features/extensions only as needed, and the internal synchronous functions are designed to error out if a required feature is not available, with a wrapper function catching the failure, loading the required feature, then retrying the function. The wrapper function returns a Promise which resolves when the wrapped function eventually completes successfully.
handleRetriesFor() is that wrapper function.
.convert() is a synchronous function, so wrapping it inside handleRetriesFor() has the additional impact of turning the call to .convert() into an asynchronous function.
There was a problem hiding this comment.
Thanks for the explanation!
Co-authored-by: Cameron DeCoster <cameron.decoster@gmail.com>
Co-authored-by: Cameron DeCoster <cameron.decoster@gmail.com>
…d larger than 16)
dc176ac to
5b03600
Compare
…ashboard index page for mathjax 4 chtml
| .style({ | ||
| overflow: 'visible', | ||
| 'pointer-events': 'none', | ||
| 'font-size': config.fontSize + 'px', |
There was a problem hiding this comment.
Note: this addition fixes a bug where MathJax expressions would not render larger than font size 16 in plotly
| "@plotly/mathjax-v3": "npm:mathjax@3.2.2", | ||
| "@plotly/mathjax-v4": "npm:mathjax@^4.1.2", |
There was a problem hiding this comment.
Here the caret (^) sign is removed for MathJax v3 pinning down the version of dependency. IMHO this is a good practice to avoid unwanted test breakages as a result of future minor/patch releases of MathJax v3.
If that's the case, then we should possibly do the same for MathJax v4 i.e.
"@plotly/mathjax-v4": "npm:mathjax@4.1.3",Please note that the latest v4 is now at 4.1.3:
https://www.npmjs.com/package/mathjax?activeTab=versions
There was a problem hiding this comment.
Did you have trouble with that in the past? If not, it seems fine to leave this unbounded and rely on the lock file to keep us in sync.
There was a problem hiding this comment.
Yes I recall noticing baseline changes between MathJax v2 patch and minor releases.
If you decide to unpin mathjax-v4 then mathjax-v3 may also be reverted to the unpin version i.e.
"@plotly/mathjax-v3": "npm:mathjax@^3.2.2",
camdecoster
left a comment
There was a problem hiding this comment.
It's working for me. Good work!
| // Strip $…$ delimiters and clean up escape charaters, | ||
| // then pass the result to MathDocument.convert() to get an SVG element back | ||
| const texMath = cleanEscapesForTex(_texString).replace(/^\$+|\$+$/g, ''); | ||
| return MathJax._.mathjax.mathjax.handleRetriesFor(function() { |
There was a problem hiding this comment.
Thanks for the explanation!
| "@plotly/mathjax-v3": "npm:mathjax@3.2.2", | ||
| "@plotly/mathjax-v4": "npm:mathjax@^4.1.2", |
There was a problem hiding this comment.
Did you have trouble with that in the past? If not, it seems fine to leave this unbounded and rely on the lock file to keep us in sync.
Update plotly.js to be compatible with MathJax v4, and drop support for MathJax v2.
Closes #7598.
Due to some logic improvements, this should also make MathJax rendering faster.
src/lib/svg_text_utils.jsto use MathJax v3/v4 syntax to render math symbolsMathJax.startup.defaultReady()on every individual renderMathDocument.convert(tex)rather thanMathJax.typeset(tex)MathJax._.mathjaxnamespace. While not officially documented, the pattern is used in several places (one, two, three) in the MathJax docs for advanced configurationmathjax3__mathjaxmock, the pie labels which were previously hidden, are now visible as raw MathJax. We don't support rendering MathJax in pie labels. Now at least the behavior is consistent between v3 and v4. However we could change the behavior to hide MathJax text strings in places where we don't support rendering MathJax.Steps for testing
npm start) which uses MathJax 4mathjaxin the name (and the other baselines changed here) looks OK and matches the new baselines on this branchmathjaxmock itself contains several strings of TeX which don't render due to us not supporting MathJax in pie labels; this is not new, and it's (somewhat) deliberate, to lock in the fallback behaviorhttp://localhost:3000/devtools/test_dashboard/index-mathjax3.html) and verify again that everything looks fine