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

Conversation

@ronkok
Copy link
Collaborator

commented Dec 11, 2017

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

Fix \vec
In accent \vec, replace the combining font glyph with an SVG.
@ronkok

This comment has been minimized.

Copy link
Collaborator Author

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.

ronkok and others added 4 commits Dec 11, 2017
Merge pull request #3 from Khan/fix-vec-screenshots
update screenshots containing vectors
@kevinbarabash

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

I missed HorizontalBraces... PR in coming.

@kevinbarabash

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

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

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

@edemaine

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator Author

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.

@kevinbarabash
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Dec 17, 2017

Member

nit: only for space spaces before the //

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

const svgData: {
[string]: ([string, number, number])

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Dec 17, 2017

Member

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.

This comment has been minimized.

Copy link
@marcianx

marcianx Dec 17, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@ronkok

ronkok Dec 17, 2017

Author Collaborator

I was following the example set by @marcianx during the conversion of stretchy.js to flow.

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Dec 18, 2017

Member

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

@kevinbarabash

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

I restarted the build.

@kevinbarabash kevinbarabash merged commit 4410d48 into KaTeX:master Dec 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ronkok ronkok deleted the ronkok:fix-vec branch Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.