Use JS for spacing between atoms instead of CSS by k4b7 · Pull Request #1070 · KaTeX/KaTeX

k4b7 · 2018-01-16T16:33:03Z

This is the first step towards creating an intermediate representation
that can be used to generate HTML, SVG, and Canvas commands for rendering.
By generating spans that contain the width of the spaces instead of
relying on CSS sibling rules we'll be able to one day replaces the spans
with intermeidate 'Glue' nodes (in a later PR). This was described in #376 (comment).

An added benefit of this approach is that is enables us to programmatically
change the values for thinspace, mediumspace, and thickspace which will
allow us to implement the \setlength command.

Summary: This is the first step towards creating an intermediate representation that can be used to generate HTML, SVG, and Canvas commands for rendering. By generating spans that contain the width of the spaces instead of relying on CSS sibling rules we'll be able to one day replaces the spans with intermeidate 'Glue' nodes (in a later PR). An added benefit of this approach is that is enables us to programmatically change the values for thinspace, mediumspace, and thickspace which will allow us to implement the \setlength command. Test Plan: - npm test - dockers/Screenshotter/screenshotter.sh --verify

k4b7 · 2018-01-16T17:01:48Z

There are some failing tests for this change that I have to work through, in particular:

BinCancellation BoldSymbol Not OperatorName OverUnderset SizingBaseline StrikeThrough StrikeThroughColor

k4b7 · 2018-01-21T20:20:49Z

Somethings have improved in the screenshots:

spaces between operators and strike through items is closer to what LaTeX does

Somethings are worse:

\not is in the wrong position when followed by a group
spaces around + in SizingBaseline now depend on the sizing, but shouldn't
spaces around dot operators in StyleSwitchings should be symmetric but aren't

Somethings are neutral:

changes to OverUnderset are minimal

…creenshots

k4b7 · 2018-01-22T00:15:00Z

\not is now looking right on for Chrome and Firefox. We need to change the glyph to a non-combining character to fix Safari.

k4b7 · 2018-01-22T00:16:36Z

SizingBaseline seems to be better too, but not perfect.

k4b7 · 2018-01-22T00:21:51Z

All tests are passing now, but there's still a bit of cleanup work that I'd like to do first. In particular:

extract helper function for generating glue
do the TODO for handling styling commands the same as sizing commands in buildExpression

k4b7 · 2018-01-22T00:22:43Z

src/buildHTML.js

+        if (groups[i].value === "\u0338") {
+            groups[i].style.position = "absolute";
+            // TODO(kevinb) fix this for Safari by switching to a non-combining
+            // character for \not.


I think TODO makes the most sense in a separate PR since font changes are involved.

k4b7 · 2018-01-22T00:23:07Z

src/buildHTML.js

+            // TODO(kevinb) fix this for Safari by switching to a non-combining
+            // character for \not.
+            // 0.194440 is the width for the \not character
+            groups[i].style.paddingLeft = `${1.0 - 0.194440}em`;


Should grab the width metric for this character from fontMetricsData.js.

k4b7 · 2018-01-22T00:23:32Z

src/buildHTML.js

        }
    }

    // Process \\not commands within the group.


I'm happy how this became a lot simpler.

k4b7 · 2018-01-22T00:25:33Z

src/functions/delimsizing.js

+                calculateSize(html.spacings[lastChildType]["mclose"], options);
+            glue.style.marginRight = `${dimension}em`;
+            inner.push(glue);
+        }


I wish that delimsizing.js didn't have to know about this. I was thinking about moving space inserting to be a post-processing step on the HTML tree, but then we don't have access to sizing/styling data so I think we'll just have to live with this.

k4b7 · 2018-01-22T03:53:22Z

This is ready for review now.

k4b7 · 2018-01-22T19:51:42Z

@kohler if you have time, could you look at this PR since you did original work to fix the spacing?

ry-randall

Love that this is the first step towards an IR

ry-randall · 2018-01-22T10:43:04Z

