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

MathJax config: Allow single $ delimiters, only apply to CSS "math" class #5504

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Oct 2, 2018

I'm not quite sure if that's the right thing to do. This should be understood as a question, but I made this PR so that it's easy to test the suggested behavior.

Feature or Bugfix

Something inbetween.

Purpose

  • Support single $ as math delimiters (probably only relevant in :nowrap: math directives).
  • Reduce MathJax processing to CSS math class.

Detail

This is some example reST code to check this out:

Dollars in normal text: $a^2$.
A single dollar sign: $.
An escaped dollar sign: \$.

Inline math: :math:`a^2`.

Normal math directive:

.. math::

    a^2

"nowrap" math with ``$``:

.. math::
    :nowrap:

    $a^2$

"nowrap" with normal text:

.. math::
    :nowrap:

    This is a \LaTeX{} equation: $a^2 + b^2 = c^2$

    This is a dollar sign: \$.

Note that this PR mainly fixes the :nowrap: cases above. This might not be important for most "normal" users. But when auto-generating reST files which have arbitrary math markup (out of my control), this is the only way I found to make it work.

@mgeier mgeier force-pushed the mathjax-config branch 2 times, most recently from 1427fa1 to d30694f Compare October 2, 2018 16:17
@codecov
Copy link

codecov bot commented Oct 2, 2018

Codecov Report

Merging #5504 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5504      +/-   ##
==========================================
+ Coverage   83.23%   83.26%   +0.02%     
==========================================
  Files         296      294       -2     
  Lines       39622    39585      -37     
  Branches     5882     5879       -3     
==========================================
- Hits        32981    32961      -20     
+ Misses       5277     5262      -15     
+ Partials     1364     1362       -2
Impacted Files Coverage Δ
sphinx/ext/mathjax.py 92.3% <100%> (-1.54%) ⬇️
sphinx/__init__.py
sphinx/make_mode.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af87e02...c07938e. Read the comment docs.

mgeier added a commit to spatialaudio/nbsphinx that referenced this pull request Oct 2, 2018
mgeier added a commit to spatialaudio/nbsphinx that referenced this pull request Oct 4, 2018
@jfbu
Copy link
Contributor

jfbu commented Oct 6, 2018

.. math::
    :nowrap:

    This is a \LaTeX{} equation: $a^2 + b^2 = c^2$

    This is a dollar sign: \$.

This modifies expected behaviour of math directive. It should not be used for arbitrary text. The :nowrap: intent is only to give Sphinx user possibility to decide of math display environment of his/her choice. The contents remained constrained to be LaTeX math not text.

At least that's how I have always considered it.

@tk0miya
Copy link
Member

tk0miya commented Oct 7, 2018

-0: I also feel strange because math directive represents "block" equations. So using $ is strange. I know it is rendered well in both imgmath extension and LaTeX builder. But it is not promised behavior to me.

@tk0miya
Copy link
Member

tk0miya commented Oct 7, 2018

In above comment, I said "block" equations. But "display" equations would be much better to describe it.

@jfbu
Copy link
Contributor

jfbu commented Oct 7, 2018

The example I quoted from @mgeier uses non-math paragraphs, in LaTeX encoding, inside math directive. This feels wrong to me in using the math directive. Besides for html+MathJax, MathJax is not LaTeX but a Javascript library partially implementing TeX mark-up particularly for math. So arbitrary LaTeX mark-up can not be allowed if it is to be rendered also by html + MathJax.

@mgeier
Copy link
Contributor Author

mgeier commented Oct 9, 2018

I agree that it is "strange" and it even "feels wrong" to write such math directives by hand. But in my use case (showing Jupyter notebook code cells with the nbsphinx extension) something like this would be necessary to get the same behavior as in the notebook application.

Is it possible to add this MathJax config from an extension without interfering when users have their own mathjax_config settings?

But apart from "being strange" and "feeling wrong", is there a real downside to my suggestion?

And what about the second part of this PR, about restricting MathJax to math CSS classes?

I think it isn't a huge problem, but it would be more "correct" to enable MathJax only in math classes, right?

This would be an admittedly quite unrealistic case where MathJax would be wrongly used in normal text:

I love $$ Money $$!

Interestingly it is still processed by MathJax if the dollars are escaped:

I love \$\$ Money \$\$!

@mgeier
Copy link
Contributor Author

mgeier commented Oct 19, 2018

Some background on why I need support for single $ signs: IPython just merged a PR that uses $\displaystyle ...$ for math output (ipython/ipython#11357) and I hope SymPy and others will follow suit. The reason for that change is left-aligning math in code cells of Jupyter notebooks when they are converted to LaTeX (via nbconvert or nbsphinx or other tools).

The reason for this PR here is to display this properly in Sphinx's HTML output when using Jupyter notebooks as source files (using the nbsphinx extension).

@mgeier
Copy link
Contributor Author

mgeier commented Nov 5, 2018

I've just re-based my commits for your merging convenience.

@mgeier
Copy link
Contributor Author

mgeier commented Dec 11, 2018

Just a little update: I've found out that using '.*' as ignoreClass is a bit restrictive, because it disables MathJax for nested elements (without the math class) within an element with the math class. I've changed this in c07938e.

Also, I've included the change directly into nbsphinx:

https://github.com/spatialaudio/nbsphinx/blob/68b709e72c22cd194470155d5db0b0dcbee19a6b/src/nbsphinx.py#L1756-L1766

... so I don't really need this PR anymore.

Feel free to close it!

@AA-Turner
Copy link
Member

@mgeier if possible, please could you add a CHANGES entry to this (& a potentially a test, though that might not be as easy, not a requirement) -- if this is useful to people beyond nbsphinx I'd like to merge it.

A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants