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 \vec #1018

Merged
merged 7 commits into from
Dec 18, 2017
Merged

Fix \vec #1018

merged 7 commits into from
Dec 18, 2017

Conversation

ronkok
Copy link
Collaborator

@ronkok ronkok commented Dec 11, 2017

For the accent \vec, replace the combining font glyph with an SVG.

In accent \vec, replace the combining font glyph with an SVG.
@ronkok
Copy link
Collaborator Author

ronkok commented Dec 11, 2017

This PR is written to fix issues #735 and #996.

\vec uses a combining character and does not have a character preceding it within its span. Safari, unlike other browsers, does not recognize the negative xMin value for such a character, so it renders \vec too far to the right.

This PR replaces the glyph with an SVG whose path is copied from the glyph. Let’s all just agree that this is an ugly hack. It is defensible only because user agents cannot be trusted to handle combining characters consistently.

The people over at MathJax must have come to the same conclusion. Their approach is also, by their own admission, a hack.

@kevinbarabash
Copy link
Member

I missed HorizontalBraces... PR in coming.

@kevinbarabash
Copy link
Member

kevinbarabash commented Dec 17, 2017

accents

KaTeX is in red, LaTeX is in green. The accent spacing looks really consistent except for over xA, but that's very minor and wouldn't worry about it.

@ronkok
Copy link
Collaborator Author

ronkok commented Dec 17, 2017

There was an alternative approach that I examined. If one prepends a non-breaking space before the combining character U+20D7, Safari will then treat it in the same way as the other browsers that I tested.

The problem then shifts to the vertical alignment. When the arrow is so treated, none of the browsers place it in the vertical position consistent with the glyph's yMin value. It's lower. By my eyeball test, it looks to be vertically centered on the x-height. But that is not documented anywhere that I can find. Rather than depend on something so dicey, I wrote the SVG.

@kevinbarabash
Copy link
Member

Rather than depend on something so dicey, I wrote the SVG.

Good call. I should be able to review this tomorrow.

@edemaine
Copy link
Member

edemaine commented Dec 17, 2017

@ronkok Indeed, I'd tried that hack (based on the MathJax hack, which I think you originally pointed me to) in an intermediate version of #802, and also couldn't get it to work. (I think MathJax has the luxury of doing different things depending on the browser. KaTeX can't do that, offline...) Thanks for finding another way! Given that most (long) accents are now drawn in SVG, this seems like a nice fix.

Just to point out: There is an alternative, to modify the font so that it has a non-combining version of the character. This is why \acute and all other accents work (I assume) in Safari. But I have never touched the fonts, so don't know how hard this approach is, or which is preferable.

@ronkok
Copy link
Collaborator Author

ronkok commented Dec 17, 2017

There is an alternative, to modify the font so that it has a non-combining version of the character.

That would be better. But fonts are beyond me. Hopefully, someone can make the font modification in some future PR. This PR can be a good enough temporary measure.

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

This all looks quite reasonable. Thanks for fixing this issue in a way that works with server side rendering.

const svgData: {
[string]: ([string, number, number])
} = {
// path, width, height
Copy link
Member

Choose a reason for hiding this comment

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

nit: only for space spaces before the //

@@ -621,6 +621,30 @@ const fontMap: {[string]: {| variant: string, fontName: string |}} = {
},
};

const svgData: {
[string]: ([string, number, number])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the parens are necessary here. Also, flow can do type inference so you should be able to get away with:

const svgData = {
   vec: ["vec", 0.471, 0.471],
};

And flow will complain about things like: svgData.vec[1].length but will be fine with svgData.vec[0].length.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think flow will infer a tuple type, but a more type-unsafe heterogeneous array. One could try to check.

My personal bias is to favor explicit typing to make the intention clear and better documented.

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 was following the example set by @marcianx during the conversion of stretchy.js to flow.

Copy link
Member

Choose a reason for hiding this comment

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

We can totally leave it in. It gets compiled away anyways.

@kevinbarabash
Copy link
Member

I restarted the build.

@kevinbarabash kevinbarabash merged commit 4410d48 into KaTeX:master Dec 18, 2017
@ronkok ronkok deleted the fix-vec branch December 19, 2017 02:49
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.

4 participants