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

\kern fixes, \hskip support, \TeX, \LaTeX, \KaTeX #974

Merged
merged 15 commits into from
Nov 27, 2017

Conversation

edemaine
Copy link
Member

@edemaine edemaine commented Nov 19, 2017

This PR does a lot of cleanup on \kern-related functions, and defines \TeX and \LaTeX macros (fixing #224). It also proposes a possible replacement to \KaTeX (needs discussion).

In more detail:

  • Move \kern, \mkern into functions directory (and use new defineFuction interface)

  • Add \hskip, \mskip support (but without supporting plus/minus -- this should be a future issue)

  • Properly separate \kern, \hskip from \mkern, \mskip.
    (The former work in both modes, and don't support mu units.
    The latter work only in math mode and only support mu units.)

  • Render horizontal spacing commands (such as \kern) using MathML <mspace>.
    Previously this was rendered as an empty <mrow> with a TODO by @kevinbarabash to figure it out.

  • Implement \TeX macro identical to LaTeX's definition using \raisebox and \kern.

  • Implement \LaTeX macro roughly identical to LaTeX's definition (using \raisebox instead of a vertical box with a fill to get top alignment). Comments welcome, but I think this is a pretty good solution for now.

  • I tried implementing a new \KaTeX logo using a similar macro (now that we have all these features), instead of the large body of code in src/functions/katex.js and static/katex.less. It's currently defined as \katex so that you can check out this branch and compare the two:

    katex

    I'd like to suggest that this definition replace the old \KaTeX, to reduce code complexity, but this definitely needs discussion. Note the spacing is a little different, because it seemed natural to me to more closely match the spacing in \LaTeX. Feel free to disagree!

    Incidentally, this is also a step toward implementing \KaTeX in LaTeX, as mentioned in List LaTeX packages that make LaTeX behave most like KaTeX #793. (The existing definition would be challenging to port.)

@kevinbarabash
Copy link
Member

Cool!

Properly separate \kern, \hskip from \mkern, \mskip.
(The former work in both modes, and don't support mu units.
The latter work only in math mode and only support mu units.)

I think we'll need to bump the version again as this may break some math that was working for people.

Render horizontal spacing commands (such as \kern) using MathML .
Previously this was rendered as an empty with a TODO by @kevinbarabash to figure it out.

Thanks for figuring this out.

Implement \LaTeX macro roughly identical to LaTeX's definition (using \raisebox instead of a vertical box with a fill to get top alignment). Comments welcome, but I think this is a pretty good solution for now.

Cool. I think some people will be pretty stoked about this.

I tried implementing a new \KaTeX logo using a similar macro (now that we have all these features), instead of the large body of code in src/functions/katex.js and static/katex.less. It's currently defined as \katex so that you can check out this branch and compare the two:

I think the new \katex implementation is closer to the \LaTeX rendering, especially the kerning between the a and the T. It might look better if there was more space between the K and a. Looking at \LaTeX, there's an even amount of space between the L and the a and the a and the T.

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

The kern and skip changes look good. I think the \KaTeX macro needs a little tweaking, but not much.

@@ -0,0 +1,65 @@
//@flow
// Horizontal spacing commands
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this file kern.js b/c type: "kern" below... or maybe horizontal-spacing.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point -- switched to kern.js.

import { calculateSize } from "../units";
import ParseError from "../ParseError";

// TODO: \hskip and \mskip should support plus and minus in lengths
Copy link
Member

Choose a reason for hiding this comment

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

Good call splitting this out, tbh I'm not sure how to do this, at least for our current HTML layout strategy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, definitely for another time. Supporting fil/fill/filll units in plus at least should be possible using Flexbox. That'd be pretty useful -- but for another issue. I've opened #990 to track.

src/macros.js Outdated
// \kern-.15em%
// \TeX}
// This code roughly aligns the top of the A with the T. Here we approximate
// this vertical alignment with a \raisebox{.45ex}, which seems close.
Copy link
Member

Choose a reason for hiding this comment

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

What font size is being selected for the A? Perhaps we could compute the raisebox amount based on the difference in size between the A and the T.

Copy link
Member Author

@edemaine edemaine Nov 25, 2017

Choose a reason for hiding this comment

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

The A should be in the equivalent of \scriptsize, which has a scale of 70%. The height of a T is 0.68333em (according to fontMetricsData.js), so the raisebox should be 0.3 * 0.68333 = 0.204999em. I tried this, and it puts the A slightly higher than what I did, but it matches the LaTeX rendering better! Good thinking. I've pushed code that computes this automatically from the font data instead of hard-coding constants (other than 0.3 which seems safe enough).

Compare this screenshot with others on this page:
katex

And here's LaTeX's rendering:
latex

src/macros.js Outdated

// New KaTeX logo based on tweaking LaTeX logo
defineMacro("\\katex", "\\text{K\\kern-.25em\\raisebox{.45ex}{\\scriptsize A}" +
"\\kern-.15em\\TeX}");
Copy link
Member

Choose a reason for hiding this comment

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

Does K\\kern-.36em even out the spaces around the A?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice objective to even out those top spaces. By manual search, I found K\\kern-.16em to balance it out pretty evenly:

katex
http://localhost:7936/?text=%5Cbegin%7Barray%7D%7Br%7D%5Ckatex%5C%5C%5CLaTeX%5C%5C%5CKaTeX%5Cend%7Barray%7D

I feel like the this puts the bottom of the A a little far from the K, though. I tried tweaking the A slightly more to the left, with K\\kern-1.17em:

katex

Preference? This may be nit-picky, but if we're redesigning the logo, we might as well try to get it right. I do find these far more readable than the old logo, which is nice (but also more readable than the LaTeX logo).

Copy link
Member

Choose a reason for hiding this comment

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

I think the top one looks the best. In the bottom one the A is too cramped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, we're talking about the top logo in the top image vs. the top logo in the bottom image, right? (Bottom logo is the same in both images, the old KaTeX logo.)

Copy link
Member

Choose a reason for hiding this comment

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

I can't tell the difference between the top logos in the two images... whichever you think is better.

@ronkok
Copy link
Collaborator

ronkok commented Nov 24, 2017

Properly separate \kern, \hskip from \mkern, \mskip.

I have, so far, purposely omitted \mkern from the function support page because I thought it might break. Once the behavior of \mkern and \mskip is more reliable, I'll put them into the documentation.

@edemaine
Copy link
Member Author

edemaine commented Nov 24, 2017

I need to respond about the other issues (thanks for the review, @kevinbarabash!), but one question I have is whether the errors on misuse of \kern (especially) and \mkern (less so) should be turned into warnings. We don't really care about the separation for our purposes, it's just to encourage people to use LaTeX correctly, so maybe a warning is better? (We could still bump the version number, but maybe unnecessary to break existing code that uses \kern{..mu}.)

* Move \kern, \mkern into functions directory
* Add \hskip, \mskip support (but without supporting plus/minus)
* Properly separate \kern, \hskip from \mkern, \mskip.
  (The former work in both modes, and don't support mu units.
  The latter work only in math mode and only support mu units.)
@edemaine
Copy link
Member Author

edemaine commented Nov 25, 2017

I addressed @kevinbarabash comments (thanks!) and switched from errors to warning when misusing mu units/mkern (and rebased to remove conflict). I kept it an error to use \mkern/\mskip outside math mode, because this seems natural, and previously no kern/skip commands worked in text mode at all (which was a bug).

This should be ready for review again, and maybe one more iteration on exact spacing of the logos. Then I'd suggest that I delete the old \KaTeX code and replace it with the new macro. (This is technically another backwards incompatibility, though I can't imagine it causing too much trouble... we could do it more slowly if desired, or provide a \KaTeXold or something...)

