Stacking text commands#1009

ry-randall · 2017-12-04T21:50:47Z

Sorry for the delay on this one. Been busy lately :(

Note: This does not include Main-BoldItalic.

k4b7 · 2017-12-05T16:21:42Z

@rrandallcainc I'll try to look at it this weekend. The screenshots look great!

k4b7

Nice start on this. I think we'll probably want to change how we're modeling styles and families. See the inline comments for details.

k4b7 · 2017-12-07T02:57:28Z

metrics/mapping.pl

+    0x14 => [0x30C,-550,0], # \check (combining)
+    0x15 => [0x306,-550,0], # \breve (combining)
+    0x16 => [0x304,-550,0], # \bar (combining)
+    0x17 => [0x30A,-608,0], # ring above (combining)


Where did the horizontal shifts come from?

Hmm, I'm not sure. I ran the metrics as prescribed. Is there something I'm missing?

Looks like this copy/pasted from https://github.com/Khan/MathJax-dev/blob/master/fonts/OTF/TeX/makeFF#L1400-L1405 which is right on.

Ah, yes that's where I grabbed them from :). Thought you were saying there was something wrong with the shifts.

k4b7 · 2017-12-07T02:58:26Z

src/Options.js

    textSize?: number;
    phantom?: boolean;
    font?: string | void;
+    fontStyles?: Array<string>;


We've been using string[] style in other diffs. Can you change to that so we're consistent?

Done (no longer necessary)

k4b7 · 2017-12-07T03:05:29Z

src/buildCommon.js

            return makeSymbol(
-                value, "AMS-Regular", mode, options, classes.concat(["amsrm"]));
+                value, fontName, mode, options,
+                classes.concat(options.fontStyles, "amsrm"));


I feel like we should just pass fontStyles to makeSymbol so that the caller isn't responsible for calling retrieveTextFontName and concatenating classes.

Hmm, isn't that more or less what it's been doing? There's a lot of calls to makeSymbol, so I'd just be worried about the implications of changing.

k4b7 · 2017-12-07T03:08:30Z

src/Options.js

    textSize: number;
    phantom: boolean;
    font: string | void;
+    fontStyles: Array<string>;


I think changing font to baseFont or fontFamily would make it clearer that the two are necessary and work together.

According to the answer to https://tex.stackexchange.com/questions/5008/when-to-use-bold-italics-small-caps-typewriter-etc there are three axes for fonts:

The shape axis covers: \textup, \textit, \textsc and \textsl

The family axis covers: \textrm, \textsf, \texttt

The weight axis covers: \textbf, \textmd

Maybe we should change from font + fontStyles array to three props:

fontShape

fontFamily

fontWeight

Thanks for finding this. I like this approach better. Done.

k4b7 · 2017-12-07T03:09:37Z

src/buildCommon.js

+    fontStyles: Array<string>
+): string {
+    const baseFontName = retrieveBaseFontName(font);
+    const fontStylesName =  retrieveFontStylesName(fontStyles);


nit: extra space

k4b7 · 2017-12-07T03:13:15Z

src/buildCommon.js

+        fontStyles.indexOf("textbf") > -1 && (fontStylesName += "Bold");
+        fontStyles.indexOf("textit") > -1 && (fontStylesName += "Italic");
+    } else {
+        fontStylesName += "Regular";


It's kind of hard to grok this, maybe:

let fontStylesName = ''; if (fontStyles.indexOf("textbf")) { fontStylesName += "Bold"; } if (fontStyles.indexOf("textit")) { fontStylesName += "Italic"; } return fontStylesName || "Regular";

k4b7 · 2017-12-07T03:17:18Z

test/katex-spec.js

+        expect(markup).toContain("<span class=\"mord textit mathsf\">R</span>");
+        expect(markup).toContain("<span class=\"mord mathsf\">G</span>");
+        expect(markup).toContain("<span class=\"mord textbf mathsf\">B</span>");
+    });


You might want to try writing snapshot tests for new tests that verify things about the HTML markup we're producing. See mathml-spec.js for examples.

