Fix #533, #534, #541. #567

kohler · 2016-11-21T18:16:20Z

\mathbf with subscripts/superscripts causes incorrect spacing #534: Implement getTypeOfGroup for font groups.
\color causes incorrect spacing with some adjacent elements when \displaystyle is present in the coloured block #533, Got wrong space of LaTeX "\!=\!=\!="? #541: Improve the ways spaces are applied to lists. Since
CSS adjacency implements mathematical spacing, it's incorrect to
introduce "convenience spans" for spaces and display changes into
the generated HTML -- those spans break adjacency. Apply display
changes directly, and shift space spans into adjacent atoms.

Requires updates to two screenshotter tests, LimitControls and
SupSubLeftAlignReset. The new results for these tests are closer
to TeX output than the old results.

Also requires updates to Jasmine tests, since those assumed output
structures that have changed.

khanbot · 2016-11-21T18:16:31Z

CLA signature looks good 👍

kohler · 2016-11-21T19:14:42Z

I have a fix for #4 that's based on this commit, and will submit it for review after hearing back.

kohler · 2016-11-26T04:44:45Z

Hi, any chance I could talk to a contributor about the approach taken by this PR?

KaTeX's approach to TeX operator spacing uses CSS adjacency rules, as in .mathord + .mathbin { margin-left: ... }. This means that, for correct spacing, KaTeX must ensure that CSS adjacency corresponds to TeX operator adjacency.

But in some cases KaTeX doesn't ensure this. Style changes, size changes, and spaces introduce spurious spans into the HTML output. These spans mess up operator spacing: TeX considers two boxes adjacent (and space them accordingly), but in CSS the spans aren't adjacent. The same root cause is behind #533, #541, #136, and to some extent #4.

The approach taken here—eliminating spurious spans—seems like a good fit for current KaTeX, although it is not a trivial change. I've enjoyed working on these features and others but if there's no chance of merge I should do something else.

@kevinbarabash

kevinbarabash · 2016-11-26T17:32:04Z

@kohler definitely interested in this PR. I'll try to do a first past this weekend.

kevinbarabash · 2016-11-26T23:50:31Z

static/katex.less

