Nested Math in Non-Default Text Fix #1111

ry-randall · 2018-01-28T19:38:10Z

Resolves:

…tests.

ry-randall · 2018-01-28T19:57:46Z

One difference with math vs text font commands, is text commands change one aspect of the font (weight, shape, or family). Math commands change all aspects of the font (weight, shape, and family). I'm wondering, probably as a cleanup item, if we'd want to leverage options.withFont() for math, and options.withFontFamily/Shape/Weight for text. Currently options.withFontFamily is being used for math fonts as well a text families.

Edit - Opened a separate issue here: #1112 for tracking purposes. Not sure it's a high priority though, nor do I think it should block this.

Edit 2 - This PR takes care of this.

kevinbarabash

Glad to see that the change was relatively straightforward.

kevinbarabash · 2018-01-28T21:34:24Z

src/Options.js

+    withMathMode(): Options {
+        return this.extend({
+            fontFamily: 'mathit',
+        });


Should this also reset the fontShape and fontWeight?

Good question. I actually thought the same thing until I broke the tests.

it("should render \\textsf{\\textbf{$\\mathrm{\\textsf{A}}$}} with the correct font", function() { const markup = katex.renderToString("\\textsf{\\textbf{$\\mathrm{\\textsf{A}}$}}"); expect(markup).toContain("<span class=\"mord textsf textbf\">A</span>"); });

Basically when going from text mode, to math mode, then back to text mode, it should retain the font weight/shape from the first time it was in text mode. Hope that makes sense.

Should it not also retain the fontFamily? What happens in this case?

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

It feels like we need to:

store font styles separate for math and text modes

store which mode we're currently in

Great catch. This means I'll probably resolve #1112 in the process.

kevinbarabash · 2018-01-28T21:35:47Z

src/buildMathML.js

@@ -259,7 +259,9 @@ groupTypes.styling = function(group, options) {
    };

    const newStyle = styleMap[group.value.style];
-    const newOptions = options.havingStyle(newStyle);
+    const newOptions = group.value.mathStart ?
+                       options.havingStyle(newStyle).withMathMode() :


I would've expected options.withMathMode() to be sufficient for this case.

Not sure I follow this. options.withMathMode() instead of options.havingStyle(newStyle)?

…TeX into Nested-Math-In-Text-Fix

kevinbarabash · 2018-01-29T19:06:09Z

src/Options.js

-    fontFamily: string | void;
+    font: string;
+    oldTextFont: boolean;
+    fontFamily: string;
    fontWeight: string;
    fontShape: string;


I was thinking we'd have properties like:

mathFont: string; textFontFamily: string; textFontWeight: string; textFontShape: string; mode: "text" | "math";

and then the API, would look something like:

.withMathFont(font: string); .withTextFontFamily(family: string); .withTextFontWeight(weight: string); .withTextFontShape(shape: string);

Hmm, how would the old text fonts be handled? I.e. \rm \it. They behave similar to math fonts in that they change all aspects of the font.

I forgot about \rm \it, but I think we could modify the suggested API in the following way:

.withTextFontFamily(family: string, isOldCommand: boolean = false); .withTextFontWeight(weight: string, isOldCommand: boolean = false); .withTextFontShape(shape: string, isOldCommand: boolean = false);

If isOldCommand is true, we would reset the other text font properties back to their defaults.

I don't think withTextFontFamily/Weight/Shape would make sense with old font commands. Old font commands effectively change all 3, not just one. Additionally you'd lose the state when you exit the old font mode (i.e. \textbf{{\it hello} world}). They behave much more similar to the math commands, which is why I opted to leave the naming conventions generic.

Really wish LaTeX fonts weren't so confusing :(

withTextFontFamily would look something like:

withTextFontFamily(family: string, isOldCommand: boolean) { if (isOldCommand) { this.extend({ fontFamily: fontFamily || this.fontFontFamily, fontWeight: "", fontStyle: "", }); } else { this.extend({ fontFamily: fontFamily || this.fontFamily, }); } };

As for the question of losing state, {\rm hello} creates a new options object when processing that group which shouldn't affect the options object for the \textbf group.

Hmm, I'll give this more thought either later today or tomorrow. Thanks for the quick review.

edemaine · 2018-01-30T21:17:47Z

#1073 points out some inconsistencies (compared to LaTeX) in the font size when using \llap. Will this PR help with this? Or perhaps unrelated.

ry-randall · 2018-01-31T01:38:09Z

@edemaine I don't believe this is related. This PR isn't concerned with the font sizing.

kevinbarabash · 2018-02-11T20:29:43Z

@rrandallcainc friendly ping to see where things are at with this PR.

ry-randall · 2018-02-11T21:16:31Z

@kevinbarabash I expect to get back to this early this week. Thanks for checking in.

codecov · 2018-02-19T18:08:46Z

Codecov Report

Merging #1111 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1111      +/-   ##
==========================================
+ Coverage   79.59%   79.61%   +0.02%     
==========================================
  Files          59       59              
  Lines        3871     3876       +5     
  Branches      648      652       +4     
==========================================
+ Hits         3081     3086       +5     
  Misses        656      656              
  Partials      134      134

Impacted Files	Coverage Δ
src/functions/operatorname.js	`45.16% <ø> (ø)`	⬆️
src/buildMathML.js	`93.5% <100%> (ø)`	⬆️
src/functions/text.js	`100% <100%> (ø)`	⬆️
src/functions/font.js	`95.45% <100%> (+0.45%)`	⬆️
src/functions/styling.js	`100% <100%> (ø)`	⬆️
src/Options.js	`92.45% <100%> (+0.29%)`	⬆️
src/buildCommon.js	`84.97% <100%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b341034...953eab0. Read the comment docs.

…TeX into Nested-Math-In-Text-Fix

ry-randall · 2018-02-19T18:35:06Z

@kevinbarabash I apologize for the delay on this one. This should be ready for another look.

kevinbarabash

The more I look at the implementation of makOrd the more I don't like it, but this PR was meant to fix a particular problem in the behavior, not refactor all of the code. I would like to tackle that eventually, but not right now.

For now this is looking pretty good. It could use a some comments and/or better typing to reduce confusion between font, fontFamily, and fontName. After that this should be good to go.

kevinbarabash · 2018-02-19T20:51:56Z

src/buildCommon.js

@@ -33,7 +33,7 @@ const mainitLetters = [
 const lookupSymbol = function(
    value: string,
    // TODO(#963): Use a union type for this.
-    fontFamily: string,
+    fontName: string,


How does fontFamily in Options.js differ from fontName here?

A fontFamily applies to a group of fonts. For example, the SansSerif family can be: regular, bold, or italic. In order to properly lookup the symbol, we need a specific font, not a font family.

kevinbarabash · 2018-02-19T20:55:50Z

src/Options.js

-    // string value.
-    fontFamily?: string | void;
+    font?: string;
+    fontFamily?: string;


How does font differ from fontFamily? Maybe add some comments and/or be more specific about the types.

Font families are a group of fonts, not a specific font. I'll add a comment.

kevinbarabash · 2018-02-19T20:59:24Z

src/Options.js

        return this.extend({
-            fontFamily: fontFamily || this.fontFamily,
+            font,


Since we aren't resetting the values of fontFamily, fontWeight, etc. I assume that font takes precedence.

Correct. Also, we can't safely reset the values of fontFamily/weight/etc.

Consider the following: \textbf{$\mathsf{\textsf{a}$}. This should render as a bold, sans-serif "a", but if we reset, we would get a regular sans-serif "a"

kevinbarabash · 2018-02-19T21:08:39Z

test/__snapshots__/katex-spec.js.snap

@@ -88,7 +88,7 @@ exports[`An implicit group parser within optional groups should work with \\colo
 ]
 `;

-exports[`An implicit group parser within optional groups should work with sizing commands: \\sqrt[\\small 3]{x} 1`] = `
+exports[`An implicit group parser within optional groups should work with old font functions: \\sqrt[\\tt 3]{x} 1`] = `


It looks like the order of these changed, but the parse trees look the same.

I fixed a typo, assuming that's why?

Sorry, I didn't mean to suggest that anything needed changing here.

kevinbarabash · 2018-02-19T21:13:17Z

test/screenshotter/ss_data.yaml

+  \begin{matrix}
+      \text{for $a < b$ and $ c < d $}. \\
+      \textsf{for $a < b$ and $ c < d $}. \\
+      \textsf{for $a < b \textbf{ and } c < d $}


It would be good to have some tests for the behavior of old fonts.

I'll update.

ry-randall · 2018-02-19T22:34:35Z

@kevinbarabash Updated.

Definitely agree that the makeOrd implementation could be cleaned up. I think it's current state was a side effect of the assumption that math/text fonts behaved similar. That being said, it looks like a lot of solid cleanup has been done around fonts recently (i.e. pulling out the oldFonts from the parser, separating the text functions, etc.). I don't think the cleanup for makeOrd should be too bad.

kevinbarabash · 2018-02-19T22:47:53Z

@rrandallcainc glad to see this PR finally merged. Thanks for seeing it through.

Fixing nested math in non-default text. Added appropriate screenshot …

479cfad

…tests.

ry-randall requested a review from kevinbarabash January 28, 2018 19:38

Adding appropriate logic for buildMathML and resolving tests.

0b5f7bc

ry-randall added the GH Review: review-needed label Jan 28, 2018

Merge branch 'master' into Nested-Math-In-Text-Fix

4e0de4b

kevinbarabash reviewed Jan 28, 2018

View reviewed changes

kevinbarabash added GH Review: needs-revision and removed GH Review: review-needed labels Jan 29, 2018

Ryan Randall added 2 commits January 29, 2018 13:37

Addressing Kevin's comment. Adding tests.

d02c906

Merge branch 'Nested-Math-In-Text-Fix' of github.com:rrandallcainc/Ka…

84c5796

…TeX into Nested-Math-In-Text-Fix

ry-randall added GH Review: review-needed and removed GH Review: needs-revision labels Jan 29, 2018

Ryan Randall and others added 3 commits January 29, 2018 13:40

Merge branch 'master' into Nested-Math-In-Text-Fix

f126266

Adding appropriate screenshots.

f066b2b

Merge branch 'Nested-Math-In-Text-Fix' of github.com:rrandallcainc/Ka…

cfb3631

…TeX into Nested-Math-In-Text-Fix

kevinbarabash reviewed Jan 29, 2018

View reviewed changes

ry-randall added GH Review: needs-revision and removed GH Review: review-needed labels Jan 29, 2018

Resolving conflicts, and responding to PR comments.

db517d1

Ryan Randall and others added 4 commits February 19, 2018 13:10

Removing unnecessary data, adding consistent naming convention.

a7f684d

Merge branch 'master' into Nested-Math-In-Text-Fix

3d06083

Cleanup

c157de2

Merge branch 'Nested-Math-In-Text-Fix' of github.com:rrandallcainc/Ka…

61ed028

…TeX into Nested-Math-In-Text-Fix

ry-randall added the GH Review: review-needed label Feb 19, 2018

ry-randall removed the GH Review: needs-revision label Feb 19, 2018

Handling possible edge cases.

8f8f8b1

kevinbarabash requested changes Feb 19, 2018

View reviewed changes

kevinbarabash added GH Review: needs-revision and removed GH Review: review-needed labels Feb 19, 2018

Updating per PR comments

953eab0

kevinbarabash approved these changes Feb 19, 2018

View reviewed changes

kevinbarabash merged commit 7de91f7 into KaTeX:master Feb 19, 2018

ry-randall mentioned this pull request Feb 19, 2018

Cleanup makeOrd Function #1175

Open

ry-randall deleted the Nested-Math-In-Text-Fix branch February 19, 2018 23:57

ronkok mentioned this pull request Mar 22, 2018

Support some Unicode Mathematical Alphanumeric Symbols #1232

Merged

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested Math in Non-Default Text Fix #1111

Nested Math in Non-Default Text Fix #1111

ry-randall commented Jan 28, 2018 •

edited

ry-randall commented Jan 28, 2018 •

edited

kevinbarabash left a comment

kevinbarabash Jan 28, 2018

ry-randall Jan 28, 2018

kevinbarabash Jan 29, 2018

ry-randall Jan 29, 2018

kevinbarabash Jan 28, 2018

ry-randall Jan 28, 2018

kevinbarabash Jan 29, 2018

ry-randall Jan 29, 2018 •

edited

kevinbarabash Jan 29, 2018 •

edited

ry-randall Jan 29, 2018 •

edited

kevinbarabash Jan 29, 2018

ry-randall Jan 29, 2018

edemaine commented Jan 30, 2018

ry-randall commented Jan 31, 2018

kevinbarabash commented Feb 11, 2018

ry-randall commented Feb 11, 2018

codecov bot commented Feb 19, 2018 •

edited

ry-randall commented Feb 19, 2018

kevinbarabash left a comment

kevinbarabash Feb 19, 2018

ry-randall Feb 19, 2018

kevinbarabash Feb 19, 2018

ry-randall Feb 19, 2018

kevinbarabash Feb 19, 2018

ry-randall Feb 19, 2018

kevinbarabash Feb 19, 2018

ry-randall Feb 19, 2018

kevinbarabash Feb 19, 2018

kevinbarabash Feb 19, 2018

ry-randall Feb 19, 2018

ry-randall commented Feb 19, 2018

kevinbarabash commented Feb 19, 2018

Nested Math in Non-Default Text Fix #1111

Nested Math in Non-Default Text Fix #1111

Conversation

ry-randall commented Jan 28, 2018 • edited

ry-randall commented Jan 28, 2018 • edited

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry-randall Jan 29, 2018 • edited

Choose a reason for hiding this comment

kevinbarabash Jan 29, 2018 • edited

Choose a reason for hiding this comment

ry-randall Jan 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edemaine commented Jan 30, 2018

ry-randall commented Jan 31, 2018

kevinbarabash commented Feb 11, 2018

ry-randall commented Feb 11, 2018

codecov bot commented Feb 19, 2018 • edited

Codecov Report

ry-randall commented Feb 19, 2018

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry-randall commented Feb 19, 2018

kevinbarabash commented Feb 19, 2018

ry-randall commented Jan 28, 2018 •

edited

ry-randall commented Jan 28, 2018 •

edited

ry-randall Jan 29, 2018 •

edited

kevinbarabash Jan 29, 2018 •

edited

ry-randall Jan 29, 2018 •

edited

codecov bot commented Feb 19, 2018 •

edited