Refactor `buildExpression` to depth-first traverse groups (nodes), fix spacings and \begingroup...\endgroup by ylemkimon · Pull Request #1706 · KaTeX/KaTeX

ylemkimon · 2018-09-07T18:04:33Z

Fixes #1108 and fixes #1679. This also allows the implementation of semi-simple (partial, transparent, i.e., doesn't affect spacing around) groups like \class and \cssId in #1437.

This PR refactors buildExpression to depth-first traverse (DFS) groups (nodes), going into Anchor and possibly other nodes in the future.

Also, this PR fixes \begingroup...\endgroup, which only can be ended by \endgroup and is semi-simple group.

ylemkimon · 2018-09-07T18:07:30Z

src/buildHTML.js

 export const getTypeOfDomTree = function(
    node: ?HtmlDomNode,
-    side: Side,
+    side: ?Side,


It'd be very nice to get rid of its only usage:
https://github.com/Khan/KaTeX/blob/f71f469d4b3ca97654702a0807e736d176f4c4c5/src/functions/supsub.js#L183-L187

The comment makes it sound kind of hacky. Is this how LaTeX resets text-align? Any thoughts on a better way to do this?

codecov · 2018-09-07T18:18:13Z

Codecov Report

Merging #1706 into master will decrease coverage by 0.09%.
The diff coverage is 95.89%.

@@            Coverage Diff            @@
##           master    #1706     +/-   ##
=========================================
- Coverage   94.01%   93.91%   -0.1%     
=========================================
  Files          78       78             
  Lines        4558     4570     +12     
  Branches      792      796      +4     
=========================================
+ Hits         4285     4292      +7     
- Misses        242      246      +4     
- Partials       31       32      +1

Flag	Coverage Δ
#screenshotter	`88.55% <91.78%> (-0.11%)`	⬇️
#test	`85.29% <95.89%> (-0.1%)`	⬇️

Impacted Files	Coverage Δ
src/functions/delimsizing.js	`97.64% <ø> (ø)`	⬆️
src/parseNode.js	`84.21% <ø> (ø)`	⬆️
src/macros.js	`97.05% <ø> (-0.02%)`	⬇️
src/Parser.js	`97.2% <100%> (ø)`	⬆️
src/functions/ordgroup.js	`100% <100%> (ø)`	⬆️
src/buildHTML.js	`95.77% <95.38%> (-3.46%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f3ab13...2f5c72b. Read the comment docs.

src/buildHTML.js

ylemkimon · 2018-09-08T12:23:29Z

src/buildHTML.js

-            right.classes[0] = "mord";
+    }, {node: dummyPrev}, dummyNext);
+
+    traverseNodes(groups, (node, prev) => {


It is possible to traverse only once, but it's more complicated since it needs to retain previous spacing information. I'm not sure which is more optimal.

My preference is for multiple passes with simpler code.

src/Parser.js

k4b7 · 2018-09-23T00:05:54Z

src/Parser.js

+        } else if (!optional && this.nextToken.text === "\\begingroup") {
+            // A group formed by /begingroup...\endgroup is a semi-simple group,
+            // which doesn't affect spacing in math mode, i.e., is transparent
+            semisimple = true;


Can you add a reference to the TeXBook or similar resource?

@kevinbarabash Semi-simple group is mentioned in TeX: The Program, but I really cannot find a reference regarding its spacing or the fact that it doesn't create a mathord.

Did you determine the behavior by seeing what LaTeX does for different test cases?

FWIW, this behavior matches my experience testing with LaTeX. I don't see it documented in The TeXbook. But the TeX source code definitely calls groups started with \begingroup "semi-simple groups" (search for begin_group), and it definitely does something special with them (search for semi_simple_group). But I'm no expert at that code...

@kevinbarabash I've added a link to stack exchange answer, as it has enough up-votes.

k4b7 · 2018-09-23T00:09:54Z

src/buildHTML.js


-    return groups;
+// Check if given node is a partial group, i.e., does not affect spacing around,
+// like fragment.


I mis-parsed this comment on the first read. The intent is that partial groups behave the same as fragment, i.e. they don't affect the spacing around it.

@kevinbarabash I've removed like fragment.

k4b7 · 2018-09-23T00:11:11Z

src/buildHTML.js

+// like fragment.
+const checkPartialGroup = function(
+    node: HtmlDomNode,
+): ?(DocumentFragment<HtmlDomNode> | Anchor) {


Maybe we should define type PartialGroup = DocumentFragment<HtmlDomNode> | Anchor.

@kevinbarabash I think it's better to have it close to instanceof checks.

k4b7 · 2018-09-23T00:13:04Z

src/buildHTML.js

+
+    traverseNodes(groups, (node, prev) => {
+        const prevType = getTypeOfDomTree(prev);
+        const type = getTypeOfDomTree(node);


Why don't we need the side argument here?

@kevinbarabash The callback function will be only called on non-partial groups.

That makes sense, but it's not obvious. Could you add a comment here before merging this?

k4b7 · 2018-09-23T00:27:23Z

src/buildHTML.js

+// previous node as arguments, optionally returning a node to insert after the
+// previous node. `prev` is an object with the previous node and `insertAfter`
+// function to insert after it. `next` is a node that will be added to the right.
+const traverseNodes = function(


It might be worth unit testing this. Also the name is very general but it some logic that's super specific to modifying spacing. Can you change the name and update the comment to reflect this?

@kevinbarabash I've renamed the function to traverseNonSpaceNodes and added comments about its usage. We really need a way to unit-test functions.

src/buildHTML.js

k4b7 · 2018-09-23T00:38:44Z

test/katex-spec.js

+
+    it("should not affect spacing in math mode", function() {
+        const built = getBuilt`\begingroup x+ \endgroup y`;
+        expect(built).toMatchSnapshot();


Maybe compare this build tree with that of x + y,

@kevinbarabash Fixed.

k4b7 · 2018-09-23T00:40:08Z

test/katex-spec.js

+
+    it("should not affect spacing around", function() {
+        const built = getBuilt`a\href{http://example.com/}{+b}`;
+        expect(built).toMatchSnapshot();


Snapshot looks good.

k4b7

LGTM. Requesting one additional comment about why we don't need to pass the side argument to getTypeOfDomTree on lines 120-121 of buildHTML.js.

k4b7 · 2018-10-29T04:32:14Z

LGTM. Requesting one additional comment about why we don't need to pass the side argument to getTypeOfDomTree on lines 120-121 of buildHTML.js.

I forgot about this. I guess it wasn't that important.

ylemkimon commented Sep 7, 2018

View reviewed changes

ylemkimon added the GH Review: review-needed label Sep 7, 2018

ylemkimon commented Sep 8, 2018

View reviewed changes

src/buildHTML.js Show resolved Hide resolved

ylemkimon added this to the Version 0.10.0 milestone Sep 8, 2018

Refactor buildExpression to depth-first traverse groups (nodes)

35b628b

ylemkimon commented Sep 8, 2018

View reviewed changes

ylemkimon mentioned this pull request Sep 8, 2018

Add \class and \cssId on non-strict mode #1437

Closed

Fix \begingroup...\endgroup

1a35c03

ylemkimon changed the title ~~Refactor buildExpression to depth-first traverse groups (nodes), fix spacings~~ Refactor buildExpression to depth-first traverse groups (nodes), fix spacings and \begingroup...\endgroup Sep 16, 2018

k4b7 reviewed Sep 23, 2018

View reviewed changes

ylemkimon mentioned this pull request Sep 23, 2018

Wrap document fragment where classes, attributes, or styles are applied #1707

Merged

Rename traverseNodes to traverseNonSpaceNodes, update comments and tests

6bd576c

edemaine mentioned this pull request Oct 13, 2018

Release 0.10.0 #1544

Closed

ylemkimon added 3 commits October 14, 2018 01:44

Merge branch 'master' into build-expression

5bab6c9

Update Parser.js

63fc7be

Update katex-spec.js.snap

d7bf897

k4b7 approved these changes Oct 28, 2018

View reviewed changes

k4b7 added GH review: accepted and removed GH Review: review-needed labels Oct 28, 2018

k4b7 assigned ylemkimon Oct 28, 2018

k4b7 added 2 commits October 27, 2018 21:56

Merge branch 'master' into build-expression

c7eeba8

Merge branch 'master' into build-expression

2f5c72b

k4b7 merged commit a3215b2 into KaTeX:master Oct 29, 2018

ylemkimon mentioned this pull request Dec 27, 2018

Fix \hphantom width #1809

Merged

ylemkimon deleted the build-expression branch March 21, 2019 14:27

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

Uh oh!

Conversation

ylemkimon commented Sep 7, 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!

codecov bot commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ylemkimon Sep 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!

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!

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!

k4b7 left a comment

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ylemkimon commented Sep 7, 2018 •

edited

Loading

codecov bot commented Sep 7, 2018 •

edited

Loading

ylemkimon Sep 23, 2018 •

edited

Loading