Fix #946

[ronkok]

Issue #946 identified a case in which array.js ate the final row of a well-written aligned environment.

This PR modifies code from PR #479 to fix this problem and to also continue to eat a trailing \\ submitted to any environment.

[kevinbarabash]

I wonder if we can test some these behaviors with unit tests that verify that one {aligned} snipped parses the same as another.

[ronkok]

I put in the test case from #946 into the screenshotter tests. I haven't thought of a more elegant way to test the parse comparison.

[edemaine]

I was hoping the following added tests in test/katex-spec.js would work, but I can't even get the second one to work on master (though it should)... must be a small difference in the parse tree.

describe("An aligned environment", function() {
    ...
    it("should eliminate final newline 1", function() {
        expect("\\begin{aligned}x&y\\\\&z\\\\\\end{aligned}")
            .toParseLike("\\begin{aligned}x&y\\\\&w\\end{aligned}");
    });

    it("should eliminate final newline 2", function() {
        expect("\\begin{aligned}x&y\\\\z&w\\\\\\end{aligned}")
            .toParseLike("\\begin{aligned}x&y\\\\z&w\\end{aligned}");
    });

I guess we could add a test that confirms the right number of rows in the parsed align table.

[kevinbarabash]

@edemaine thanks for checking that. I would've expected that to work. :\

[ronkok]

The code that that removes a trailing \\ is located in array.js, not in Parser.js. We can't expect a unit test on the parse tree to tell us anything useful about the handling of a trailing \\.

Maybe I can think of a unit test that works on buildTree or on even on buildMathML. I'll look into it.

Not right away though. My next few days are pretty booked up. If someone would like to suggest a test sooner than that, feel free.

[kevinbarabash]

The code that that removes a trailing \ is located in array.js, not in Parser.js. We can't expect a unit test on the parse tree to tell us anything useful about the handling of a trailing \.

But the Parser.js imports environments.js which imports array.js and we do call the handler for a particular environment when we encounter it from Parser.js. I'd be interested in seeing how those parse trees diff for those case where we expect them to be the same.

[ronkok]

The most recent commit fixes a bug and adds two tests that confirm the correct number of rows.

[ronkok]

Can anyone suggest why Travis has not completed its check? Or what I can try to get it working?

When I ran jest locally, the tests all ran well.

[kevinbarabash]

@ronkok not sure why. I'm going to "update branch" and see if that does anything.

[kevinbarabash]

Sorry for the delay in reviewing this. Code changes look good, requesting a minor update to one of the tests. See inline comments for details.

[kevinbarabash]

Should the TeX code be:

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

to include the extra newline that's supposed to get eaten?

[ronkok]

Right you are. I'll make the change and resubmit.

[kevinbarabash]

Thanks for fixing this... I wish our parse tree was nicer.

[ronkok]

@kevinbarabash Thank you. That did the trick.

[kevinbarabash]

LGTM

[kevinbarabash]

@ronkok thanks for fixing that test.

[edemaine]

FWIW, I think this rather significant bug fix is worth a release. Thanks guys for making it happen!

[ccorn]

Thanks too, though to me it seems that the 2nd test misses an & and thus would let the unfixed version pass as well. Update: Fixed. Thanks!