src/buildHTML.js


+const thinspace = {
+    number: 3,
+    unit: "mu",


What was the reasoning for switching from em to mu? Although, looking at whole numbers is definitely cleaner.

I originally used em and had comments and math. This seemed a lot simpler and maps directly to LaTeX definitions.

ry-randall · 2018-01-23T12:09:48Z

src/buildCommon.js

    return text;
 };

+const makeGlue = (measurement: Measurement, options: Options): domTree.span => {


I think a comment here would be useful. The term glue might be confusing to people.

ry-randall · 2018-01-23T12:10:31Z

src/buildHTML.js

 export const buildExpression = function(expression, options, isRealGroup) {
    // Parse expressions into `groups`.
-    const groups = [];
+    const rawGroups = [];


What was the reasoning for the rename?

I thought about insert spaces into this array, but inserts mess with indices and I thought the code would be cleaner copying non space groups over to a new array as the spaces were being inserted.

ry-randall · 2018-01-23T12:11:00Z

src/buildHTML.js

        const output = buildGroup(group, options);
        if (output instanceof domTree.documentFragment) {
-            Array.prototype.push.apply(groups, output.children);
+            rawGroups.push(...output.children);


ry-randall · 2018-01-23T12:19:10Z

static/katex.less

    @thickspace: 0.27778em;     // 5mu

-    // These spacings apply in textstyle and displaystyle.
-    .mord {


❤️ Love that this is gone

ry-randall · 2018-01-23T12:26:51Z

src/buildHTML.js

    }

    // Process \\not commands within the group.
-    // TODO(kevinb): Handle multiple \\not commands in a row.


Assuming this comment handled by this PR?

ry-randall · 2018-01-23T12:26:57Z

src/buildHTML.js


    // Process \\not commands within the group.
-    // TODO(kevinb): Handle multiple \\not commands in a row.
-    // TODO(kevinb): Handle \\not{abc} correctly.  The \\not should appear over


Same as above

See the new screenshots for Not. :)

ry-randall · 2018-01-23T12:28:34Z

src/buildHTML.js

+    unit: "mu",
+};
+
+export const spacings = {


This all looking super clean 👍 . Since this is just data with no dependencies. Perhaps it could be extracted outside of buildHTML into it's own module? Maybe a spacing.js?

ry-randall · 2018-01-23T12:30:46Z

src/buildHTML.js

-                Array.prototype.push.apply(groups, spaces);
-                break;
+    // spacing (e.g., "add thick space between mord and mrel").
+    const nonSpaces =


The comment above indicates that this is spacing, but the variable name is nonSpaces. Is that intentional?

I'll expand on this comment. Basically, we want to look at the relationship between non space groups to determine what spaces to insert. This means we want to disregard explicit spaces when determining which implicit spaces to add.

ry-randall · 2018-01-23T12:34:38Z

src/buildHTML.js

+    },
+};
+
+const tightSpacings = {


What's the reasoning for distinguishing these from spacings?

The spacings are different when we're in \scriptstyle or \scriptscriptstyle. I'm open to suggestions of other ways to organize these alternate values.

k4b7 · 2018-01-23T13:20:51Z

@rrandallcainc thanks for the review. I'll make the requested changes this evening.

ylemkimon · 2018-01-23T13:35:07Z

src/buildHTML.js

-                break;
+    // spacing (e.g., "add thick space between mord and mrel").
+    const nonSpaces =
+        rawGroups.filter(group => group && group.classes[0] !== "mspace");


I think we can remove using dummy spans in the enclosing element like:
https://github.com/Khan/KaTeX/blob/732f2a84ccb000099d29fb1f61dd02e611fab64b/src/functions/href.js#L34-L65
and handle spacing here.

Good catch. ~~I'm guessing that this diff breaks \href spacing which means we should probably have a test for it.~~

~~I should probably update getTypeOfDomTree to be able to get the class of the rightmost or leftmost element in the tree.~~

@ylemkimon as for removing the dummy spans, it's still possible that the anchor wraps a fragment. makeAnchor always returns a single element even when it has multiple children so I believe we still need to do this otherwise I don't see how we'd communicate there being atoms of different classes at either end of the anchor's content.

k4b7 · 2018-01-24T01:17:01Z

@rrandallcainc this is ready for another pass.

ylemkimon · 2018-01-24T04:52:24Z

src/functions/href.js

                classes = [first];
            } else { // Case 3: both ends have different types.
+                // TODO(kevinb): figure out a better way to communicate this
+                // information to buildHTML.js#buildExpression.


I think we can just return new buildCommon.makeAnchor(href, [], elements, options) in makeAnchor()

I was confused at first, but this makes sense given your comments in getTypeOfDomTree.

Currently spacings around \href are duplicated as dummy spans are created.

ylemkimon · 2018-01-24T04:53:54Z

src/buildHTML.js

 // Return math atom class (mclass) of a domTree.
-export const getTypeOfDomTree = function(node) {
+export const getTypeOfDomTree = function(node, side = "right") {
    if (node instanceof domTree.documentFragment) {


and get leftmost or rightmost children type for node instanceof domTree.anchor too.

Also I think we can define a special class name for span that encloses elements without affecting spacings.

I ran into some issues with spans so I'm going to leave them out for now.

kohler

I gave this a once-over (no time for in depth) and it looks good!

kohler · 2018-01-24T21:23:55Z

src/buildHTML.js

-                // Otherwise, put any spaces back at the end of the groups.
-                Array.prototype.push.apply(groups, spaces);
-                break;
+    // Ignore explicit spaces (e.g., \;, \,) when determine what implicit


“when determining”

kohler · 2018-01-24T21:24:48Z

src/buildHTML.js

+            const right = getTypeOfDomTree(nonSpaces[j + 1], "left");
+
+            // We use buildExpression inside of sizingGroup, but it returns a
+            // document fragement of elements.  sizingGroup sets `isRealGroup`


“fragment”

k4b7 · 2018-01-24T21:34:25Z

@kohler thanks!

I'll fix all the nits before merging.

ry-randall · 2018-01-25T00:53:25Z

@kevinbarabash This is looking good. I'll do one more pass tomorrow morning.

EDIT: Did it now

ry-randall · 2018-01-25T01:01:44Z

src/buildCommon.js

    return text;
 };

+// Glue is a concept from TeX which is a flexible space between elements in


Cool, didn't know this!

ry-randall

LGTM

k4b7 · 2018-01-25T04:09:32Z

Thanks everyone for reviewing this. This should make #687 much more tractable now.

ry-randall · 2018-01-25T17:43:39Z

@kevinbarabash Thanks for implementing. Would extracting these: https://github.com/Khan/KaTeX/blob/master/static/katex.less#L217 follow a similar pattern?

k4b7 · 2018-01-25T19:46:37Z

@rrandallcainc yup. Anywhere where we create a span using one of those styles could be replaced with a call to makeGlue.

ylemkimon · 2018-01-26T08:56:55Z

src/buildHTML.js

+// leftmost node in the fragment.
+// 'mtight' indicates that the node is script or scriptscript style.
+export const isLeftTight = function(node) {
+    if (node instanceof domTree.documentFragment) {


Can the node be a domTree.documentFragment? Aren't they flattened at the start of buildExpression?

Good catch.

@kevinbarabash It seems that node's children can be a domTree.documentFragment, e.g., \phantom{x}^2.

ylemkimon · 2018-01-26T09:00:53Z

src/buildHTML.js

+    for (let i = 0; i < nonSpaces.length; i++) {
+        if (isBin(nonSpaces[i])) {
+            if (isBinLeftCanceller(nonSpaces[i - 1], isRealGroup)
+                    || isBinRightCanceller(nonSpaces[i + 1], isRealGroup)) {


I think these functions should take domTree.documentFragment and domTree.anchor into account

domTree.anchor yes, but as noted in your previous comment, documentFragments get flattened.

ylemkimon · 2018-01-27T17:54:41Z

src/buildHTML.js

+                    }
+
+                    const glue = buildCommon.makeGlue(
+                        spacings[left][right], glueOptions);


Is using spacings[left][right] instead of space an intended behavior?

Yes. Thanks for catching this. It's obvious that we don't have any tests for the "tight" situation.

Tracking in #1102.

Kevin Barabash added 2 commits January 21, 2018 15:07

fixed failures in BinCancellation, BoldSymbol, and OperatorName

0969303

update screenshots

590e077

Kevin Barabash added 4 commits January 21, 2018 17:33

don't use current size when determining size of spaces, update more s…

88a34b5

…creenshots

fix spacing in SizingBaseline and StyleSwitching

f76f3cb

actually do the right thing for sizing groups

2f03357

fix \not for Chrome and Firefox

9751805

k4b7 mentioned this pull request Jan 22, 2018

Spacing around underset/overset commands is wrong #1081

Open

k4b7 commented Jan 22, 2018

View reviewed changes

ronkok mentioned this pull request Jan 22, 2018

bounding box is wrong for \cancel groups #1080

Closed

do TODOs

f596115

k4b7 added the GH Review: review-needed label Jan 22, 2018

k4b7 mentioned this pull request Jan 22, 2018

support \setlength and measurement constants #687

Open

Merge branch 'master' into js_spacing

33e8d81

ry-randall reviewed Jan 23, 2018

View reviewed changes

ylemkimon reviewed Jan 23, 2018

View reviewed changes

k4b7 and others added 3 commits January 23, 2018 20:02

Merge branch 'master' into js_spacing

752481c

address feedback from the code review

9474730

fix issue in delimsizing.js

7606be7

add TODO to think about a better solution in href.js

75dc6ea

ylemkimon reviewed Jan 24, 2018

View reviewed changes

k4b7 added GH Review: needs-revision and removed GH Review: review-needed labels Jan 24, 2018

ylemkimon mentioned this pull request Jan 24, 2018

[PoC]Plugin architecture using webpack (HTML \class extension) #1074

Closed

4 tasks

kohler approved these changes Jan 24, 2018

View reviewed changes

ry-randall reviewed Jan 25, 2018

View reviewed changes

ry-randall approved these changes Jan 25, 2018

View reviewed changes

fix typos, simplify href, be honest about paddingLeft for \not

d09058b

k4b7 added GH review: accepted and removed GH Review: needs-revision labels Jan 25, 2018

k4b7 merged commit 63f541b into master Jan 25, 2018

k4b7 deleted the js_spacing branch January 25, 2018 04:09

ylemkimon reviewed Jan 26, 2018

View reviewed changes

ylemkimon reviewed Jan 27, 2018

View reviewed changes

edemaine mentioned this pull request Oct 27, 2021

20% of katex spans can be removed without visual change and can significantly improve page speed and DOM size #3344

Open

Uh oh!

Conversation

k4b7 commented Jan 16, 2018

Uh oh!

k4b7 commented Jan 16, 2018

Uh oh!

k4b7 commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k4b7 commented Jan 22, 2018

Uh oh!

k4b7 commented Jan 22, 2018

Uh oh!

k4b7 commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Jan 22, 2018

Uh oh!

k4b7 commented Jan 22, 2018

Uh oh!

ry-randall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Jan 24, 2018

Uh oh!

ylemkimon Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

k4b7 commented Jan 21, 2018 •

edited

Loading

k4b7 commented Jan 22, 2018 •

edited

Loading

k4b7 Jan 23, 2018 •

edited

Loading

k4b7 Jan 23, 2018 •

edited

Loading

ylemkimon Jan 24, 2018 •

edited

Loading

ylemkimon Jan 24, 2018 •

edited

Loading

ry-randall commented Jan 25, 2018 •

edited

Loading