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

feat: Support bold Fraktur #3777

Merged
merged 7 commits into from Oct 2, 2023
Merged

feat: Support bold Fraktur #3777

merged 7 commits into from Oct 2, 2023

Conversation

ronkok
Copy link
Collaborator

@ronkok ronkok commented Jan 18, 2023

Support bold Fraktur. Resolves #3776.

Note: KaTeX has a font for bold Fraktur, but no font metrics. That is why I left bold Fraktur out of the original PR for wide-character Unicode support. In this PR, I evade the lack of metrics by using the font metrics for regular Fraktur, not bold Fraktur.

@ronkok
Copy link
Collaborator Author

ronkok commented Jan 18, 2023

The second commit makes this PR more general in addressing the complaint of issue #3776. The results of function wideCharacterFont() are no longer used when inapplicable. That avoids all the bad error messages that would come from inapplicable use.

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #3777 (29ee8da) into main (4f1d916) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3777   +/-   ##
=======================================
  Coverage   92.98%   92.99%           
=======================================
  Files          91       91           
  Lines        6773     6779    +6     
  Branches     1575     1576    +1     
=======================================
+ Hits         6298     6304    +6     
  Misses        437      437           
  Partials       38       38           
Files Coverage Δ
src/buildCommon.js 96.94% <100.00%> (+0.03%) ⬆️
src/symbols.js 100.00% <100.00%> (ø)
src/wide-character.js 70.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

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

Copy link
Member

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay in review here. Slowly getting back into things...

Overall, this seems like a good fix to bold Fraktur, so this PR should be probably be merged. However, I noticed one thing more generally about Fraktur support:

I just tested and \text{𝔄-ℨ} generates an error, because \u212D (Z), \u210C (H), and \u2128 (Z) are macro'd in src/macros.js to use \mathfrak but sometimes we want \textfrak. (Not sure why this doesn't generate an error when building the documentation. Perhaps it does and it's being silently ignored.)

Perhaps this is legacy code and can simply be removed?

KaTeX/src/macros.js

Lines 256 to 258 in 5dd9bc4

defineMacro("\u212D", "\\mathfrak{C}"); // Fraktur
defineMacro("\u210C", "\\mathfrak{H}");
defineMacro("\u2128", "\\mathfrak{Z}");

@edemaine
Copy link
Member

P.S. I also looked into generating actual metrics for the bold Fraktur font. (Part of the delay.) My attempts failed, though, so I think this should go as is; we can consider adding them later if I figure out how metrics get built.

const [wideFontName, wideFontClass] = text.charCodeAt(0) !== 0xD835
? ["", ""]
: wideCharacterFont(text, mode);
if (wideFontName.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to avoid the array creation in the negative case. How about this:

let wideFontName, wideFontClass;
if (text.charCodeAt(0) === 0xD835) {
  [wideFontName, wideFontClass] = wideCharacterFont(text, mode);
}
if (wideFontName) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like it.

@edemaine
Copy link
Member

Perhaps this is legacy code and can simply be removed?

@ronkok Do you agree with this assessment? If so, I can remove the code and make the other requested change.

@ronkok
Copy link
Collaborator Author

ronkok commented Jun 25, 2023

Perhaps this is legacy code and can simply be removed?

I don't think that is correct. Most Fractur characters are outside the Unicode Basic Multilingual Plane (BMP). They are addressed by the wide character function modified in this PR.

But a few Fractur characters are in the BMP. KaTeX defines them in these macros.

KaTeX/src/macros.js

Lines 247 to 258 in ad03d1e

// Characters omitted from Unicode range 1D400–1D7FF
defineMacro("\u212C", "\\mathscr{B}"); // script
defineMacro("\u2130", "\\mathscr{E}");
defineMacro("\u2131", "\\mathscr{F}");
defineMacro("\u210B", "\\mathscr{H}");
defineMacro("\u2110", "\\mathscr{I}");
defineMacro("\u2112", "\\mathscr{L}");
defineMacro("\u2133", "\\mathscr{M}");
defineMacro("\u211B", "\\mathscr{R}");
defineMacro("\u212D", "\\mathfrak{C}"); // Fraktur
defineMacro("\u210C", "\\mathfrak{H}");
defineMacro("\u2128", "\\mathfrak{Z}");

I suppose I could have instead defined them in symbols.js. That would probably be better than the macro method. It would be easier in symbols.js to define them both as a mathord and as a textord. The lack of a textord definition is causing the error.

I think a symbols.js definition of the BMP characters should be located below the loop where wide character Frakturs are defined.

KaTeX/src/symbols.js

Lines 815 to 817 in ad03d1e

wideChar = String.fromCharCode(0xD835, 0xDD04 + i); // A-Z a-z Fractur
defineSymbol(math, main, mathord, ch, wideChar);
defineSymbol(text, main, textord, ch, wideChar);

Then the BMP definitions would overwrite the first definition.

@ronkok ronkok merged commit 240d5ae into KaTeX:main Oct 2, 2023
9 checks passed
@ronkok ronkok deleted the boldFraktur branch October 2, 2023 20:07
KaTeX-bot added a commit that referenced this pull request Oct 2, 2023
## [0.16.9](v0.16.8...v0.16.9) (2023-10-02)

### Features

* Support bold Fraktur ([#3777](#3777)) ([240d5ae](240d5ae))
@KaTeX-bot
Copy link
Member

🎉 This PR is included in version 0.16.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Font metrics not found for font" error
3 participants