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

Support for \' \` \^ \~ \= \u \. \" \r \H \v text-mode accents #802

Merged
merged 8 commits into from
Aug 23, 2017

Conversation

edemaine
Copy link
Member

I took a stab at implementing the text-mode accents that were easy to implement using current fonts, as outlined by @gagern in #638. Here's a sample screenshot:

accents

\begin{array}{l}
\text{e \'e \`e \^e \~e \u{e} \.e \"e \r{e} \H{e} \v{e}}\\
\text{l \'l \`l \^l \~l \u{l} \.l \"l \r{l} \H{l} \v{l}}\\
\text{X \'X \`X \^X \~X \u{X} \.X \"X \r{X} \H{X} \v{X}}
\end{array}

This seems to work, but feels a bit hacky because the accents are combining characters so don't behave well in isolation, so there's a magic number in a CSS style (similar but different to the \vec accent). It also misses key accents like \c because they're not in the font. Makes me wonder whether this is worthwhile, or we should correct the font somehow to make non-combining characters for these accents like we have for math mode. On the other hand, this support is better than nothing for now.

@ronkok
Copy link
Collaborator

ronkok commented Aug 17, 2017

Does this work properly on Safari? Issue #735 is a combining character problem.

@edemaine
Copy link
Member Author

@ronkok Probably not -- should have the same issue. Another reason to "wonder whether this is worthwhile, or we should correct the font somehow to make non-combining characters for these accents like we have for math mode. On the other hand, this support is better than nothing for now."

@kevinbarabash
Copy link
Member

We should look at porting this fix https://github.com/mathjax/MathJax/pull/1775/files. I feel like that's a separate issue/PR.

@ronkok
Copy link
Collaborator

ronkok commented Aug 17, 2017

Yesterday, I tried inserting a non-breaking space:

if (group.value.label === "\\vec") {
    accent.value = "\u00A0" + accent.value;
}

... and then adjusting the advance value in CSS class accent-vec by 0.25em (because nbsp is 0.25 em wide). I found that the arrow came in too low. I don't think I'll get any farther with it. I've got other issues at the moment.

@edemaine
Copy link
Member Author

Cool, I hadn't seen that. Now that I understand the accent generation code, I pushed a fix which ought to work for Safari. Can someone test? In my experiments, 0.262em seems a closer approximation to the width of a space -- how did you compute 0.25em?

@ronkok
Copy link
Collaborator

ronkok commented Aug 17, 2017

I got the 0.25 from http://glyphrstudio.com/online/. I don't take it as conclusive.
spacewidth

@ronkok
Copy link
Collaborator

ronkok commented Aug 17, 2017

Safari on the left, Chrome on the right.
accents

@ronkok
Copy link
Collaborator

ronkok commented Aug 17, 2017

@edemaine Instead of combining characters, could you use modifier characters?

@edemaine
Copy link
Member Author

Hmm, not sure why that's not working. Perhaps the combination of left and margin-left? I don't think we have a choice of the type of character, if we fix the font as-is.

@kevinbarabash
Copy link
Member

screen shot 2017-08-19 at 1 22 40 am

The Chrome changes look good for me. I started playing around with the styles on Safari and it looks like margin-left should be -spaceWidth + xMin for each of the accents. The xMin for the accents is a negative number b/c it's to the left of the origin.

The width of the space character is 0.250em according to opentype.js (I uploaded KaTeX_Main-Regular.ttf). For each of the accents the average of xMin and yMin is -0.250em which ends of centering it perfectly over the center of the space. Since Chrome renders the accent character correctly over the space, all we have to do is to correct for the space alone which is why the margin-left: -0.250em does the trick.

Chrome however treats the accent as a character with a real width from xMin to the origin. In the case of the umlaut which has an xMin of -0.405em, the resulting correction is margin-left: -0.655em.

screen shot 2017-08-19 at 1 34 01 am

screen shot 2017-08-19 at 1 34 46 am

Notice that this doesn't quite fix the single dot accent to the left of the umlaut. This is because its xMin is a different value.

This means that the solution is different for Safari and Chrome. The MathJax hack special cases Safari as well. The unfortunate result of this is that server side rendering will only work if you return different markup based on the browser's user agent. For client side rendering special casing shouldn't pose a problem.

One last note: fontMetricsData.js doesn't actually contain the values we need for the accent characters to fix Safari so we're going to need to extract those from the font.

@kevinbarabash
Copy link
Member

@edemaine let's not worry about fixing Safari accent positioning in this diff.


.accent-body.accent-text > span {
position: relative;
left: 0.262em; // width of space, apparently
Copy link
Member

Choose a reason for hiding this comment

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

The width, at least for KaTeX_Main-Regular, is 0.250em. Thankfully it appears that this value is the same, at least for the fonts I checked which were KaTeX_Main-Bold and KaTeX_SansSerif-Regular.

src/buildHTML.js Outdated
// Some browsers (Safari in particular) don't like combining
// characters without a preceding character. For each such accent,
// we add an artificial nonbreaking space with a negative margin
// to compensate for its width.
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth leaving this code in here with a TODO describe what's necessary to get the accents lining up on Chrome that way other contributors can build on top of the foundation you started here.

@edemaine
Copy link
Member Author

@kevinbarabash I'm a little confused about whether I should include the space hack in this patch at all. My impression was that it wasn't necessary at all for Chrome, but perhaps your investigation has found that it makes it easier to get the horizontal offset more consistent? Still grokking.

@kevinbarabash
Copy link
Member

@edemaine I didn't try removing the hack. If all the new accents work on Chrome without the hack, let's take it and get this merged and we can deal with Safari separately.

@edemaine
Copy link
Member Author

opentype.js was insightful! I realized that most of these accent characters have noncombining versions in the current font. I switched to those, so they should work in Safari. The only missing one is \H. I still use combining characters for that one.

I found that the width of a space (0.250em) looked best for \H. I would have thought something like: offset by -xMin to start at position 0; then shift back by half of the width (xMax - xMin) to center the accent. This worked well for e.g. \" (not that I need that anymore), but I think only because it ended up resulting in an almost identical offset of 0.2505em... @kevinbarabash If you understand this better, let me know if you think this offset is slightly off...

Other changes:

@edemaine edemaine mentioned this pull request Aug 20, 2017
3 tasks
@kevinbarabash
Copy link
Member

To fix \H for Safari we could look at updating the font to create a non-combining version of the glyph, but that's another PR. Also, if we end up not using the combining versions of these glyph we might look at removing them completely from the font to save bytes.

} else if (this.mode === "math" &&
funcData.allowedInMath === false) {
throw new ParseError(
"Can't use function '" + func + "' in math mode",
Copy link
Member

Choose a reason for hiding this comment

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

For future diffs, this could be written as:

`Can't use function '${func}' in math mode`,

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. (And thanks for teaching me ES interpolation strings in the first place!) You can tell I was copy/pasting from above. 😄

expect("\\'echec").toFailWithParseError(
"Can't use function '\\'' in math mode" +
" at position 1: \\̲'̲echec");
});
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for enforcing this and writing a test to verify enforcement.

@kevinbarabash kevinbarabash merged commit 2011932 into KaTeX:master Aug 23, 2017
@edemaine
Copy link
Member Author

@kevinbarabash Thanks for the review! I agree that we don't need combining characters in the font, and it'd be nice to add \H, \", and \vec accents as non-combining characters.

@edemaine edemaine mentioned this pull request Aug 23, 2017
@edemaine edemaine mentioned this pull request Dec 17, 2017
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.

3 participants