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

add \ae, \AE, \oe, \OE, \o, \O, \ss with unicode support #1030

Merged
merged 1 commit into from
Dec 22, 2017

Conversation

kevinbarabash
Copy link
Member

screen shot 2017-12-22 at 12 00 52 pm

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.

Great, I didn't realize that these are already in the fonts! (I confirmed that they render using Computer Modern fonts, and look just like LaTeX's output.)

LaTeX throws warnings (LaTeX Warning: Command \ae invalid in math mode on input line 7.) if we use these commands in math mode, but technically it does support them (generating textords). Should we? This PR currently only supports the characters in text mode.

Technically, some lines in fontMetrics.js such as this one should be removed. You can also feel free to leave this to #992 if you'd like.

it("should render ligature commands like their unicode characters", () => {
const commands = getBuilt("\\text{\\ae\\AE\\oe\\OE\\o\\O\\ss}");
const unicode = getBuilt("\\text{æÆœŒøØß}");
expect(commands).toEqual(unicode);
Copy link
Member

Choose a reason for hiding this comment

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

You can use toParseLike instead of this: expect("\\text{...}").toParseLike("\\text{...}")

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 tried that, but they don't actually parse the same.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, getBuilt renders to HTML. I didn't realize that -- it suggests some other tests we could do!

@kevinbarabash kevinbarabash mentioned this pull request Dec 22, 2017
@kevinbarabash
Copy link
Member Author

Technically, some lines in fontMetrics.js such as this one should be removed. You can also feel free to leave this to #992 if you'd like.

I can remove those.

@@ -103,7 +103,6 @@ const extraCharacterMap = {
'Ã': 'A',
'Ä': 'A',
'Å': 'A',
'Æ': 'A',
Copy link
Member Author

Choose a reason for hiding this comment

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

There weren't any entries for Œ or œ.

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.

LGTM!

@kevinbarabash kevinbarabash merged commit 522b238 into master Dec 22, 2017
@edemaine
Copy link
Member

edemaine commented Dec 22, 2017

@kevinbarabash What are your thoughts about supporting these commands in textmath mode?

edemaine added a commit to edemaine/KaTeX that referenced this pull request Dec 22, 2017
@kevinbarabash
Copy link
Member Author

@edemaine I assume you mean "math mode". I didn't realize that they appear in both. I'll put up another PR for that.

@edemaine
Copy link
Member

@kevinbarabash "LaTeX throws warnings (LaTeX Warning: Command \ae invalid in math mode on input line 7.) if we use these commands in math mode, but technically it does support them (generating textords)."

@kevinbarabash
Copy link
Member Author

@edemaine I was testing using http://quicklatex.com. I tried it again with pdflatex locally and am seeing the same issue you are. In that case let's not bother. The change was non trivial to b/c while we have the glyphs in KaTeX_Main-Italic they need to either be added to KaTeX_Math-Italic or we would've had to special case things in the code.

@edemaine
Copy link
Member

If they're specified as textords, I thought they'd be rendered in text mode using text fonts. I could be wrong though. (That is also what LaTeX does: it renders them in Roman font by default.)

@kevinbarabash
Copy link
Member Author

For text mode they are textords and they do get rendered using KaTeX_Main-* which are the text fonts.

edemaine added a commit to edemaine/KaTeX that referenced this pull request Dec 28, 2017
kevinbarabash pushed a commit that referenced this pull request Dec 29, 2017
* Unicode accents

* Lexer now looks for combining dicritical marks and adds them to the same character
* Parser's `parseSymbol` now recognizes both combined and uncombined forms of Unicode accents, and builds accent objects just like the accent functions
* Added CJK support to math mode (not just text mode)

* Add invalid combining character test

* Add MathML test

* Add weak support for other Latin-1 characters

This maintains backwards compatibility, but it uses the wrong font.
There's a TODO to fix this later.

Also refactor symbol code to use for..of

* Update Unicode screenshot

* Remove dot from accented i and j (in math mode)

Also add dotless Unicode characters to support some accented i's and j's

* Fix \imath, \jmath, \pounds, and more tests

* Switch from for..of to .split().forEach()

Save around 800 bytes in minified code

* Fix split

* normalize() detection

* Convert back to vanilla for loops

* Fix merge

* Move normalize dependency to unicodeMake.js

* Make unicodeSymbols into a lookup table instead of macros

This is important for multi-accented characters.

* Add comments about when to run

* Move symbols definition into unicodeMake/Symbols.js

* Remove CJK support in text mode

* Add missing semicolon

* Refactor unicodeAccents to its own file

* Dotless i/j support in text mode

* Remove excess character mappings

* Fix Åå in math mode (still via Times)

* Update to support #1030

* Add accented Greek letter support (for supported Greek symbols)

* Update screenshot

* remove Æ, æ, Ø, ø, and ß from math mode test
@kevinbarabash kevinbarabash deleted the ligatures branch December 29, 2017 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants