-
-
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
update fonts and metrics so that accents are positioned correctly #1094
Conversation
src/symbols.js
Outdated
defineSymbol(text, main, accent, "\u02ca", "\\'"); // acute | ||
defineSymbol(text, main, accent, "\u02cb", "\\`"); // grave | ||
defineSymbol(text, main, accent, "\u00b4", "\\'"); // acute | ||
defineSymbol(text, main, accent, "\u0060", "\\`"); // grave |
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 nice if all of these were in the \u02c0
to \u02cf
range.
@@ -666,7 666,7 @@ defineSymbol(text, main, accent, "\u02d9", "\\."); // dot above | |||
defineSymbol(text, main, accent, "\u02da", "\\r"); // ring above | |||
defineSymbol(text, main, accent, "\u02c7", "\\v"); // caron | |||
defineSymbol(text, main, accent, "\u00a8", '\\"'); // diaresis |
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.
Same.
It looks like I broke |
\text{\'\i} & \text{\.\i} & \text{\`\i} & \text{\"\i} & \text{\H\i} & \text{\r\i} \\ | ||
\text{\'\j} & \text{\.\j} & \text{\`\j} & \text{\"\j} & \text{\H\j} & \text{\r\j} \\ | ||
\text{\'a} & \text{\.a} & \text{\`a} & \text{\"a} & \text{\H{a}} & \text{\r{a}} \\ | ||
\text{\'A} & \text{\.A} & \text{\`A} & \text{\"A} & \text{\H{A}} & \text{\r{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.
KA has specific bug reports from translators about e with a double acute accent and turkish dotless i and dotted captial I, so if you're willing to add those here as well that would be great.
@@ -42,7 42,7 @@ exports[`A MathML builder accents turn into <mover accent="true"> in MathML 1`] | |||
e | |||
</mi> | |||
<mo> | |||
´ |
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.
This changed b/c we switched some of the accents to be in the "Spacing Modifier Characters" Unicode block. See https://en.wikipedia.org/wiki/Spacing_Modifier_Letters.
test/screenshotter/ss_data.yaml
Outdated
\text{\'\j} & \text{\.\j} & \text{\`\j} & \text{\"\j} & \text{\H\j} & \text{\r\j} \\ | ||
\text{\'a} & \text{\.a} & \text{\`a} & \text{\"a} & \text{\H{a}} & \text{\r{a}} \\ | ||
\text{\'A} & \text{\.A} & \text{\`A} & \text{\"A} & \text{\H{A}} & \text{\r{A}} \\ | ||
\text{\.I} & \text{\H e} & \text{\i ı} |
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.
@davidflanagan would you like me to change \text{\H e}
to \text{\H ee̋ }
?
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.
That would be great, if you don't mind. And maybe add an explicit dotted capital I as well to make the whole row the same. Thanks!
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 fixing this, Kevin! (And thanks for talking me through how fonts work in KaTeX so I could understand the patch)
It is pretty confusing that when we've got an input character like á, we map that (in unicodeSymbols) to \u0061\u0301 (using a combining acute accent) and then (in Parser.js) break those apart and map (from unicodeAccents.js) \u301 to ' or \acute, and then somewhere map those TeX functions to the non-combining \u02ca form of the acute accent.
It works though, so I'm not complaining. Maybe file an issue to write some documentation about how this is all handled though.
Also, src/domTree.js has some special cases for forms of lowercase letter i. I'm not sure why those are there, but they also use the combining form of the accents rather than the non-combining form. Please check that code still makes sense with this change before merging.
"176": [0, 0.69444, 0, 0, 0.86944], | ||
"177": [0.13333, 0.63333, 0, 0, 0.89444], | ||
"180": [0, 0.69444, 0, 0, 0.575], | ||
"198": [0, 0.68611, 0, 0, 1.04166], |
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.
feature request for the future: it would be nice if the script that generates this file included comments up at the top saying that it is an autogenerated file and explaining how to regenerate it.
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.
Good call, I've opened an issue for this.
I've opened an issue for this. |
I had a look at the PR for that and it sounds like I wasn't doing the right thing at the time anyways. Also, we don't allow any other unicode chars outside of text mode. I'm okay with removing that behavior. We'll want to mention it in the release notes as a breaking change. Fixing that probably makes sense as its own PR though. |
@kevinbarabash What does this do regarding |
@ronkok I haven't fixed the |
@kevinbarabash Thanks for the update. I will take no action. |
This depends on KaTeX/katex-fonts#5 which changed almost every combining glyph in the fonts to be non-combining. The ones that are left don't appear to be used. I'd like to remove unused glyphs from the fonts eventually but in a separate PR.