-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Stacking text commands #1009
Stacking text commands #1009
Conversation
@rrandallcainc I'll try to look at it this weekend. The screenshots look great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did the horizontal shifts come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure. I ran the metrics as prescribed. Is there something I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this copy/pasted from https://github.com/Khan/MathJax-dev/blob/master/fonts/OTF/TeX/makeFF#L1400-L1405 which is right on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes that's where I grabbed them from :). Thought you were saying there was something wrong with the shifts.
src/Options.js
Outdated
@@ -43,6 +43,7 @@ export type OptionsData = { | |||
textSize?: number; | |||
phantom?: boolean; | |||
font?: string | void; | |||
fontStyles?: Array<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been using string[]
style in other diffs. Can you change to that so we're consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (no longer necessary)
src/buildCommon.js
Outdated
return makeSymbol( | ||
value, "AMS-Regular", mode, options, classes.concat(["amsrm"])); | ||
value, fontName, mode, options, | ||
classes.concat(options.fontStyles, "amsrm")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should just pass fontStyles
to makeSymbol
so that the caller isn't responsible for calling retrieveTextFontName
and concatenating classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/Options.js
Outdated
@@ -60,6 +61,7 @@ class Options { | |||
textSize: number; | |||
phantom: boolean; | |||
font: string | void; | |||
fontStyles: Array<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing font
to baseFont
or fontFamily
would make it clearer that the two are necessary and work together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this. I like this approach better. Done.
src/buildCommon.js
Outdated
fontStyles: Array<string> | ||
): string { | ||
const baseFontName = retrieveBaseFontName(font); | ||
const fontStylesName = retrieveFontStylesName(fontStyles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/buildCommon.js
Outdated
fontStyles.indexOf("textbf") > -1 && (fontStylesName += "Bold"); | ||
fontStyles.indexOf("textit") > -1 && (fontStylesName += "Italic"); | ||
} else { | ||
fontStylesName += "Regular"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
test/katex-spec.js
Outdated
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>"); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great suggestions. Done.
I'm guessing you wanted:
\textsf{\textbf{$\mathrm{A}$}}
and
\textsf{\textbf{$\mathrm{\textsf{A}}$}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :(
test/katex-spec.js
Outdated
@@ -1590,31 +1590,38 @@ describe("An HTML font tree-builder", function() { | |||
|
|||
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>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of strange to see the mathrm
class being applied to text that's inside \textit
. Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@kevinbarabash Thanks for the quick feedback. Will take a look tomorrow or the wekeend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinbarabash Updated
src/buildCommon.js
Outdated
fontStyles: Array<string> | ||
): string { | ||
const baseFontName = retrieveBaseFontName(font); | ||
const fontStylesName = retrieveFontStylesName(fontStyles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/buildCommon.js
Outdated
fontStyles.indexOf("textbf") > -1 && (fontStylesName += "Bold"); | ||
fontStyles.indexOf("textit") > -1 && (fontStylesName += "Italic"); | ||
} else { | ||
fontStylesName += "Regular"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/Options.js
Outdated
@@ -43,6 +43,7 @@ export type OptionsData = { | |||
textSize?: number; | |||
phantom?: boolean; | |||
font?: string | void; | |||
fontStyles?: Array<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (no longer necessary)
src/Options.js
Outdated
@@ -60,6 +61,7 @@ class Options { | |||
textSize: number; | |||
phantom: boolean; | |||
font: string | void; | |||
fontStyles: Array<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this. I like this approach better. Done.
src/buildCommon.js
Outdated
return makeSymbol( | ||
value, "AMS-Regular", mode, options, classes.concat(["amsrm"])); | ||
value, fontName, mode, options, | ||
classes.concat(options.fontStyles, "amsrm")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/katex-spec.js
Outdated
@@ -1590,31 +1590,38 @@ describe("An HTML font tree-builder", function() { | |||
|
|||
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>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
test/katex-spec.js
Outdated
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>"); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great suggestions. Done.
I'm guessing you wanted:
\textsf{\textbf{$\mathrm{A}$}}
and
\textsf{\textbf{$\mathrm{\textsf{A}}$}}
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure. I ran the metrics as prescribed. Is there something I'm missing?
@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. |
@@ -10,6 +10,10 @@ exports[`An HTML font tree-builder should render \\textbf{R} with the correct fo | |||
|
|||
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>"`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to see if I can make it printout nice snapshots like the ones from mathml-spec.js.
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>"`; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"coverage": "jest --coverage", | ||
"copy": "cp -a static/. build/ && cp -a contrib build/", | ||
"clean": "rm -rf build/* node_modules/", | ||
"clean-install": "npm run clean && npm i", | ||
"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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was happening if build
was still around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/questions/20705677/mv-cannot-overwrite-directory-with-non-directory
Couldn't overwrite a directory with the same name.
@@ -59,7 +61,9 @@ class Options { | |||
size: number; | |||
textSize: number; | |||
phantom: boolean; | |||
font: string | void; | |||
fontFamily: string | void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"\\texttt": "mathtt", "\\textnormal": "mathrm", "\\textbf": "mathbf", | ||
const textFontFamilies = { | ||
"\\text": undefined, "\\textrm": "textrm", "\\textsf": "textsf", | ||
"\\texttt": "texttt", "\\textnormal": "textrm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I didn't realize that the math font commands behaved differently from the text commands. Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all of the revisions on this PR.
Thanks for the review @kevinbarabash |
@rrandallcainc great work! |
Per: #676
Sorry for the delay on this one. Been busy lately :(
Note: This does not include
Main-BoldItalic
.