Added support for bold italic symbols#1011
Conversation
|
Hey @aterenin, Thanks for the PR! Mind signing our Contributor License Agreement? When you've done so, go ahead and comment Yours truly, |
|
[clabot:check] |
|
CLA signature looks good 👍 |
|
@aterenin thanks for the PR. I thought for some reason we didn't have the fonts or the metrics but apparently we have both. This could probably use a screenshot test. Please see https://github.com/Khan/KaTeX/blob/master/CONTRIBUTING.md#screenshot-tests for details. |
|
Tests implemented. I'm currently in the middle of travel (coded up this PR while on a flight) so I'm not equipped to install docker to run the screenshotter at the moment. It'd be great if someone could run it and add to the PR, otherwise I'll handle it later. |
test/katex-spec.js
Outdated
| expect(markup).toContain("<mi mathvariant=\"bold-italic\">\u03A9</mi>"); // \Omega | ||
| expect(markup).toContain("<mi>\u0131</mi>"); // \imath | ||
| expect(markup).toContain("<mo>+</mo>"); | ||
| }); |
There was a problem hiding this comment.
We've started using snapshot tests for testing MathML markup, see mathml-spec.js. Can you add a test there instead?
There was a problem hiding this comment.
I do not understand what those tests do or how they work. There are no similar tests implemented for \mathbf or any similar command. Could you either provide reference on how to implement that or accept as-is and move to mathml-spec.js later, when \mathbf is moved there too?
There was a problem hiding this comment.
it("should render \\boldsymbol{" + contents + "} with the correct mathvariants", () => {
const tex = "\\boldsymbol{" + contents + "}";
const tree = getParsed(tex);
const markup = buildMathML(tree, tex, defaultOptions).toMarkup();
expect(markup).toMatchSnapshot();
});
You'll need to run the tests once locally in order to update the katex-spec.js.snap. Include the changes to that file in this diff and you should be good.
The tests work by comparing the output of future runs agains the snapshot. The snapshot can also be regenerated. I should add more detail to https://github.com/Khan/KaTeX/blob/master/CONTRIBUTING.md#jest-tests along with an npm script to regenerate the snapshots when things change.
test/screenshotter/ss_data.yaml
Outdated
| \end{array} | ||
| BinomTest: \dbinom{a}{b}\tbinom{a}{b}^{\binom{a}{b}+17} | ||
| BoldSpacing: \mathbf{A}^2+\mathbf{B}_3*\mathscr{C}' | ||
| BoldSymbol: \boldsymbol{\omega}+\boldsymbol{\Omega}+\boldsymbol{A}+\boldsymbol{x} |
There was a problem hiding this comment.
You'll need to generate screenshots and include those files as well. Let me know if you have issues running make screenshots.
There was a problem hiding this comment.
make screenshots does not work. I get the following:
/Users/aterenin/Git/KaTeX/node_modules/selenium-webdriver/lib/promise.js:654
throw error;
^
Error: Timed out waiting for the WebDriver server at http://127.0.0.1:49315/hub
at onError (/Users/aterenin/Git/KaTeX/node_modules/selenium-webdriver/http/util.js
:87:11)
...
There was a problem hiding this comment.
Figured out the problem: I previously didn't install the correct docker.
Screenshots added and pushed.
There was a problem hiding this comment.
@aterenin cool. I take it npm install docker installs a client. I should update the README.md for the Screenshot to point to link docker's download page or homepage so there's less confusion.
There was a problem hiding this comment.
It might be good to include some other symbols in this such as \sum and \int.
|
Great stuff! Would it be possible to add |
|
@jeanm |
|
Not in LaTeX, true, but do these differences still apply here? And even in LaTeX, the differences are so minimal that they don't matter in the majority of real-world use cases I see in papers in my field. It would be annoying to have to rewrite a bunch of formulae that use |
|
@jeanm we'd like avoid implementing commands where the semantics don't match LaTeX's as much as possible. See https://github.com/khan/KaTeX#rendering-options for how to define your own macros. |
|
@aterenin Awesome feature addition! Thanks for working on this. In #585, I concluded that we should aim to support |
|
I guess I'm okay with the alias too as long as we communicate the short coming and at the very least have an issue in github to track the issue with which command is non-compliant. |
|
There's something to be said for simplicity, especially given people can define a macro for I can easily add an alias for |
It's sounds like the general consensus to add the alias. |
|
Alias added. |
| <mo> | ||
| + | ||
| </mo> | ||
| </mrow> |
There was a problem hiding this comment.
I think everything in this row should have mathvariant="bold-italic" set.
There was a problem hiding this comment.
Are you sure? These include the imaginary number symbol, as well as the plus symbol, for which mathvariant="bold" is not set even for \mathbf.
There was a problem hiding this comment.
There was a problem hiding this comment.
Turns out the same situation occurs for \mathit - using that as a template, I was able to implement \imath and 2. + is still not correct, because for some reason the font doesn't get applied to it, will look at that shortly.
There was a problem hiding this comment.
Done. Please see new screenshots that have been pushed.
|
Thanks for adding the snapshot test. Almost there, just need to add |
|
Test updated. |
src/buildCommon.js
Outdated
| "\\imath", // dotless i | ||
| "\\jmath", // dotless j | ||
| "+", // plus symbol | ||
| "-", // minus symbol |
There was a problem hiding this comment.
I looked at the font file for KaTeX_MainBold and realized that there are more symbols in it that we should show in bold, e.g. =, <, >, etc. We could use lookupSymbol to determine whether we have a bold variant or not, that way we can include all of them without the extra table.
There was a problem hiding this comment.
Agreed; this should ideally not be hard coded.
There was a problem hiding this comment.
I've removed the hardcoded symbols - this was done for mathit so I copied the approach - and am now using lookupSymbol.
src/buildCommon.js
Outdated
| ): {| fontName: string, fontClass: string |} { | ||
| if (/[0-9]/.test(value.charAt(0)) || | ||
| // glyphs for \imath and \jmath do not exist in Math-BoldItalic so | ||
| // we need to use Main-BoldItalic instead |
| // "mathit" and "boldsymbol" are missing because they require the use of two | ||
| // fonts: Main-Italic and Math-Italic for "mathit", and Math-BoldItalic and | ||
| // Main-Bold for "boldsymbol". This is handled by a special case in makeOrd | ||
| // which ends up calling mathit and boldsymbol. |
k4b7
left a comment
There was a problem hiding this comment.
LGTM. Thanks for all the revisions.
|
@aterenin thanks for fixing my lint. 😅 |
|
No worries. I pushed a 1-character fix to make CI happy. Any idea when this'll make it into the release? With this added, I can now switch the live version of my blog from MathJax to KaTeX. Only thing missing for me now is auto equation numbering, which I've no doubt will make it in eventually. And if I'm allowed to wish for things MathJax can't do |
|
@aterenin thanks for this PR. |
|
I'm using KaTeX from the CDN, which is using version 0.9.0-alpha2. Am I correct that support for bolded italics has not yet been included in the CDN? Thank you! |
|
@stapeleliz that's right. I need to publish a new release. |






This adds support for
\boldsymbol, as requested in #633 and #585.It uses the bold italic fonts already present in KaTeX. It works with both Greek and Latin letters.