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

Fix all AMS mathord symbols #618

Merged
merged 11 commits into from
Jan 16, 2017
85 changes: 56 additions & 29 deletions src/buildCommon.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var fontMetrics = require("./fontMetrics");
var symbols = require("./symbols");
var utils = require("./utils");

/* No longer needed; see mathit below.
var greekCapitals = [
"\\Gamma",
"\\Delta",
Expand All @@ -22,14 +23,30 @@ var greekCapitals = [
"\\Psi",
"\\Omega"
];
*/

// The following have to be loaded from Main-Italic font, using class mainit
var mainitLetters = [
"\u0131", // dotless i, \imath
"\u0237", // dotless j, \jmath
"\u00a3" // \pounds
"\\imath", // dotless i
"\\jmath", // dotless j
"\\pounds" // pounds symbol
];

/**
* Looks up the given symbol in fontMetrics, after applying any symbol
* replacements defined in symbol.js
*/
var lookupSymbol = function(value, fontFamily, mode) {
// Replace the value with its replaced value from symbol.js
if (symbols[mode][value] && symbols[mode][value].replace) {
value = symbols[mode][value].replace;
}
return {
value: value,
metrics: fontMetrics.getCharacterMetrics(value, fontFamily)
};
};

/**
* Makes a symbolNode after translation via the list of symbols in symbols.js.
* Correctly pulls out metrics for the character, and optionally takes a list of
Expand All @@ -40,12 +57,9 @@ var mainitLetters = [
* should if present come first in `classes`.
*/
var makeSymbol = function(value, fontFamily, mode, options, classes) {
// Replace the value with its replaced value from symbol.js
if (symbols[mode][value] && symbols[mode][value].replace) {
value = symbols[mode][value].replace;
}

var metrics = fontMetrics.getCharacterMetrics(value, fontFamily);
var lookup = lookupSymbol(value, fontFamily, mode);
var metrics = lookup.metrics;
value = lookup.value;

var symbolNode;
if (metrics) {
Expand Down Expand Up @@ -100,10 +114,19 @@ var mathsym = function(value, mode, options, classes) {
*/
var mathDefault = function(value, mode, options, classes, type) {
if (type === "mathord") {
return mathit(value, mode, options, classes);
var fontLookup = mathit(value, mode, options, classes);
return makeSymbol(value, fontLookup.fontName, mode, options,
classes.concat([fontLookup.fontClass]));
} else if (type === "textord") {
return makeSymbol(
value, "Main-Regular", mode, options, classes.concat(["mathrm"]));
var font = symbols[mode][value] && symbols[mode][value].font;
if (font === "ams") {
return makeSymbol(
value, "AMS-Regular", mode, options, classes.concat(["amsrm"]));
} else { // if (font === "main") {
return makeSymbol(
value, "Main-Regular", mode, options,
classes.concat(["mathrm"]));
}
} else {
throw new Error("unexpected type: " + type + " in mathDefault");
}
Expand All @@ -116,13 +139,19 @@ var mathit = function(value, mode, options, classes) {
if (/[0-9]/.test(value.charAt(0)) ||
// glyphs for \imath and \jmath do not exist in Math-Italic so we
// need to use Main-Italic instead
utils.contains(mainitLetters, value) ||
utils.contains(greekCapitals, value)) {
return makeSymbol(
value, "Main-Italic", mode, options, classes.concat(["mainit"]));
utils.contains(mainitLetters, value)) {
// For greek capitals, we have a choice, but Math-Italic
// seems closer to LaTeX.
//|| utils.contains(greekCapitals, value)) {
Copy link
Member

@kevinbarabash kevinbarabash Jan 15, 2017

Choose a reason for hiding this comment

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

Is this code supposed to be commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant to ask you about this. Yes, it seems so:

Greek capital letters are in both fonts, Main-Italic and Math-Italic. So which should we use? The master code claims to use Main-Italic for Greek capital letters. But if you try the demo with \mathit{\Omega\imath}, and inspect, you'll see that the \Omega gets class mathit while \imath gets mainit:

<span class="mord mathit" style="margin-right: 0.05017em;">Ω</span>
<span class="mord mainit">ı</span>

In fact, the master code ends up calling mathit() with a single-unicode-character string (Ω), while greekCapitals is defined in terms of the macro name ("\Omega"). When my code fixed this, I ended up changing Greek capital letters to use Main-Italic font, which caused some tests to fail. I confirmed with texcmp that Math-Italic seems slightly closer, so kept it there.

Let me know if you'd like this code changed at all...

Copy link
Member

Choose a reason for hiding this comment

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

In that case, can you remove the commented out code? TBH, I'm not sure why we have the same glyphs duplicated in different fonts. It would be nice if we could avoid this and save a few bytes. I'm curious to find out why the rendering looks different even though the glyphs are the same. Maybe the advance or skew metrics for the Greek capitals are differ between fonts. I think it's safe to investigate this separate from this pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I remove the list of greekCapitals as well? It's currently commented out (to prevent lint from complaining).

Copy link
Member

Choose a reason for hiding this comment

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

Yes please.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

return {
fontName: "Main-Italic",
fontClass: "mainit"
};
} else {
return makeSymbol(
value, "Math-Italic", mode, options, classes.concat(["mathit"]));
return {
fontName: "Math-Italic",
fontClass: "mathit"
};
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the doc string for mathit now that it no longer creates a symbol?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!


Expand All @@ -132,24 +161,22 @@ var mathit = function(value, mode, options, classes) {
var makeOrd = function(group, options, type) {
var mode = group.mode;
var value = group.value;
if (symbols[mode][value] && symbols[mode][value].replace) {
value = symbols[mode][value].replace;
}

var classes = ["mord"];

var font = options.font;
if (font) {
var fontLookup;
if (font === "mathit" || utils.contains(mainitLetters, value)) {
return mathit(value, mode, options, classes);
fontLookup = mathit(value, mode, options, classes);
} else {
var fontName = fontMap[font].fontName;
if (fontMetrics.getCharacterMetrics(value, fontName)) {
return makeSymbol(
value, fontName, mode, options, classes.concat([font]));
} else {
return mathDefault(value, mode, options, classes, type);
}
fontLookup = fontMap[font];
}
if (lookupSymbol(value, fontLookup.fontName, mode).metrics) {
return makeSymbol(value, fontLookup.fontName, mode, options,
classes.concat([fontLookup.fontClass || font]));
} else {
return mathDefault(value, mode, options, classes, type);
}
} else {
return mathDefault(value, mode, options, classes, type);
Expand Down
Binary file modified test/screenshotter/images/Symbols1-chrome.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/screenshotter/images/Symbols1-firefox.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.