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

Add \jot lineskip to aligned environment, switch contents to displaystyle, and add gathered #725

Merged
merged 4 commits into from
Jun 10, 2017

Conversation

edemaine
Copy link
Member

@edemaine edemaine commented Jun 10, 2017

AMS environments aligned and gathered (not yet implemented in KaTeX, see #682) use \openup\jot to increase \baselineskip by \jot (which defaults to 3pt, see #687 for overriding). This lineskip in turns defines \strutbox in the array environment, which then gets scaled by \arraystretch. (As reported in #686, which this PR fixes.)

I've added a rowsAreLines option to array objects in the parse tree that encourages the outputter (here, buildHTML) to add \jot (currently hard-coded to 3pt, see #687) to each line. This seemed simpler than e.g. manually putting the equivalent of 3pt in the environment definition, and will be useful when we implement gathered in #682.

texcmp Aligned before this change:

aligned_before

texcmp Aligned after this change:

aligned_after

Note that other environments like array remain unaffected, as they were already correct (vertically anyway):

arrays

@edemaine
Copy link
Member Author

While I'm here, I tweaked the definition of the aligned environment to put all contents in \displaystyle. This feature was pointed out by @arnog in #682, and I confirmed that it's the case in LaTeX. For example, $\begin{aligned} \frac{x}{y} \end{aligned}$ now uses a display fraction, as it does in LaTeX.

I'm going to add the gathered environment next, to this PR, as it's almost trivial at this point.

@edemaine
Copy link
Member Author

edemaine commented Jun 10, 2017

I added the gathered environment, fixing #682. The test case (with \begin{gathered} on the left and \begin{array}{c} on the right) renders pretty well, but not quite as perfectly:

gathered

Let me know if you can see where the extra vertical space is coming from in LaTeX... The difference is the \frac in the cell -- both gathered and aligned behave the same in terms of vertical spacing (in both KaTeX and LaTeX). Anyway, the new spacing is far better than current KaTeX.

@edemaine edemaine changed the title Add \jot lineskip to aligned environment. Fix #686 Add \jot lineskip to aligned environment, switch contents to displaystyle, and add gathered Jun 10, 2017
@@ -592,6 +592,9 @@ groupTypes.array = function(group, options) {

// Vertical spacing
const baselineskip = 12 * pt; // see size10.clo
// Default \jot from ltmath.dtx
// TODO(edemaine): allow overriding \jot via \setlength (#687)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice TODO

@kevinbarabash
Copy link
Member

@edemaine can you rebase and re-run make screenshot now that #718 has been merged?

@kevinbarabash
Copy link
Member

whoops... I only meant to "comment" not "close and comment".

};
res = parseArray(context.parser, res);
res = parseArray(context.parser, res, "display");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the aligned environment also supposed to be in \displaystyle only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I checked in LaTeX, and $\begin{aligned} \frac{x}{y} \end{aligned}$ renders a display fraction.

// Count number of columns = maximum number of cells in each row.
// At the same time, prepend empty group {} at beginning of every second
// cell in each row (starting with second cell) so that operators become
// binary.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so that operators at the start of cell will have the proper spacing around them. What about operators at the end of a cell, or does that not happen in practice?

Copy link
Member Author

@edemaine edemaine Jun 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added documentation here, because it took me a long time to figure out why the existing code was adding empty groups. I checked and this is indeed how it's implemented in amsmath, specifically in \start@aligned. I'll add a note this effect.

src/buildHTML.js Outdated
// In AMS multiline environments such as aligned and gathered, rows
// correspond to lines that have additional \jot added to the
// \baselineskip via \openup.
if (group.value.rowsAreLines) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of this variable is confusing to me. Aren't rows already lines? This just adds more space between the rows/lines. Maybe change this to addJotBetweenRows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that is confusing. Renamed to addJot, which seems clear enough (as \jot is an interline spacing).

@edemaine
Copy link
Member Author

edemaine commented Jun 10, 2017

Comments addressed in 3c00b27 . Thanks for the review!

@kevinbarabash
Copy link
Member

@edemaine did you rebase and run make screenshots? I would've expected the LimitControls screenshots to change, see https://github.com/Khan/KaTeX/pull/718/files#diff-e55441e2f8a6090eac1fdb9e12ed86ed.

@edemaine
Copy link
Member Author

@kevinbarabash Yes, I rebased. Almost no screenshot tests involve aligned or gathered environments --
just the old Aligned test (modified in this PR) and the new Gathered test (new to this PR). Is there a reason LimitControls should have changed that I'm missing?

@kevinbarabash
Copy link
Member

@edemaine sorry... I wasn't looking at things carefully. I was comparing the ascent of glyphs when I should've been looking at the baselines. Also, for some reason I thought this was a diff that was opened before the #718, but it was actually only opened a few hours ago.

@kevinbarabash kevinbarabash merged commit 06d6c96 into KaTeX:master Jun 10, 2017
@kevinbarabash
Copy link
Member

Thanks for the PR. Awesome to see how quickly new environments can be added.

edemaine added a commit to edemaine/KaTeX that referenced this pull request Jun 10, 2017
edemaine added a commit to edemaine/KaTeX that referenced this pull request Jun 10, 2017
kohler added a commit to kohler/KaTeX that referenced this pull request Jun 29, 2017
edemaine added a commit to edemaine/KaTeX that referenced this pull request Jun 30, 2017
edemaine added a commit to edemaine/KaTeX that referenced this pull request Jun 30, 2017
kevinbarabash pushed a commit that referenced this pull request Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants