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
Nested Math in Non-Default Text Fix #1111
Changes from 5 commits
479cfad
0b5f7bc
4e0de4b
d02c906
84c5796
f126266
f066b2b
cfb3631
db517d1
a7f684d
3d06083
c157de2
61ed028
8f8f8b1
953eab0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,9 +42,9 @@ export type OptionsData = { | |
size?: number; | ||
textSize?: number; | ||
phantom?: boolean; | ||
// TODO(#1009): Keep consistent with fontFamily/fontWeight. Ensure this has a | ||
// string value. | ||
fontFamily?: string | void; | ||
font?: string; | ||
oldTextFont?: boolean; | ||
fontFamily?: string; | ||
fontWeight?: string; | ||
fontShape?: string; | ||
sizeMultiplier?: number; | ||
|
@@ -64,7 +64,9 @@ class Options { | |
size: number; | ||
textSize: number; | ||
phantom: boolean; | ||
fontFamily: string | void; | ||
font: string; | ||
oldTextFont: boolean; | ||
fontFamily: string; | ||
fontWeight: string; | ||
fontShape: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking we'd have properties like:
and then the API, would look something like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, how would the old text fonts be handled? I.e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot about
If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think Really wish LaTeX fonts weren't so confusing :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As for the question of losing state, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'll give this more thought either later today or tomorrow. Thanks for the quick review. |
||
sizeMultiplier: number; | ||
|
@@ -82,7 +84,9 @@ class Options { | |
this.size = data.size || Options.BASESIZE; | ||
this.textSize = data.textSize || this.size; | ||
this.phantom = !!data.phantom; | ||
this.fontFamily = data.fontFamily; | ||
this.font = data.font || ""; | ||
this.oldTextFont = data.oldTextFont || false; | ||
this.fontFamily = data.fontFamily || ""; | ||
this.fontWeight = data.fontWeight || ''; | ||
this.fontShape = data.fontShape || ''; | ||
this.sizeMultiplier = sizeMultipliers[this.size - 1]; | ||
|
@@ -101,6 +105,8 @@ class Options { | |
textSize: this.textSize, | ||
color: this.color, | ||
phantom: this.phantom, | ||
font: this.font, | ||
oldTextFont: this.oldTextFont, | ||
fontFamily: this.fontFamily, | ||
fontWeight: this.fontWeight, | ||
fontShape: this.fontShape, | ||
|
@@ -192,12 +198,37 @@ class Options { | |
}); | ||
} | ||
|
||
/** | ||
* Create a new options object when switching from text to math mode. | ||
*/ | ||
withMathMode(): Options { | ||
return this.extend({ | ||
font: 'mathit', | ||
}); | ||
} | ||
|
||
/** | ||
* Creates a new options object with the given math font. | ||
* @type {[type]} | ||
*/ | ||
withFont(font: string): Options { | ||
return this.extend({ | ||
font, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we aren't resetting the values of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. Also, we can't safely reset the values of fontFamily/weight/etc. Consider the following: |
||
}); | ||
} | ||
|
||
withOldTextFont(): Options { | ||
return this.extend({ | ||
oldTextFont: true, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also reset the fontShape and fontWeight? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I actually thought the same thing until I broke the tests.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it not also retain the
It feels like we need to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great catch. This means I'll probably resolve #1112 in the process. |
||
} | ||
|
||
/** | ||
* Create a new options objects with the give font. | ||
*/ | ||
withFontFamily(fontFamily: ?string): Options { | ||
withFontFamily(fontFamily: string): Options { | ||
return this.extend({ | ||
fontFamily: fontFamily || this.fontFamily, | ||
fontFamily, | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ const mainitLetters = [ | |
const lookupSymbol = function( | ||
value: string, | ||
// TODO(#963): Use a union type for this. | ||
fontFamily: string, | ||
fontName: string, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A |
||
mode: Mode, | ||
): {value: string, metrics: ?CharacterMetrics} { | ||
// Replace the value with its replaced value from symbol.js | ||
|
@@ -42,7 +42,7 @@ const lookupSymbol = function( | |
} | ||
return { | ||
value: value, | ||
metrics: fontMetrics.getCharacterMetrics(value, fontFamily, mode), | ||
metrics: fontMetrics.getCharacterMetrics(value, fontName, mode), | ||
}; | ||
}; | ||
|
||
|
@@ -58,12 +58,12 @@ const lookupSymbol = function( | |
*/ | ||
const makeSymbol = function( | ||
value: string, | ||
fontFamily: string, | ||
fontName: string, | ||
mode: Mode, | ||
options?: Options, | ||
classes?: string[], | ||
): domTree.symbolNode { | ||
const lookup = lookupSymbol(value, fontFamily, mode); | ||
const lookup = lookupSymbol(value, fontName, mode); | ||
const metrics = lookup.metrics; | ||
value = lookup.value; | ||
|
||
|
@@ -80,7 +80,7 @@ const makeSymbol = function( | |
// TODO(emily): Figure out a good way to only print this in development | ||
typeof console !== "undefined" && console.warn( | ||
"No character metrics for '" + value + "' in style '" + | ||
fontFamily + "'"); | ||
fontName + "'"); | ||
symbolNode = new domTree.symbolNode(value, 0, 0, 0, 0, 0, classes); | ||
} | ||
|
||
|
@@ -117,7 +117,7 @@ const mathsym = function( | |
// text ordinal and is therefore not present as a symbol in the symbols | ||
// table for text, as well as a special case for boldsymbol because it | ||
// can be used for bold + and - | ||
if ((options && options.fontFamily && options.fontFamily === "boldsymbol") && | ||
if ((options && options.font && options.font === "boldsymbol") && | ||
lookupSymbol(value, "Main-Bold", mode).metrics) { | ||
return makeSymbol(value, "Main-Bold", mode, options, | ||
classes.concat(["mathbf"])); | ||
|
@@ -231,27 +231,28 @@ const makeOrd = function( | |
|
||
const classes = ["mord"]; | ||
|
||
const fontFamily = options.fontFamily; | ||
if (fontFamily) { | ||
// Math mode or Old font (i.e. \rm) | ||
const isFont = mode === "math" || options.oldTextFont; | ||
const fontOrFamily = isFont ? options.font : options.fontFamily; | ||
if (fontOrFamily) { | ||
let fontName; | ||
let fontClasses; | ||
if (fontFamily === "boldsymbol") { | ||
if (fontOrFamily === "boldsymbol") { | ||
const fontData = boldsymbol(value, mode, options, classes); | ||
fontName = fontData.fontName; | ||
fontClasses = [fontData.fontClass]; | ||
} else if (fontFamily === "mathit" || | ||
} else if (fontOrFamily === "mathit" || | ||
utils.contains(mainitLetters, value)) { | ||
const fontData = mathit(value, mode, options, classes); | ||
fontName = fontData.fontName; | ||
fontClasses = [fontData.fontClass]; | ||
} else if (fontFamily.indexOf("math") !== -1 || mode === "math") { | ||
// To support old font functions (i.e. \rm \sf etc.) or math mode. | ||
fontName = fontMap[fontFamily].fontName; | ||
fontClasses = [fontFamily]; | ||
} else if (isFont) { | ||
fontName = fontMap[fontOrFamily].fontName; | ||
fontClasses = [fontOrFamily]; | ||
} else { | ||
fontName = retrieveTextFontName(fontFamily, options.fontWeight, | ||
fontName = retrieveTextFontName(fontOrFamily, options.fontWeight, | ||
options.fontShape); | ||
fontClasses = [fontFamily, options.fontWeight, options.fontShape]; | ||
fontClasses = [fontOrFamily, options.fontWeight, options.fontShape]; | ||
} | ||
if (lookupSymbol(value, fontName, mode).metrics) { | ||
return makeSymbol(value, fontName, mode, options, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ export const makeText = function(text, mode) { | |
* Returns the math variant as a string or null if none is required. | ||
*/ | ||
const getVariant = function(group, options) { | ||
const font = options.fontFamily; | ||
const font = options.font; | ||
if (!font) { | ||
return null; | ||
} | ||
|
@@ -244,7 +244,10 @@ groupTypes.spacing = function(group) { | |
|
||
groupTypes.font = function(group, options) { | ||
const font = group.value.font; | ||
return buildGroup(group.value.body, options.withFontFamily(font)); | ||
const newOptions = group.value.oldTextFont ? | ||
options.withFont(font).withOldTextFont() : | ||
options.withFont(font); | ||
return buildGroup(group.value.body, newOptions); | ||
}; | ||
|
||
groupTypes.styling = function(group, options) { | ||
|
@@ -259,7 +262,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() : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would've expected There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow this. |
||
options.havingStyle(newStyle); | ||
|
||
const inner = buildExpression(group.value.value, newOptions); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -303,7 +303,12 @@ TextStacked: | |
\textsf{\textrm{\textbf{abc123}} \textbf{abc123} \textit{abc123}}\\ | ||
\textit{abc123 \textbf{abc123} \textsf{abc123}}\\ | ||
\end{matrix} | ||
TextWithMath: \text{for $a < b$ and $ c < d $}. | ||
TextWithMath: | ||
\begin{matrix} | ||
\text{for $a < b$ and $ c < d $}. \\ | ||
\textsf{for $a < b$ and $ c < d $}. \\ | ||
\textsf{for $a < b \textbf{ and } c < d $} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to have some tests for the behavior of old fonts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll update. |
||
\end{matrix} | ||
Unicode: \begin{matrix}\text{ÀàÇçÉéÏïÖöÛû} \\ \text{БГДЖЗЙЛФЦШЫЮЯ} \\ \text{여보세요} \\ \text{私はバナナです} \end{matrix} | ||
Units: | | ||
\begin{array}{ll} | ||
|
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.
How does
font
differ fromfontFamily
? Maybe add some comments and/or be more specific about the types.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 families are a group of fonts, not a specific font. I'll add a comment.