It would be good to have some tests that compare the markup produced for things that should be similar, e.g.

I would expect

\textsf{\textbf{mathrm{A}}}

to produce the same markup as

\mathrml{A}

A little bit more complicated but

\textsf{\textbf{\mathrm{\textsf{A}}}}

is equivalent to

\textsf{\textbf{A}}

I checked using quicklatex.com which I believe calls actual LaTeX on its backend.

These are great suggestions. Done.

I'm guessing you wanted:

\textsf{\textbf{$\mathrm{A}$}}

and

\textsf{\textbf{$\mathrm{\textsf{A}}$}}

I thought the HTML trees rendered would be equivalent between \textsf{\textbf{$\mathrm{A}$}} and \mathrm{A}, but they're not b/c the first one has extra spans. It's just that the visual output looks the same... unfortunately that's hard to test. :(

k4b7 · 2017-12-07T03:18:56Z

test/katex-spec.js

    it("should render \\textit{R} with the correct font", function() {
        const markup = katex.renderToString("\\textit{R}");
-        expect(markup).toContain("<span class=\"mord textit\">R</span>");
+        expect(markup).toContain("<span class=\"mord textit mathrm\">R</span>");


It's kind of strange to see the mathrm class being applied to text that's inside \textit. Is this necessary?

No, but I believe this should be textrm. Since it would be leveraging the default font family. Changed appropriately (and made the other text families separate).

ry-randall · 2017-12-07T14:49:02Z

@kevinbarabash Thanks for the quick feedback. Will take a look tomorrow or the wekeend.

ry-randall

@kevinbarabash Updated

ry-randall · 2017-12-12T15:51:56Z

src/buildCommon.js

+    fontStyles: Array<string>
+): string {
+    const baseFontName = retrieveBaseFontName(font);
+    const fontStylesName =  retrieveFontStylesName(fontStyles);


ry-randall · 2017-12-12T15:52:09Z

src/buildCommon.js

+        fontStyles.indexOf("textbf") > -1 && (fontStylesName += "Bold");
+        fontStyles.indexOf("textit") > -1 && (fontStylesName += "Italic");
+    } else {
+        fontStylesName += "Regular";


ry-randall · 2017-12-12T16:09:42Z

src/Options.js

    textSize?: number;
    phantom?: boolean;
    font?: string | void;
+    fontStyles?: Array<string>;


Done (no longer necessary)

ry-randall · 2017-12-12T16:29:33Z

src/Options.js

    textSize: number;
    phantom: boolean;
    font: string | void;
+    fontStyles: Array<string>;


Thanks for finding this. I like this approach better. Done.

ry-randall · 2017-12-12T19:03:52Z

src/buildCommon.js

            return makeSymbol(
-                value, "AMS-Regular", mode, options, classes.concat(["amsrm"]));
+                value, fontName, mode, options,
+                classes.concat(options.fontStyles, "amsrm"));


Hmm, isn't that more or less what it's been doing? There's a lot of calls to makeSymbol, so I'd just be worried about the implications of changing.

ry-randall · 2017-12-12T19:05:12Z

test/katex-spec.js

    it("should render \\textit{R} with the correct font", function() {
        const markup = katex.renderToString("\\textit{R}");
-        expect(markup).toContain("<span class=\"mord textit\">R</span>");
+        expect(markup).toContain("<span class=\"mord textit mathrm\">R</span>");


No, but I believe this should be textrm. Since it would be leveraging the default font family. Changed appropriately (and made the other text families separate).

ry-randall · 2017-12-12T19:26:32Z

test/katex-spec.js

+        expect(markup).toContain("<span class=\"mord textit mathsf\">R</span>");
+        expect(markup).toContain("<span class=\"mord mathsf\">G</span>");
+        expect(markup).toContain("<span class=\"mord textbf mathsf\">B</span>");
+    });


These are great suggestions. Done.

I'm guessing you wanted:

\textsf{\textbf{$\mathrm{A}$}}

and

\textsf{\textbf{$\mathrm{\textsf{A}}$}}

ry-randall · 2017-12-12T19:28:31Z

metrics/mapping.pl

+    0x14 => [0x30C,-550,0], # \check (combining)
+    0x15 => [0x306,-550,0], # \breve (combining)
+    0x16 => [0x304,-550,0], # \bar (combining)
+    0x17 => [0x30A,-608,0], # ring above (combining)


Hmm, I'm not sure. I ran the metrics as prescribed. Is there something I'm missing?

…eX into stacking-text-commands

ry-randall · 2017-12-13T00:29:58Z

@kevinbarabash This should be ready for another look. I opted to keep the existing unit testing method as it looked more verbose/readable. I did end up adding the tests you suggested however.

k4b7 · 2017-12-13T03:11:53Z

test/__snapshots__/katex-spec.js.snap


 exports[`An HTML font tree-builder should render \\textit{R} with the correct font 1`] = `"<span class=\\"katex\\"><span class=\\"katex-mathml\\"><math><semantics><mrow><mtext>R</mtext></mrow><annotation encoding=\\"application/x-tex\\">\\\\textit{R}</annotation></semantics></math></span><span class=\\"katex-html\\" aria-hidden=\\"true\\"><span class=\\"strut\\" style=\\"height:0.68333em;\\"></span><span class=\\"strut bottom\\" style=\\"height:0.68333em;vertical-align:0em;\\"></span><span class=\\"base\\"><span class=\\"mord text\\"><span class=\\"mord textit\\">R</span></span></span></span></span>"`;

+exports[`An HTML font tree-builder should render \\textsf{\\textbf{$\\mathrm{\\textsf{A}}$}} with the correct font 1`] = `"<span class=\\"katex\\"><span class=\\"katex-mathml\\"><math><semantics><mrow><mstyle scriptlevel=\\"0\\" displaystyle=\\"false\\"><mrow><mtext>A</mtext></mrow></mstyle></mrow><annotation encoding=\\"application/x-tex\\">\\\\textsf{\\\\textbf{$\\\\mathrm{\\\\textsf{A}}$}}</annotation></semantics></math></span><span class=\\"katex-html\\" aria-hidden=\\"true\\"><span class=\\"strut\\" style=\\"height:0.69444em;\\"></span><span class=\\"strut bottom\\" style=\\"height:0.69444em;vertical-align:0em;\\"></span><span class=\\"base\\"><span class=\\"mord text\\"><span class=\\"mord text\\"><span class=\\"mord\\"><span class=\\"mord text\\"><span class=\\"mord textsf textbf\\">A</span></span></span></span></span></span></span></span>"`;


I'll have to see if I can make it printout nice snapshots like the ones from mathml-spec.js.

k4b7 · 2017-12-13T03:24:08Z

test/__snapshots__/katex-spec.js.snap

-exports[`An HTML font tree-builder should render \\textsf{\\textit{R}G\\textbf{B}} with the correct font 1`] = `"<span class=\\"katex\\"><span class=\\"katex-mathml\\"><math><semantics><mrow><mtext>RGB</mtext></mrow><annotation encoding=\\"application/x-tex\\">\\\\textsf{\\\\textit{R}G\\\\textbf{B}}</annotation></semantics></math></span><span class=\\"katex-html\\" aria-hidden=\\"true\\"><span class=\\"strut\\" style=\\"height:0.69444em;\\"></span><span class=\\"strut bottom\\" style=\\"height:0.69444em;vertical-align:0em;\\"></span><span class=\\"base\\"><span class=\\"mord text\\"><span class=\\"mord text\\"><span class=\\"mord textsf textit\\">R</span></span><span class=\\"mord textsf\\">G</span><span class=\\"mord text\\"><span class=\\"mord textsf textbf\\">B</span></span></span></span></span></span>"`;
-
-exports[`An HTML font tree-builder should render \\textsf{R} with the correct font 1`] = `"<span class=\\"katex\\"><span class=\\"katex-mathml\\"><math><semantics><mrow><mtext>R</mtext></mrow><annotation encoding=\\"application/x-tex\\">\\\\textsf{R}</annotation></semantics></math></span><span class=\\"katex-html\\" aria-hidden=\\"true\\"><span class=\\"strut\\" style=\\"height:0.69444em;\\"></span><span class=\\"strut bottom\\" style=\\"height:0.69444em;vertical-align:0em;\\"></span><span class=\\"base\\"><span class=\\"mord text\\"><span class=\\"mord textsf\\">R</span></span></span></span></span>"`;
-


I totally get why you reverted these. Thanks for trying. I'll try to make the snapshots better in the future. The tests you have are good for now.

k4b7 · 2017-12-13T03:34:31Z

package.json

    "test": "check-dependencies && npm run lint && npm run flow && npm run jest",
    "build-css": "lessc --clean-css static/katex.less build/katex.css",
-    "prestart": "npm run build-css && npm run copy",
+    "prestart": "rimraf build && npm run build-css && npm run copy",


What was happening if build was still around?

https://stackoverflow.com/questions/20705677/mv-cannot-overwrite-directory-with-non-directory

Couldn't overwrite a directory with the same name.

k4b7 · 2017-12-13T03:36:25Z

src/Options.js

    textSize: number;
    phantom: boolean;
-    font: string | void;
+    fontFamily: string | void;


I think it's fine leaving this as is, but we might want to change this to fontFamily: string in a follow up diff to match fontWeight and fontShape. Could you add a TODO?

k4b7 · 2017-12-13T04:03:36Z

src/functions/text.js

-    "\\texttt": "mathtt", "\\textnormal": "mathrm", "\\textbf": "mathbf",
+const textFontFamilies = {
+    "\\text": undefined, "\\textrm": "textrm", "\\textsf": "textsf",
+    "\\texttt": "texttt", "\\textnormal": "textrm",


The reason for using mathrm, mathsf, etc. here is that that's what's used in fontMap in buildCommon. Not quite sure how this was working without updating fontMap, it'd be good to figure that out. Maybe we can get rid of it. We probably should separate the font family from whether we're in math mode or text mode. Maybe we should change this to:

"\\text": undefined, "\\textrm": "rm", "\\textsf": "sf", ...

Same thing for font weights and font shapes as we have both \\textbf and \\mathbf as well as \\textit and \\mathit commands.

We'll also need to update fontMap if it's still needed.

Font map is now strictly for math functions, and old text commands (\rm, \sf, etc.). The reason is the two are treated differently.

Text allows the fontFamily/fontWeight/fontShape to stack. In other words you can't simply use the last function to determine what the font is.

i.e. \textsf{\textbf{H}} combines the two to make a bold SansSerif H.

This means that the old fontMap either wont work well for text, or will need to be very verbose (contain every permutation of family, weight, shape).

Math and old font commands change the font as a whole, so the last one always wins

i.e. \mathsf{\mathbf{H}} will make a bold, non-SansSerif H.

The old fontMap works fine here.

The determination for which route to go with happens here:

https://github.com/Khan/KaTeX/pull/1009/files#diff-91067d2948380956002a57296faf3c18R208

Cool. I didn't realize that the math font commands behaved differently from the text commands. Thanks for the explanation.

k4b7

LGTM. Thanks for all of the revisions on this PR.

ry-randall · 2017-12-13T14:11:34Z

Thanks for the review @kevinbarabash

k4b7 · 2017-12-13T14:12:48Z

@rrandallcainc great work!

Ryan Randall and others added 8 commits October 2, 2017 13:34

Adding support for SansSerif-Bold

427481d

Updating to include SansSerif Italic.

f895ac4

Merge branch 'master' into SansSerif-Bold

cb512e7

Merge branch 'master' into SansSerif-Bold

b1cca00

Merge branch 'master' into SansSerif-Bold

e0a4795

WIP

c62d308

Working text stacking

cdd0511

More robust screenshot.

2bbf30e

ry-randall added the GH Review: review-needed label Dec 4, 2017

Don't want to break users :)

e593b33

k4b7 requested changes Dec 7, 2017

View reviewed changes

k4b7 added GH Review: needs-revision and removed GH Review: review-needed labels Dec 7, 2017

Updating per PR comments.

6113594

ry-randall commented Dec 12, 2017

View reviewed changes

Ryan Randall and others added 3 commits December 12, 2017 15:56

Merge branch 'master' into stacking-text-commands

84ca22b

Fixing Unicode and updating snapshots.

6cc85bc

Merge branch 'stacking-text-commands' of github.com:rrandallcainc/KaT…

dd798e3

…eX into stacking-text-commands

ry-randall added GH Review: review-needed and removed GH Review: needs-revision labels Dec 12, 2017

Ryan Randall added 2 commits December 12, 2017 17:47

Adding suggested tests.

8b2283c

Opting to use old method for unit testing.

b3686c2

k4b7 reviewed Dec 13, 2017

View reviewed changes

Adding TODO

5bbfef2

k4b7 reviewed Dec 13, 2017

View reviewed changes

Resolving merge conflicts.

1152e03

k4b7 approved these changes Dec 13, 2017

View reviewed changes

k4b7 added GH review: accepted and removed GH Review: review-needed labels Dec 13, 2017

k4b7 merged commit 50765a0 into KaTeX:master Dec 13, 2017

ry-randall deleted the stacking-text-commands branch December 13, 2017 14:11

ronkok mentioned this pull request Dec 31, 2017

Trouble with fonts in local build due to symlink #1052

Closed

ccorn mentioned this pull request Jan 27, 2018

Remove non-portable use of String method .includes #1097

Merged

ylemkimon mentioned this pull request Mar 16, 2018

Better documentation and dev UX for snapshot testing #1013

Closed


		exports[`An HTML font tree-builder should render \\textit{R} with the correct font 1`] = `"<span class=\\"katex\\"><span class=\\"katex-mathml\\"><math><semantics><mrow><mtext>R</mtext></mrow><annotation encoding=\\"application/x-tex\\">\\\\textit{R}</annotation></semantics></math></span><span class=\\"katex-html\\" aria-hidden=\\"true\\"><span class=\\"strut\\" style=\\"height:0.68333em;\\"></span><span class=\\"strut bottom\\" style=\\"height:0.68333em;vertical-align:0em;\\"></span><span class=\\"base\\"><span class=\\"mord text\\"><span class=\\"mord textit\\">R</span></span></span></span></span>"`;

		exports[`An HTML font tree-builder should render \\textsf{\\textbf{$\\mathrm{\\textsf{A}}$}} with the correct font 1`] = `"<span class=\\"katex\\"><span class=\\"katex-mathml\\"><math><semantics><mrow><mstyle scriptlevel=\\"0\\" displaystyle=\\"false\\"><mrow><mtext>A</mtext></mrow></mstyle></mrow><annotation encoding=\\"application/x-tex\\">\\\\textsf{\\\\textbf{$\\\\mathrm{\\\\textsf{A}}$}}</annotation></semantics></math></span><span class=\\"katex-html\\" aria-hidden=\\"true\\"><span class=\\"strut\\" style=\\"height:0.69444em;\\"></span><span class=\\"strut bottom\\" style=\\"height:0.69444em;vertical-align:0em;\\"></span><span class=\\"base\\"><span class=\\"mord text\\"><span class=\\"mord text\\"><span class=\\"mord\\"><span class=\\"mord text\\"><span class=\\"mord textsf textbf\\">A</span></span></span></span></span></span></span></span>"`;

Uh oh!

Comments

Conversation

ry-randall commented Dec 4, 2017

Uh oh!

k4b7 commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k4b7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ry-randall commented Dec 7, 2017

Uh oh!

ry-randall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ry-randall commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

k4b7 commented Dec 5, 2017 •

edited

Loading

k4b7 Dec 13, 2017 •

edited

Loading

ry-randall commented Dec 13, 2017 •

edited

Loading

ry-randall Dec 13, 2017 •

edited

Loading