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

Make accents zero width #1033

Merged
merged 3 commits into from
Dec 25, 2017
Merged

Make accents zero width #1033

merged 3 commits into from
Dec 25, 2017

Conversation

edemaine
Copy link
Member

@edemaine edemaine commented Dec 22, 2017

Make the width of an accented character equal to the width of the character, by making accents zero width. In particular, fixes #1028 (\ddot\imath).

This involved reworking all of the accent offset machinery, in particular skew and accent-hungarian. A bit cleaner now.

Also added support for the new width character metrics in symbolNode.

A bunch of screenshots involving skew correction have changed in an invisible way (diffs look entirely black and white). I assume this is a consequence of switching from margin-left to left as the mechanism for skew shift on accents.

Make the width of an accented character equal to the width of the character,
by making accents zero width.  In particular, fixes KaTeX#1028 (`\ddot\imath`).

This involved reworking all of the accent offset machinery, in particular
skew and accent-hungarian.  A bit cleaner now.

Also added support for the new width character metrics in symbolNode.
@edemaine edemaine mentioned this pull request Dec 22, 2017
@kevinbarabash
Copy link
Member

You can see the difference in the spacing in the font tests.

@kevinbarabash
Copy link
Member

@edemaine does this mean that we don't use combining accent glyphs for anything (besides \H) anymore? If so, we'll want to update all fonts to have all non-combining accents glyphs, many don't at the moment. Also, we should be able to remove all combining accent glyphs.

@kevinbarabash
Copy link
Member

@edemaine the screenshots look good for \H, but are off for Safari. KaTeX/katex-fonts#5 will add non-combining glyph for \H. I made other changes in that PR so it's probably best to land this as is and deal with the \H Safari issue separately.

@@ -733,6 +733,8 @@ const staticSvg = function(value: string, options: Options): domTree.span {
const svgNode = new domTree.svgNode([path], {
"width": width + "em",
"height": height + "em",
// Override CSS rule `.katex svg { width: 100% }`
"style": "width:" + width + "em",
"viewBox": "0 0 " + 1000 * width + " " + 1000 * height,
"preserveAspectRatio": "xMinYMin",
});
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to get rid of this once we have non-combining \vec as well. Then all of the non-stretchy accent code should use the same path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. That makes me feel better about the hacky duplication of width.

@@ -688,6 +688,7 @@ groupTypes.accent = function(group, options) {
// So now we use an SVG.
// If Safari reforms, we should consider reverting to the glyph.
accent = buildCommon.staticSvg("vec", options);
accent.width = parseFloat(accent.style.width);
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of hacky, but since it should disappear once we have non-combining \vec I'm okay with this.

src/buildHTML.js Outdated
if (group.value.label === '\\H') {
accentClass = "accent-hungarian";
left += 0.5; // width of space
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 this will work for all fonts, but since we're going to be adding non-combining double acute accents to all fonts soon it's okay to leave this as is for now.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I don't think this comment is correct as the width of a space is 250 (or thereabouts depending on the font).

Copy link
Member Author

@edemaine edemaine Dec 24, 2017

Choose a reason for hiding this comment

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

Ah, right, maybe it's double the width of a space. (I was copy/pasting the comment from the accent-hungarian CSS.) It might also be the width of the accent character, which is 0.5...

// because we are centering the accent, so by adding 2*skew to the left,
// we shift it to the right by 1*skew.
accentBody.style.marginLeft = 2 * skew + "em";
accentBody.style.left = left + "em";
Copy link
Member

Choose a reason for hiding this comment

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

I found that change the left value in the browser didn't actually change the position of the accent body. It appears that .katex .accent>.vlist-t is the style that's actively centering accents. Maybe we don't need the glyph width after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true that vlist is doing horizontal centering. That's how accents were able to work before. The catch is that we need to set the accent to zero width so that it doesn't affect the bounding box. When that happens, the left edge of the accent gets centered, and the accent itself extends to the right. Hence the need for the horizontal offset.

I do wonder whether there's a way to do this with CSS, without needing the width of the character...

@edemaine
Copy link
Member Author

@kevinbarabash Can you confirm whether \H used to work correctly in Safari? I would have guessed not (at one point I tried to copy the old mechanism from \H to \vec, and I don't think that worked, even though they're effectively the same). If my guess is correct, then I think it's safe to merge this PR. Otherwise we have a small regression, and I leave the decision (whether to wait for \H noncombining symbol) up to you.

I pushed a tweak to the comment.

@kevinbarabash
Copy link
Member

@edemaine let's deal with the non-combining \H separately.

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.

LGTM

@kevinbarabash kevinbarabash merged commit d6791b7 into KaTeX:master Dec 25, 2017
@ronkok
Copy link
Collaborator

ronkok commented Dec 25, 2017

@edemaine, You're correct. \H does not work in Safari in the current release.
combinh

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.

\ddot\imath is too wide
3 participants