Fix #337 - Array/Matrix environments do not trim newlines

[cbreeden]

This is an attempt to fix #337. With all my testing it appears to work as shown in the images below:

tex
  \begin{bmatrix}
    a & b \\
    c & d \\ 
  \end{bmatrix}

You should trim at most the last newline. This agrees with LaTeX:

  \begin{bmatrix}
   \\ a & b \\ \\
    c & d \\ \\
  \end{bmatrix}

Though there is one question that I was hoping to clear up. I wasn't able to trigger rowGaps to be anything other than null. I'm not sure how they are supposed to be handled in the case for deleting a row.

I also wasn't exactly sure how I should go about using the screenshooter. I was able to get the docker to run. Am I supposed to run some kind of image diff on the results with what you have on github?

EDIT: Sorry for the duplicate pull request. I wanted to work on something else so I needed to put these changes into a branch.

[k4b7]

@cbreeden thanks for the pull request. I'll have a look at it tomorrow.

Please sign the CLA, found at found at www.khanacademy.org/r/cla, so we can merge this in.

[k4b7]

To use the screenshotter, you'll need to either add a new entry in https://github.com/Khan/KaTeX/blob/master/test/screenshotter/ss_data.yaml or modify an existing entry. I think it makes sense to create a new one and label it appropriately so that people know that the test is looking for a particular behavior. After running make screenshots it should generate two new image files which should be added to this pull request.

[cbreeden]

@kevinbarabash Thanks! I filled out the CLA. Can we put a hold on this? I have been spending some time trying to write up a proposal AST which would take out a lot of the TeX logic from the BuildHTML with a few goals:

1. Setup a foundation for pluggable renderers
2. Provide a starting point for implementing macros (at least have a working sketch of where they would fit)
3. Auditing compliance of relevant TeX rules found in TeXbook.

I started working on implementing <math atom> commands like: \mathop, \mathrel, etc.. In hopes to be able to implement commands like \stackrel, and I was able to for the most part except that \mathop{=}\limits^{?} wouldn't work primarily due to TeX logic living in BuildHTML.js. Certainly I could hack it out, but there were other issues involved such as implementing \mathrel commands would essentially require a new relgroup, just like there is a mathordgroup and so on to allow recursive expansion. This brought me to the conclusion that it would most likely be better to wait for a new parser/AST implementation before continuing.

The only reason I ask for the hold is that I believe I am almost complete with the proposal. I just need to finish two more tasks:

• Review placement for user macro expansions and "hard-coded" expansions (ie: matrix)
• Check to make sure all of the current "dirty-tricks" would translate to the new proposal.

[k4b7]

@cbreeden looking forward to your proposal.

[k4b7]

What happens if there are more extra trailing line breaks?

[cbreeden]

Inside the macro for matrices, TeX leaves a \crcr for the last row which acts like a \cr, unless it follows a \cr in which case it does nothing. This is why the last \cr or \\ is optional with matrices. But this also means it trims at most the last row. See this reference

[k4b7]

Though there is one question that I was hoping to clear up. I wasn't able to trigger rowGaps to be anything other than null. I'm not sure how they are supposed to be handled in the case for deleting a row.

The following works:

\begin{pmatrix}
  a & c \\[1em]
  b & d\\[2em]
  c & e
\end{pmatrix}

[k4b7]

I see... extra \\ add spacing after lines (or before if there is one before), but we should be rendering an empty line after the extra space. @cbreeden It's probably worth updating the matrix screenshot test to include an example that tests this behavior. Could you ad d a test case to https://github.com/Khan/KaTeX/blob/master/test/screenshotter/ss_data.yaml? Then run make screenshots to regenerate the screenshot files and commit the changed files.

[k4b7]

@cbreeden friendly ping on updating/adding screenshots.

[cbreeden]

Sorry. I'll make the screenshots when I get back next week.

[edemaine]

@cbreeden This PR recently reached its 1-year anniversary! It looks like this was almost done -- just needed screenshots. Let me know if you want help with those.

[k4b7]

I cherry picked the change on to master and it doesn't seem to have any effect.

  \begin{bmatrix}
   \\ a & b \\ \\
    c & d \\ \\
  \end{bmatrix}

stills has an extra space.

It turns out that lastRow is a little more complicated that it used to be:

[cbreeden]

It turns out that lastRow is a little more complicated that it used to be

Indeed. There seems to be more nesting than I remember. I needed to add a few more .values and that seemed to do the trick. I had an issue getting docker working (this is a local issue, not an issue with KaTeX), would it be possible to get someone to do a run for me?

[k4b7]

@cbreeden thanks for updating. I can run the docker. I'll send you a PR with the screenshots.

[cbreeden]

Ok not sure what those errors are about. I'll try getting docker working on my machine again and take a look.

[k4b7]

It's odd that it's only failing for Chrome. I can see why it might change though:

AccentsText: |
    \begin{array}{l}
    \text{\H{e}}\\
    \text{\H{X}}\\
    \end{array}

[k4b7]

@cbreeden I'll send you another PR with screenshots for AccentsText.

[cbreeden]

I was able to get the docker to work again! 🥇

The AccentText failure makes sense. The HTML should be different, but it should look roughly the same here since we aren't using delimiters. But it would've made a difference if it were embedded in an HTML document for instance.

When I ran docker locally, I had a sporadic failure for

M       test/screenshotter/images/Unicode-chrome.png

The Unicode test was interesting:

Unicode: \begin{matrix}\text{ÀàÇçÉéÏïÖöÛû} \\ \text{БГДЖЗЙЛФЦШЫЮЯ} \\ \text{여보세요} \\ \text{私はバナナです} \end{matrix}

This PR shouldn't affect this test since it doesn't have a trailing empty line, but just to be sure I inspected the HTML output for both with and without this PR and they were identical: https://gist.github.com/cbreeden/72ca53e6089b9d39f2fcc54bffcda3a5

[cbreeden]

Chrome is a little finicky with these tests. I wonder if the new headless API for chrome would help here.

[k4b7]

LGTM

[k4b7]

I wonder if the new headless API for chrome would help here.

Something we might look into. I think we'd still want to do it inside a docker though b/c there's lots of variables that affect the rendering.

[k4b7]

@cbreeden thanks for PR.