@edemaine
Copy link
Member Author

Here's a texdiff for \LaTeX and \TeX (red is KaTeX, green is LaTeX):

latex

It appears to me that the A is slightly smaller in KaTeX, which causes the horizontal shift of the L. I don't understand why; I confirmed that the A is in a font size exactly 70% of the T, and LaTeX says the same in its rendering:

\hbox(0.0+0.0)x15.0
\OT1/cmr/m/n/10 L
\kern -3.6
\vbox(6.83331+0.0)x5.90282, glue set 2.04997fil
.\hbox(4.78334+0.0)x5.90282
..\OT1/cmr/m/n/7 A
.\glue 0.0 plus 1.0fil minus 1.0fil
\kern -1.49994
\OT1/cmr/m/n/10 T
\kern -1.66702
\hbox(6.83331+0.0)x6.80557, shifted 2.15277
.\OT1/cmr/m/n/10 E
\kern -1.25
\OT1/cmr/m/n/10 X
spacefactor 1000

I also notice that there's some difference between Chrome vs. Firefox rendering (notice height of A):

katex-chrome vs. katex-firefox

Personally I'd be OK with leaving it as is, as I'm running out of ideas (and time) for how to improve it, it matches the definition, and the rendering is reasonably close now.

I've committed this as a new screenshot test so that it can be explored later. I also switched over \KaTeX to the new macro, and deleted the old function code. So now the latest choice for the KaTeX logo is in the screenshot test.

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM. I love how macros help reduce the amount of the code we have to ship!

@kevinbarabash
Copy link
Member

@edemaine I'm not sure why all of the fonts screenshot tests are failing. 😕

@edemaine
Copy link
Member Author

@kevinbarabash Turns out those screenshots all have \KaTeX in them, so it makes sense (in hindsight). I can see their purpose: \KaTeX is supposed to reset the font. The new version doesn't -- I'm working on fixing.

@edemaine
Copy link
Member Author

OK, should be all fixed now.

@kevinbarabash
Copy link
Member

@edemaine thanks for investing and fixing the issue.

@kevinbarabash kevinbarabash merged commit 7036eb8 into KaTeX:master Nov 27, 2017
@edemaine edemaine mentioned this pull request Nov 28, 2017
edemaine added a commit to edemaine/KaTeX that referenced this pull request Feb 4, 2019
Update `test/screenshotter/test.tex`'s definition of `\KaTeX` macro to match
the metrics of `src/macros.js`'s definition of `\KaTeX`, and thereby fix KaTeX#1703.
I just changed the `\kern` metrics to match the update from KaTeX#974, and left
the font selection code to match LaTeX's definition of `\LaTeX`.
ylemkimon pushed a commit that referenced this pull request Feb 21, 2019
Update `test/screenshotter/test.tex`'s definition of `\KaTeX` macro to match
the metrics of `src/macros.js`'s definition of `\KaTeX`, and thereby fix #1703.
I just changed the `\kern` metrics to match the update from #974, and left
the font selection code to match LaTeX's definition of `\LaTeX`.
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.

3 participants