Skip to content

Conversation

@dpvc
Copy link
Member

@dpvc dpvc commented Oct 11, 2017

This PR adds support for italic correction, based on TeX's rules. We use the difference between a characters right-bearing and its width as a surrogate for italic-correction (since we don't have the actual TeX metrics for this).

The italic correction is added to the last character in mi and mo nodes (and should probably be added in mn as well, but numbers are usually in roman and so don't have italic correction). So an mi wrapper is added and the mo wrapper modified to do that. The italic correction is added via the CSS.

Italic correction is not to be used in some superscript and under-over situations. So there is CSS set up to remove the italic correction when needed in those cases.

Also, the italic correction is used to properly place super- and subscripts and under-over constructs, as per TeX placement rules. We adjust the italic correction value in theses cases to more closely match the actual TeX placements (since we don't have the actual TeX values used).

if (options.ic) {
styles['.MJX-TEX mjx-mi:not([noIC="true"])' + char.substr(1) + ':last-child::before'] =
styles['.MJX-TEX mjx-mo:not([noIC="true"])' + char.substr(1) + ':last-child::before'] = {
width: this.em(w + options.ic)
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 confusing. Can we have some auxiliary variables handling substrings? E.g., everything after mo/mi is the same.
Also, I think multiple assignments for initialisation is fine, but I would avoid them for more meaningful assignments.

let m = 0;
for (const i of widths.keys()) {
dw[i] = (w - widths[i]) / 2 + delta[i];
if (dw[i] < m) m = -dw[i];
Copy link
Member

Choose a reason for hiding this comment

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

braces

/*
* @param{string} variant The variant in which to look for the character
* @param{number} n The number of the character to look up
* @param{CharData} The full CharData object, with CharOptions guaranteed to be defined
Copy link
Member

Choose a reason for hiding this comment

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

@return{CharData} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right.

* @param{CharData} The full CharData object, with CharOptions guaranteed to be defined
*/
protected getChar(variant: string, n: number) {
const char = this.font.getChar(variant, n) || [0, 0, 0, null];
Copy link
Member

Choose a reason for hiding this comment

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

Why null and not {}?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default in the data arrays is null. It gets set to {} in the next line anyway, so does it really matter?

let [u, v] = (this.isCharBase() ? [0, 0] : [this.getU(basebox, supbox),
Math.max(basebox.d + tex.sub_drop * subbox.rscale, tex.sub2)]);
const subscriptshift = this.length2em(this.node.attributes.get('subscriptshift'), tex.sub2);
const drop = (this.isCharBase() ? 0 : basebox.d + tex.sub_drop * subbox.rscale);
Copy link
Member

Choose a reason for hiding this comment

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

These (also below) seem important computations. Can you add a comment what exactly goes on for the benefit of others (e.g, this reviewer).

Copy link
Member Author

Choose a reason for hiding this comment

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

These are the computations given in the TeXbook (Appendix G). I've cited that in the comment for the function itself. It's not always clear to me what they are up to, either, as the comments in the TeXBook are pretty sparse as well. I'll see what I can do.

const basebox = this.base.getBBox();
const underbox = this.script.getBBox();
const [k, v] = this.getUnderKV(basebox, underbox);
const delta = DELTA * this.baseCore.bbox.ic / 2;
Copy link
Member

Choose a reason for hiding this comment

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

delta vs DELTA

Copy link
Member Author

Choose a reason for hiding this comment

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

If you can't use the distinction between upper and lower case, why have them be distinct? The all-caps version is a constant, and the lower case one is a variable. I'm not sure why that is such a problem. In any case, I've changed the delta to something less meaningful.

*/
protected getOffset(bbox: BBox, sbox: BBox) {
return 0;
return [0, 0];
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should have a type for pairs and triples of numbers. They appear a lot and might help finding bugs. number[] could be too general. Just a thought, maybe for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, we can consider that for the future.

@dpvc
Copy link
Member Author

dpvc commented Jan 2, 2018

I've made the requested changes.

@dpvc dpvc merged commit efd937e into math-tag-support Jan 20, 2018
@dpvc dpvc deleted the italic-correction branch January 20, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants