Fix interaction between styles and sizes. #719

kohler · 2017-06-06T23:59:15Z

Rather than having both textstyle CSS classes and size5 CSS classes affect the font size (and step on each other), implement sizes more the way TeX does: a command like \displaystyle changes the current size.

Remove displaystyle/textstyle CSS classes entirely, since they do nothing now.

This is actually a simplification, since now only size affects the size. Simplifies CSS and computation. Many screenshotter tests change; most of them (all?) change to be more like TeX.

kevinbarabash · 2017-06-07T00:52:13Z

Thanks for the PR. I wonder how this interacts with #718. I'm guessing that after rebasing now that #718 has been merged will result in some additional changes to some of the screenshots. I'll try to review this over the next couple of days.

kohler · 2017-06-07T02:38:53Z

Nah, there's no interaction between this and #718—this actually assumed #718 :)

kohler · 2017-06-07T03:01:10Z

(Rebased.)

kohler · 2017-06-07T04:39:09Z

c2aaf90 fixes a longstanding bug in \sqrt in scriptsizes.

kohler · 2017-06-07T14:46:50Z

I'm going to re-commit with some changes to the mechanism, because of corner cases in the interaction between sizes and styles.

Here's the issue. Consider:

\scriptstyle\large a vs. \large\scriptstyle a

These commands have no equivalent in TeX, but these work, and have the following result:

$\text{\large$a$}$---$\text{\large$\scriptstyle a$}$---$\scriptstyle\text{\large$a$}$

Note that \large overrides the preceding \scriptstyle size change. The question is, does \large override all properties of \scriptstyle, or just the size?

In this commit, Options has a baseSize which determines the sizes chosen by future style changes. A sizing command overrides both the currently active size and the baseSize, but does not override other aspects of the style.

This means that TeX like \scriptstyle\large a\scriptstyle b will have possibly-surprising results. Both “a” and “b” will nominally be in \scriptstyle (which affects, e.g., math spacing), but “b” will be smaller.

I'm not sure there's a more coherent solution than this (maybe you can think of one), but this seems most TeXlike.

kohler · 2017-06-07T15:14:23Z

After playing with more examples, including a^{2^{\sqrt{\sqrt{\large 2^{2^2}}}}}\large 2^{2^2}, I now believe sizing commands should change to \textstyle (if the style is currently \scriptstyle or \scriptscriptstyle), and that's what's there now.

kevinbarabash · 2017-06-10T20:35:25Z

The \sqrt changes look awesome. It's interesting that many of the screenshots changed, but there doesn't appear to be any visual change. I can only assume that some change in the order of operations of some of the calculations resulted in an imperceptible change to the layout. I was surprised by the changes to ModScript-chrome.png without changes to ModScript-firefox.png.

I now believe sizing commands should change to \textstyle (if the style is currently \scriptstyle or \scriptscriptstyle), and that's what's there now.

This is so that both the sizes glyphs and the spaces between them is consistent no matter where the sizing command is used, is that right?

kohler · 2017-06-11T01:22:32Z

The \sqrt changes look awesome. It's interesting that many of the screenshots changed, but there doesn't appear to be any visual change. I can only assume that some change in the order of operations of some of the calculations resulted in an imperceptible change to the layout. I was surprised by the changes to ModScript-chrome.png without changes to ModScript-firefox.png.

That is my assumption too, and I share your surprise. The main change here is that, e.g., \scriptsize changes are applied via the size mechanism, which might be, you know, 0.000001 off from the previous relative fontsize or something.

This is so that both the sizes glyphs and the spaces between them is consistent no matter where the sizing command is used, is that right?

Yes.

For what it's worth, and it's up to you, but this will probably conflict with #519. Whatever gets merged first I'm happy to help resolve conflicts.

kohler · 2017-06-15T16:01:54Z

Hi @kevinbarabash any other comments on this?

edemaine · 2017-06-15T16:03:18Z

@kohler Did you mean something other than #519?

kohler · 2017-06-15T17:08:59Z

Whoops, thanks, I meant the @ronkok stretchy wide PR, which is #670

edemaine · 2017-06-15T18:33:18Z

Ah, OK, I figured. Probably #670 wants to go first, as @kevinbarabash has been working on that for a while.

kevinbarabash · 2017-06-15T23:06:11Z

@kohler sorry for the delay. I want to merge #670 first. I'm going to spend some more time tonight looking at it and will hopefully be able to merge it. My hope is to get your PR reviewed either tomorrow or sometime this weekend.

edemaine · 2017-06-16T13:15:27Z

#670 has landed. I don't see any conflicts, but perhaps tests need to be rerun?

kohler · 2017-06-17T03:16:29Z

The new PR is ready, including necessary changes to the stretchy functions (#670). I also added a screenshotter improvement.

kevinbarabash · 2017-06-17T19:44:27Z

src/Options.js

 /**
- * Create a new options object with the given style.
+ * Return an options object with the given style. If `this.style === style`,
+ * returns `this`.


This seems functionally equivalent as long as we don't mutate these objects. Can you add a warning somewhere not to mutate these objects and to only use the functions provided to create new Option instances from existing ones?

Sure.

FWIW it would also be OK to use the old .with* names. The .having* names were very useful during auditing.

kevinbarabash · 2017-06-17T19:44:40Z

src/Options.js

-    return this.extend({
-        size: size,
-    });
+Options.prototype.havingCrampedStyle = function() {


Where is this used?

buildHTML uses it for convenience.

Thanks. I totally missed that file b/c github auto-collapsed it.

I'll try to review the changes to buildHTML.js over the next couple of days.

kevinbarabash · 2017-06-17T19:51:21Z

src/Options.js

@@ -5,6 +5,25 @@
 * `.reset` functions.
 */

+const Style = require("./Style");
+
+const sizeStyleMap = [ // [textsize, scriptsize, scriptscriptsize]


Where do these numbers come from?

So these numbers are chosen by applying 0.7x and 0.5x the base size and rounding to an available KaTeX size. But your question inspired an experiment.

In old KaTeX, scriptsize is always 0.7x the size of textsize, and scriptscriptsize is 0.5x the size of textsize. This also holds in LaTeX at 10pt:

\documentclass[10pt]{article} \usepackage[utf8]{inputenc} \showboxbreadth=\maxdimen \showboxdepth=\maxdimen \begin{document} \tracingonline=1 $a^{b^c}$\showlists \end{document}

shows

\OML/cmm/m/it/10 a \hbox(5.17056+0.0)x7.7638, shifted -3.62892 .\OML/cmm/m/it/7 b .\hbox(2.15277+0.0)x3.74713, shifted -3.01779 ..\OML/cmm/m/it/5 c

But at other point sizes these ratios don't hold precisely, since LaTeX was originally built for a fixed palette of fonts at specific sizes. For instance, 11pt gives sizes 10.95/8/6, and 12pt gives 12/8/6.

FWIW here are the sizes LaTeX uses at base font size 10pt, compared with the PR's current sizes:

TeX command TeX Sizes Current PR sizes

\tiny 5/5/5 5/5/5

\scriptsize 7/5/5 7/5/5

\footnotesize 8/6/5 8/7/5 [NB KaTeX has no 6pt]

\small 9/6/5 9/7/5

\normalsize 10/7/5 10/7/5

\large 12/8/6 12/8/7

\Large 14.4/10/7 14.4/10/7

\LARGE 17.28/12/10 17.3/12/9

\huge 20.74/14.4/12 20.7/14.4/10

\Huge 24.88/20.74/17.28 24.9/17.3/12

So some differences.

So what to do? We could keep the current sizes (& maybe document them). Or we could implement these more TeX-precise sizes in KaTeX, with or without the currently-missing 6pt font. We can also delay this choice (i.e. leave the current sizes in and then change them later).

My understanding of KaTeX fonts is that we only have 10pt size fonts and we scale them appropriately. What would prevent us from scaling that font to 6pt when necessary?

I agree this shouldn't be blocking, but if we defer please add a comment in the code.

There is nothing preventing us from scaling the font to 6pt. But because we do things with CSS here, it would mean that “normalsize”, which has been “size5” in CSS classes for a while, would become “size6”. Probably doesn't matter.

kevinbarabash · 2017-06-17T19:53:30Z

src/Options.js

@@ -58,22 +68,69 @@ Options.prototype.extend = function(extension) {
    return new Options(data);
 };

+function sizeAtStyle(size, style) {
+    return style.size < 2 ? size : sizeStyleMap[size - 1][style.size - 1];


Is this equivalent to the following?

return sizeStyleMap[size - 1][style.size - 1];

No because style.size might be 0 (DISPLAY).

It would be equivalent to sizeStyleMap[size - 1][Math.max(0, style.size - 1)] but I find this easier to understand

For some reason I thought the sizes where started counting from 1. I think my reason for confusion is that I was conflating sizing command sizes with style command sizes. :( The fact that we're accessing values in sizeStyleMap should've clued me in.

kevinbarabash · 2017-06-17T19:54:36Z

src/Options.js

+        // Ensure style is at least `\textstyle`.
+        let style = this.style;
+        if (style.size > 1) {
+            style = style.cramped ? Style.TEXT.cramp() : Style.TEXT;


I'm curious why you chose not reset the cramped-ness?

It didn't hurt.

(I don't have a crisp response. :) )

kevinbarabash · 2017-06-17T20:01:50Z

src/Options.js

- * Create a new options object with the same style, size, and color. This is
- * used so that parent style and size changes are handled correctly.
+ * Return the CSS sizing classes required to switch from enclosing options
+ * `oldOptions` to `this`. Returns an array of classes.


Nice comment.

kevinbarabash · 2017-06-17T20:04:22Z

src/Parser.js

@@ -483,7 +483,7 @@ Parser.prototype.parseImplicitGroup = function() {
        const body = this.parseExpression(false);
        return new ParseNode("sizing", {
            // Figure out what size to use based on the list of functions above
-            size: "size" + (utils.indexOf(sizeFuncs, func) + 1),
+            size: utils.indexOf(sizeFuncs, func) + 1,


nice cleanup

kevinbarabash · 2017-06-17T20:06:44Z

src/buildMathML.js

@@ -489,8 +490,7 @@ groupTypes.sizing = function(group, options) {
    // in, so we can't reset the size to normal before changing it.  Now
    // that we're passing an options parameter we should be able to fix
    // this.
-    node.setAttribute(
-        "mathsize", buildCommon.sizingMultiplier[group.value.size] + "em");
+    node.setAttribute("mathsize", newOptions.sizeMultiplier + "em");


Does the comment above still apply?

I have no idea and I have no way to check and it's MathML :(

This should be checkable by rendering a expression with nested sizing commands and verify that the mathsize values in the MathML tree in the DOM match the sizes we use for same elements in visible HTML nodes that KaTeX renders.

kevinbarabash · 2017-06-17T20:12:06Z

src/delimiter.js

@@ -317,10 +311,11 @@ const makeStackedDelim = function(delim, heightTotal, center, options, mode,
    inners.push(makeInner(top, font, mode));

    // Finally, build the vlist
-    const inner = buildCommon.makeVList(inners, "bottom", depth, options);
+    const newOptions = options.havingBaseStyle(Style.TEXT);


Why are we reset the base style here?

Because stacked delimiters are taken from the extension font, and in old TeX, the extension font was only used at size 10.

However, there is a wrinkle: In AMSLaTeX, the extension font is available (and used) at smaller sizes too. I could see changing the code to do that instead.

The delimiters look pretty good in the screenshots as is.

kevinbarabash · 2017-06-17T20:18:32Z

dockers/Screenshotter/screenshotter.js

+        }
+    });
+    return deferred.promise;
+}


These changes seem unrelated to the rest of the PR. Were you using this to compare the sizing changes with LaTeX output?

Yes. I'm happy to take them out of the PR but shrug it's all going in the same place eventually

@gagern is probably the most experienced with the screenshotter, so he might want to look at this part of the PR (or separate the PR into two).

It's definitely preferable to to have these as separate commits. Even though it's all going to the same place, smaller commits will take less time to review and if one review needs revision it doesn't hold up the other.

…zes. Rather than having both `textstyle` CSS classes and `size5` CSS classes affect the font size (and step on each other), implement sizes more the way TeX does: a command like `\displaystyle` changes the current size. This is actually a simplification, since now only `size` affects the size. Simplifies CSS and computation. Many screenshotter tests change; they change to be more like TeX. For instance, `\sqrt` fixes some discrepancies in size treatment. Also: Remove the `Options.withX()` methods in favor of `.havingX()`, which might return the same `options`. Remove `Style.cls()` and `Style.reset()`. Remove `Options.reset()`. You should never modify an `Options`; they should change only by the `havingX()` methods.

kohler · 2017-06-17T21:53:04Z

I took out the screenshotter diff and updated some comments as requested.

kohler · 2017-06-17T22:50:48Z

I'm going to check in another commit that adds the 6pt size and implements TeX's size relationships for scriptstyle/scriptscriptstyle. I also gave KaTeX's sizes more significant digits.

At every size level. Also make the sizes match TeX to the last decimal.

kohler · 2017-06-18T02:44:41Z

The TeX size relationships are in.

kohler · 2017-06-23T14:03:52Z

Hi @kevinbarabash any comments?

kevinbarabash · 2017-06-23T17:59:48Z

@kohler sorry. I haven't reviewed changes to buildHTML.js. I'll have time this weekend.

kevinbarabash

There are a few nits which may or may not make sense to change. Mostly questions so that I understand what's going on. This is a complex change and I want to make sure I'm not missing something before merging this.

kevinbarabash · 2017-06-25T22:03:47Z

src/buildHTML.js

-        newOptions = options.withStyle(style.sup());
-        sup = buildGroup(group.value.sup, newOptions);
-        supmid = makeSpan([style.reset(), style.sup().cls()],
-            [sup], newOptions);


I'm curious why the wrapping isn't necessary anymore.

It looks like it's being handled by buildGroup now that we're passing parent options.

kevinbarabash · 2017-06-25T22:05:11Z

src/buildHTML.js

+        if (!isCharacterBox(group.value.base)) {
+            subShift = base.depth + newOptions.style.metrics.subDrop
+                * newOptions.sizeMultiplier / options.sizeMultiplier;
+        }


Moving this up here to avoid unnecessary calculations when there isn't both sub and sup parts is a nice optimization.

I'm okay with keeping this change as it makes reviewing easier.

kevinbarabash · 2017-06-25T22:06:27Z

src/buildHTML.js

-    let sub;
+    const base = buildGroup(group.value.base, options);
+    let supm;
+    let subm;


Why the name change from sub and sup? Keeping those variables names would've reduced the number of changed lines.

So, in the old code, sub and sup were not decorated with the classes necessary to change size, whereas supmid and submid were so wrapped.

In the new code, we don't need both versions, because we create the wrapped versions in one step. These are more like the mid versions, so I kept a suffix.

kevinbarabash · 2017-06-25T22:10:20Z

src/buildHTML.js

-            [options.style.reset(), Style.TEXT.cls(), "frac-line"]);
-        // Manually set the height of the line because its height is
-        // created in CSS
-        mid.height = ruleWidth;


I see makeLineSpan takes care of setting the height.

kevinbarabash · 2017-06-25T22:15:45Z

src/buildHTML.js

@@ -450,20 +436,16 @@ groupTypes.genfrac = function(group, options) {
    const dstyle = style.fracDen();
    let newOptions;


I wish we weren't reusing this identifier for different options within the frac layout, but since this is the way the code was I guess we'll have to change it later.

kevinbarabash · 2017-06-25T22:43:02Z

src/buildHTML.js

@@ -770,18 +748,18 @@ groupTypes.spacing = function(group, options) {

 groupTypes.llap = function(group, options) {
    const inner = makeSpan(
-        ["inner"], [buildGroup(group.value.body, options.reset())]);
+        ["inner"], [buildGroup(group.value.body, options)]);


The reset() isn't necessary anymore here b/c buildGroup handles that now.

No: The .reset() isn't necessary any more because Options objects are immutable.

I was mixing up options.reset() with the reset- CSS classes.

kevinbarabash · 2017-06-25T22:45:00Z

src/buildHTML.js

+const makeLineSpan = function(className, options) {
+    const baseOptions = options.havingBaseStyle();
+    const line = makeSpan(
+        [className].concat(baseOptions.sizingClasses(options)),


Are adding the sizing classes necessary here? The span has no children and we're already setting its height explicitly.

The sizing classes were on rules previously too, I think.

kevinbarabash · 2017-06-25T22:48:18Z

src/buildHTML.js

-            inner[i].maxFontSize = fontSize;
-        } else if (inner[i].classes[pos + 1] === "reset-" + group.value.size) {
+            inner[i].classes.push("sizing", "reset-size" + baseOptions.size,
+                                  "size" + options.size);


Could we not using options.sizingClasses(baseOptions) here?

kevinbarabash · 2017-06-25T22:54:25Z

src/buildMathML.js

@@ -489,8 +490,7 @@ groupTypes.sizing = function(group, options) {
    // in, so we can't reset the size to normal before changing it.  Now
    // that we're passing an options parameter we should be able to fix
    // this.
-    node.setAttribute(
-        "mathsize", buildCommon.sizingMultiplier[group.value.size] + "em");
+    node.setAttribute("mathsize", newOptions.sizeMultiplier + "em");


This should be checkable by rendering a expression with nested sizing commands and verify that the mathsize values in the MathML tree in the DOM match the sizes we use for same elements in visible HTML nodes that KaTeX renders.

kevinbarabash · 2017-06-25T22:57:05Z

src/buildHTML.js

-
-    return new buildCommon.makeFragment(inner);
+    const newOptions = options.havingStyle(newStyle);
+    return sizingGroup(group.value.value, newOptions, options);


I really like how you unified the handle of style groups and sizing groups.

kohler · 2017-06-26T03:03:18Z

Thanks for the comments, I checked in some changes.

The MathML values look correct to me in a couple of examples, so I think it's OK.

kohler · 2017-06-27T14:32:59Z

Hi @kevinbarabash, any more comments or requests on this?

(I have some other changes blocked here.)

kevinbarabash · 2017-06-28T00:55:29Z

@kohler thanks for making those changes and for checking the MathML output. The output looks good so if there are things that I've missed they must be minor.

Corrects a bug in KaTeX#719.

kohler mentioned this pull request Jun 6, 2017

Font sizing commands #709

Closed

kevinbarabash self-assigned this Jun 7, 2017

kohler force-pushed the ssize branch from b011cd7 to 3826bee Compare June 7, 2017 02:59

kohler force-pushed the ssize branch from 3826bee to c2aaf90 Compare June 7, 2017 04:38

kohler force-pushed the ssize branch from c2aaf90 to 99e34b8 Compare June 7, 2017 14:45

kohler force-pushed the ssize branch from 99e34b8 to 3956779 Compare June 7, 2017 15:14

kohler mentioned this pull request Jun 15, 2017

Support absolute TeX units #732

Merged

edemaine added the blocked label Jun 15, 2017

edemaine removed the blocked label Jun 16, 2017

kohler force-pushed the ssize branch from 3956779 to bd21cc7 Compare June 17, 2017 03:14

kohler force-pushed the ssize branch from bd21cc7 to d8cc4e8 Compare June 17, 2017 03:17

kevinbarabash reviewed Jun 17, 2017

View reviewed changes

kohler force-pushed the ssize branch from d8cc4e8 to 5fb6804 Compare June 17, 2017 21:51

kohler force-pushed the ssize branch from 607b9a2 to ba2346f Compare June 18, 2017 02:02

Implement TeX sizing for scriptsize/scriptscriptsize.

3ac84ae

At every size level. Also make the sizes match TeX to the last decimal.

kohler force-pushed the ssize branch from ba2346f to 3ac84ae Compare June 18, 2017 02:41

kevinbarabash reviewed Jun 25, 2017

View reviewed changes

Review comments.

8301c41

kohler force-pushed the ssize branch from 1c57b91 to 8301c41 Compare June 26, 2017 13:55

kevinbarabash merged commit b866cd5 into KaTeX:master Jun 28, 2017

kohler added a commit to kohler/KaTeX that referenced this pull request Jun 30, 2017

Make \kern use relative units.

9f2dd08

Corrects a bug in KaTeX#719.

kohler mentioned this pull request Jul 1, 2017

Correct computation of TeX sizes. #755

Merged

ronkok mentioned this pull request Jul 14, 2018

Sizing Commands spacing not right with roots #1476

Closed

TeX command	TeX Sizes	Current PR sizes
`\tiny`	5/5/5	5/5/5
`\scriptsize`	7/5/5	7/5/5
`\footnotesize`	8/6/5	8/7/5 [NB KaTeX has no 6pt]
`\small`	9/6/5	9/7/5
`\normalsize`	10/7/5	10/7/5
`\large`	12/8/6	12/8/7
`\Large`	14.4/10/7	14.4/10/7
`\LARGE`	17.28/12/10	17.3/12/9
`\huge`	20.74/14.4/12	20.7/14.4/10
`\Huge`	24.88/20.74/17.28	24.9/17.3/12

		@@ -450,20 +436,16 @@ groupTypes.genfrac = function(group, options) {
		const dstyle = style.fracDen();
		let newOptions;

Fix interaction between styles and sizes. #719

Fix interaction between styles and sizes. #719

Conversation

kohler commented Jun 6, 2017

kevinbarabash commented Jun 7, 2017

kohler commented Jun 7, 2017

kohler commented Jun 7, 2017

kohler commented Jun 7, 2017

kohler commented Jun 7, 2017

kohler commented Jun 7, 2017

kevinbarabash commented Jun 10, 2017

kohler commented Jun 11, 2017

kohler commented Jun 15, 2017

edemaine commented Jun 15, 2017

kohler commented Jun 15, 2017

edemaine commented Jun 15, 2017

kevinbarabash commented Jun 15, 2017

edemaine commented Jun 16, 2017

kohler commented Jun 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohler commented Jun 17, 2017

kohler commented Jun 17, 2017

kohler commented Jun 18, 2017

kohler commented Jun 23, 2017

kevinbarabash commented Jun 23, 2017

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohler commented Jun 26, 2017

kohler commented Jun 27, 2017

kevinbarabash commented Jun 28, 2017

kevinbarabash Jun 17, 2017 •

edited

Loading

kevinbarabash Jun 17, 2017 •

edited

Loading