+    }
+
+    .minner {
+        & + .mop.mtight { margin-left: @thinspace; }


I've checked these changes against https://books.google.ca/books?id=rstheMEAf2gC&pg=PA257&lpg=PA257&dq=texbook+ord+bin+rel+op+punct+spacing+rules&source=bl&ots=TU_zSmaVkr&sig=4ndj36478T4s9EI7OgrIAwwlUws&hl=en&sa=X&ved=0ahUKEwjltL3-zMfQAhUC9IMKHZhVCK0Q6AEIHTAA#v=onepage&q=texbook%20ord%20bin%20rel%20op%20punct%20spacing%20rules&f=false. They look good.

kevinbarabash · 2016-11-26T23:54:19Z

test/screenshotter/ss_data.yaml

@@ -36,6 +37,7 @@ Cases: |
 Colors:
    tex: \blue{a}\color{#0f0}{b}\color{red}{c}
    nolatex: different syntax and different scope
+ColorSpacing: \color{black}{\displaystyle \int x} + 1


Can you change the color to something other than black?

kevinbarabash · 2016-11-26T23:55:30Z

test/screenshotter/ss_data.yaml

@@ -106,6 +109,7 @@ Sqrt: |
    ^{\sqrt{\sqrt{\sqrt{x}}}}}
 SqrtRoot: |
    1+\sqrt[3]{2}+\sqrt[1923^234]{2^{2^{2^{2^{2^{2^{2^{2^{2^{2^{2^2}}}}}}}}}}}
+StyleSwitching: a\cdot b\scriptstyle a\cdot b


Could you add some style switches at boundaries between atom types, e.g. a\cdot\textstyle b

kevinbarabash · 2016-11-27T00:01:32Z

src/domTree.js

@@ -30,14 +30,22 @@ var createClass = function(classes) {
 * an inline style. It also contains information about its height, depth, and
 * maxFontSize.
 */
-function span(classes, children, height, depth, maxFontSize, style) {
+function span(classes, children, options) {


nice cleanup, I didn't realize we weren't using using half of the parameters to domTree.span.

kevinbarabash · 2016-11-27T00:13:19Z

src/buildCommon.js

@@ -35,7 +35,7 @@ var mainitLetters = [
 * Correctly pulls out metrics for the character, and optionally takes a list of
 * classes to be attached to the node.
 */
-var makeSymbol = function(value, style, mode, color, classes) {
+var makeSymbol = function(value, style, mode, options, classes) {


Can you rename style to fontFamily?

kevinbarabash · 2016-11-27T00:16:05Z

src/buildHTML.js

+        var atoms = group.value.value;
+        return getTypeOfGroup(atoms[atoms.length - 1]);
+    } else if (group.type === "font") {
+        return getTypeOfGroup(group.value.body);


Shouldn't we get the rightmost atom of the group.value.body for `fonts?

No. Font bodies are not arrays—unlike for style, etc. You can see this both in the creation code (in functions.js) and in groupTypes.font, which calls buildGroup() on the body rather than buildExpression on the body array.

Right b/c styles apply to everything which follows them whereas the font contains a single group. It's been a while since I've looked at the code – can you tell?

kevinbarabash · 2016-11-27T00:30:25Z

src/buildHTML.js

+                node = groups[i] = makeSpan(node.classes, [node]);
+            }
+            buildCommon.prependChildren(node,
+                groups.splice(lastSpace, i - lastSpace));


I found this kind of hard to follow. I think maybe doing two passes would make this more understandable, in the first pass we could add divide groups up into set of arrays and the second pass could create the spans. This should avoid the need for prependChildren.

I've tried a different one-pass arrangement.

kevinbarabash · 2016-11-27T00:38:58Z

src/buildHTML.js

+    newOptions = options.withStyle(nstyle);
+    var numer = buildGroup(group.value.numer, newOptions);
+    var numerreset = makeSpan([style.reset(), nstyle.cls()],
+        [numer], newOptions);


Is newOptions necessary for the numerreset? The .reset-X rules don't seem to interact with .tight. Am I missing something?

I doubt the newOptions are necessary, but neither are they harmful.

In that case it's probably best to either always pass options. There are a few places where it's not being passed, groupTypes.spacing, groupTypes.sqrt, groupTypes.array, and buildExpression. Can you add it to those call sites.

Ugh. I will try this, but if any tests fail, I will ask you to tolerate the inconsistency, which is really no different from (e.g.) previous commits, which didn't always pass options.color() in to makeSpan.

Yeah no look let me push back on this. There are makeSpan calls in delimiter.js, etc., that as of this commit have no options to pass. How about I put a TODO to improve API consistency.

A TODO sounds good.

kevinbarabash · 2016-11-27T00:39:47Z

src/buildHTML.js

@@ -684,14 +736,14 @@ groupTypes.spacing = function(group, options, prev) {
        // things has an entry in the symbols table, so these will be turned
        // into appropriate outputs.
        return makeSpan(
-            ["mord", "mspace"],
+            ["mspace"],


Why did "mord" disappear?

Because “mspace” nodes are no longer considered atoms. They are not atoms and TeX doesn't treat them as such. So they shouldn't affect operator spacing. If they were mord they'd affect operator spacing (e.g. if followed by an mbin)

That makes sense.

kevinbarabash · 2016-11-27T00:40:09Z

src/buildHTML.js

            [buildCommon.mathsym(group.value, group.mode)]
        );
    } else {
        // Other kinds of spaces are of arbitrary width. We use CSS to
        // generate these.
        return makeSpan(
-            ["mord", "mspace",
+            ["mspace",


kevinbarabash · 2016-11-27T00:41:53Z

src/buildHTML.js

-            group.value.body, fontName, "math", options.getColor(),
-            ["op-symbol", large ? "large-op" : "small-op", "mop"]);
+            group.value.body, fontName, "math", options,
+            ["mop", "op-symbol", large ? "large-op" : "small-op"]);


Thanks... this order for the classes is better. It would be nice if the param order between makeSpan and makeSymbol was more similar. Can you add a TODO to clean that up later (there's already enough changes in this PR)?

kevinbarabash · 2016-11-27T00:48:59Z

src/buildHTML.js

+    fontSize = fontSize * style.sizeMultiplier;
+
+    // Add size-resetting classes to the inner list and set maxFontSize
+    // manually.


Is this to handle a size change inside of a color group? Can you update this comment to include why we're doing this?

The old code set maxFontSize manually as well. In case you meant the pos stuff, I've updated the comment.

kevinbarabash · 2016-11-27T00:53:50Z

src/buildHTML.js

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


It seems like this is going to result in a lot of reset- styles being added. Is that the case? It seems like this is unavoidable.

This will add more reset- styles than the previous code will; on the other hand, it will add fewer nested <span>s. I don't see a way to avoid the additional classes, short of adding more passes and/or arguments to the builder functions. That would be another PR.

kevinbarabash · 2016-11-27T00:54:42Z

src/buildHTML.js

-        group.value.value, options.withStyle(newStyle), prev);
+        group.value.value, newOptions, prev);
+
+    // Add size-resetting classes to the inner list.


size-resetting or style-resetting?

kohler · 2016-11-27T04:35:27Z

OK @kevinbarabash I’ve addressed your comments! Let me know if you'd like anything else.

kevinbarabash · 2016-11-27T22:53:34Z

src/buildHTML.js

+            } else {
+                i = j;
+            }
+        }


This is still really hard to follow. I was thinking something more along the lines of the following (if I understand the code above correctly):

var spans = []; var children = []; for (i = 0; i < groups.length; i++) { var child = groups[i]; if (isSpace(child && children.length > 0) { spans.push(makeSpan(children[0].classes, children)); children = []; } else { children.push(child); } } if (children.length > 0) { spans.push(makeSpan(children[0].classes, children)); } return spans;

This doesn't do the same thing. Aside from the obvious bugs (the isSpace(child) case doesn’t actually put the current child anywhere), by introducing spurious spans, it would screw up class adjacency.

I think the code is pretty simple and you should take it as is.

The following code should work but is longer. If you really won't take the splice solution i'll do this.

var spaces = []; var nextGroups = []; for (i = 0; i < groups.length; i++) { group = groups[i]; if (isSpace(group)) { spaces.push(group); } else { if (spaces.length > 0) { if (group instanceof domTree.symbolNode) { group = makeSpan(group.classes, [group]); } buildCommon.prependChildren(group, spaces); spaces = []; } nextGroups.push(group); } } if (spaces.length > 0) { Array.prototype.push.apply(nextGroups, spaces); }

Thanks for catching my mistake with isSpace(child.

This is better. I was confused as to what was being prepended. buildCommon.prependChildren(group, spaces); makes that really clear. Question about the instanceof domTree.symbolNode check: if it fails is it possible that the group being passed into prependChildren is not a span?

OK. I still don't find the original confusing. Would

var spaces = groups.splice(i, j - i);

make it clearer. This isn't a huge deal but creating additional arrays isn't free; in the normal case where there are no spaces my code doesn't do any array allocation

As for instanceof domTree.symbolNode, as the immediately previous comment says,

// At this point `groups` consists entirely of `symbolNode`s and `span`s.

because the documentFragments have been spliced in

Not just the current color. This will facilitate applying options to built nodes in a standardized way, rather than changing all callsites.

Specifically, infer style from a class on the *current* element, rather than the parent element. Use "mtight" class to denote elements with tight spacing (scriptstyle or scriptscriptstyle). Apply that class automatically based on options.

- KaTeX#534: Implement getTypeOfGroup for font groups. - KaTeX#533, KaTeX#541: Improve the ways spaces are applied to lists. Since CSS adjacency implements mathematical spacing, it's incorrect to introduce "convenience spans" for spaces and display changes into the generated HTML -- those spans break adjacency. Apply display changes directly, and shift space spans into adjacent atoms. Requires updates to two screenshotter tests, LimitControls and SupSubLeftAlignReset. The new results for these tests are closer to TeX output than the old results. Also requires updates to Jasmine tests, since those assumed output structures that have changed.

This is so the size commands don't hide the types of their enclosed atoms. Addresses KaTeX#136. This slightly changes the vertical position of the Sizing test. Not sure the vertical position matters, so change the test.

kohler · 2016-11-28T15:03:30Z

OK, I've added the TODO (in commit 0911e5f) and updated the problematic loop (in d3ecb33). I hope we are now ready to merge.

kevinbarabash · 2016-11-28T15:45:46Z

@kohler thanks for updating the loop. I left a minor nit in the inline comments. I understand the desire to keep performance in check. TBH, performance isn't something I've put a whole lot of thought into :(, but it's one of the selling points of KaTeX so I wouldn't want to do anything that adversely affects it. We should probably have some sort of performance tests that we can run on each build to avoid perf regressions.

kevinbarabash · 2016-11-28T17:01:24Z

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.

I noticed that too. Can you run texcmp.sh to see how it compares against LaTeX's output? See https://github.com/Khan/KaTeX/blob/e6de31d2d622f01aae6d465cf332a268332942f4/dockers/texcmp/README.md for details.

kohler · 2016-11-28T17:21:12Z

It doesn't work, either on local or docker.

At least this diff is needed then I could get local to work

diff --git a/dockers/texcmp/texcmp.js b/dockers/texcmp/texcmp.js
index 3c16dc5..c164e5d 100644
--- a/dockers/texcmp/texcmp.js
+++ b/dockers/texcmp/texcmp.js
@@ -7,7 +7,7 @@ var path = require("path");
 var Q = require("q"); // To debug, pass Q_DEBUG=1 in the environment
 var pngparse = require("pngparse");
 var fft = require("ndarray-fft");
-var ndarray = require("ndarray-fft/node_modules/ndarray");
+var ndarray = require("ndarray");
 
 var data = require("../../test/screenshotter/ss_data");

Docker looks like some latex packages are missing from the docker

The diff is attached; there are font differences clearly; the baselines are equal, meaning the output of this PR is better than the previous

kevinbarabash · 2016-11-28T18:02:03Z

@kohler thanks for checking the output for the Sizing test. Hmm... I'm thinking the issue with ndarray is probably a different version of npm... it's using a flat directory structure now. We should probably add ndarray as a direct dependency.

kohler mentioned this pull request Nov 21, 2016

Fix #534: Teach getTypeOfGroup about font groups. #565

Closed

kohler force-pushed the styling-spacing-atoms branch 3 times, most recently from 44fdf40 to e95dda4 Compare November 26, 2016 04:44

kevinbarabash added the GH Review: review-needed label Nov 26, 2016

kevinbarabash self-assigned this Nov 26, 2016

kevinbarabash reviewed Nov 26, 2016

View reviewed changes

kevinbarabash reviewed Nov 27, 2016

View reviewed changes

kevinbarabash mentioned this pull request Nov 27, 2016

Fix #4 - Spacing interactions between atoms #492

Closed

kohler force-pushed the styling-spacing-atoms branch from e95dda4 to 45d6f6f Compare November 27, 2016 04:32

kevinbarabash reviewed Nov 27, 2016

View reviewed changes

kevinbarabash added GH Review: needs-revision and removed GH Review: review-needed labels Nov 27, 2016

kohler added 5 commits November 27, 2016 17:56

Internal: Pass full options objects to makeSpan/makeSymbol.

0911e5f

Not just the current color. This will facilitate applying options to built nodes in a standardized way, rather than changing all callsites.

Add style switching test: text and scriptstyle in the same group.

9784815

Fix KaTeX#136: Size commands generate fragments, not spans.

e9899d8

This is so the size commands don't hide the types of their enclosed atoms. Addresses KaTeX#136. This slightly changes the vertical position of the Sizing test. Not sure the vertical position matters, so change the test.

kohler force-pushed the styling-spacing-atoms branch from 45d6f6f to e9899d8 Compare November 28, 2016 14:51

kevinbarabash added GH review: accepted and removed GH Review: needs-revision labels Nov 28, 2016

kevinbarabash merged commit 9d3cdf6 into KaTeX:master Nov 28, 2016

kohler deleted the styling-spacing-atoms branch November 29, 2016 03:52

Fix #533, #534, #541. #567

Fix #533, #534, #541. #567

Conversation

kohler commented Nov 21, 2016

khanbot commented Nov 21, 2016

kohler commented Nov 21, 2016

kohler commented Nov 26, 2016

kevinbarabash commented Nov 26, 2016 • edited

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

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 Nov 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohler commented Nov 27, 2016

kevinbarabash Nov 27, 2016 • edited

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 Nov 28, 2016

kevinbarabash commented Nov 28, 2016

kevinbarabash commented Nov 28, 2016

kohler commented Nov 28, 2016

kevinbarabash commented Nov 28, 2016

kevinbarabash commented Nov 26, 2016 •

edited

kohler Nov 27, 2016 •

edited

kevinbarabash Nov 27, 2016 •

edited