Support \includegraphics by ronkok · Pull Request #1620 · KaTeX/KaTeX

ronkok · 2018-08-15T19:26:32Z

This PR provides partial support for the \includegraphics function from the graphix package. Thus it addresses issue #898 and possibly #1596.

In the first argument, this PR supports the following key/value pairs:

height=<size>
width=<size>
totalheight=<size>
alt=<string>

Items 1 thru 3 in that list match some of the arguments in the graphix package. The provision for alt is non-standard. Other graphix options such as scale or angle are not supported. Perhaps that could be done for a future release.

Given this input:

\def\logo{\includegraphics[height=0.8em, totalheight=0.9em, width=0.9em, alt=KA logo]{https://cdn.kastatic.org/images/apple-touch-icon-57x57-precomposed.new.png}}
\def\logoB{\includegraphics[height=0.4em, totalheight=0.9em, width=0.9em, alt=KA logo]{https://cdn.kastatic.org/images/apple-touch-icon-57x57-precomposed.new.png}}
\begin{array}{l}
\underline{A\logo} + \sqrt{\logo} + \tfrac{A\logo}{\logo}\\[1em]
\underline{A\logoB} + \sqrt{x\logoB} + \tfrac{A\logoB}{\logoB}
\end{array}

We get the following rendering:

ronkok · 2018-08-15T19:30:53Z

This PR triggers many flow errors. I've done a good faith effort to fix them and succeeded at fixing a few. I don't intend to spend any more time on it. If someone wishes to collaborate with me on this PR and fix the flow errors, I would welcome that.

k4b7 · 2018-08-17T00:10:48Z

@ronkok this is awesome! I'm glad to see it was too difficult. One thing that you could do to make it easier for people collaborate on this PR is push your branch to Khan/KaTeX and create a PR from that. This will allow other people with push access to pushing updates to this PR without having to create PRs against your fork.

edemaine · 2018-08-17T07:04:45Z

@kevinbarabash We can push directly to ronkok/KaTeX as follows:

git remote add ronkok git@github.com:ronkok/KaTeX.git
git fetch ronkok
git checkout ronkok/includegraphics
...make changes, commit...
git push ronkok includegraphics  # I think this is right; I might be off

edemaine · 2018-08-17T07:09:52Z

I feel like embedding arbitrary images is also a potential security hole, in that it could be used for tracking users. This is a third situation (I believe) where we might want some kind of security option. (The others are #1437 and #1596.)

In this case, we should probably also restrict the URLs to use protocols from the allowedProtocols setting. But I also fear that's not enough -- links are relatively safe, while embeds less so...

k4b7 · 2018-08-17T13:12:31Z

@edemaine I didn't realize that. I would've assumed that @ronkok would've needed to explicitly grant push access to his repo, but maybe we have push access to that specific branch due b/c it's being used as a PR against this repo. I'll give that a try this evening.

codecov · 2018-08-30T04:16:43Z

Codecov Report

Merging #1620 into master will decrease coverage by 0.21%.
The diff coverage is 83.15%.

@@            Coverage Diff             @@
##           master    #1620      +/-   ##
==========================================
- Coverage   93.89%   93.67%   -0.22%     
==========================================
  Files          79       80       +1     
  Lines        4571     4666      +95     
  Branches      796      813      +17     
==========================================
+ Hits         4292     4371      +79     
- Misses        247      261      +14     
- Partials       32       34       +2

Flag	Coverage Δ
#screenshotter	`88.62% <82.1%> (+0.06%)`	⬆️
#test	`85.04% <73.68%> (-0.24%)`	⬇️

Impacted Files	Coverage Δ
src/functions.js	`100% <ø> (ø)`	⬆️
src/mathMLTree.js	`98.3% <ø> (ø)`	⬆️
src/parseNode.js	`84.21% <ø> (ø)`	⬆️
src/Parser.js	`97.23% <100%> (+0.03%)`	⬆️
src/domTree.js	`88.58% <59.09%> (-4.01%)`	⬇️
src/functions/includegraphics.js	`89.85% <89.85%> (ø)`

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 f628ca1...3df6573. Read the comment docs.

k4b7 · 2018-08-30T04:21:42Z

src/functions/includegraphics.js

+import type {CssStyle} from "../domTree";
+
+// TODO: make it easier for commands to get the macro expanded text
+// from inside an ordgroup argument.


@edemaine how difficult do you think this would be? The code below for stringFromParseGroup is incomplete since it doesn't handle nested groups. Also, it seems inefficient b/c we're traversing the tree when we should be able to slice part of the expanded token stream and more easily concatenate those tokens into a string.

k4b7 · 2018-08-30T04:24:02Z

src/functions/includegraphics.js

 import mathMLTree from "../mathMLTree";
+import {assertNodeType, checkNodeType} from "../parseNode";

-const stringFromParseGroup = function(textArray: [ParseNode<"textord">]): string {


@ronkok [T] means a 1-tuple of type T where as T[] is an array of T.

k4b7 · 2018-08-30T04:25:45Z

src/functions/includegraphics.js

-        const style = {height: height + depth + "em"};
+        const style: CssStyle = {height: height + depth + "em"};
        if (width > 0) {
            style.width = width + "em";


The flow errors here stemmed from the fact that when you create an object literal, flow considers that to be its type and complains about adding new properties after the fact. To get around this you have to specify the type explicitly.

k4b7 · 2018-08-30T04:27:00Z

src/functions/includegraphics.js

            const width = calculateSize(group.width, options);
            node.setAttribute("width", width + "em");
        }
-        node.setAttribute("src", group.path);


flow actually helped me find this error.

k4b7

@ronkok sorry for the delay. I'm requesting the use of "url" (if possible) for one for one of the arg types and tests for the failure modes.

k4b7 · 2018-09-05T01:42:55Z

docs/support_table.md

 |\implies|$P\implies Q$|`P\implies Q`|
 |\in|$\in$||
-|\includegraphics|<span style="color:firebrick;">Not supported</span>|[Issue #898](https://github.com/Khan/KaTeX/issues/898)|
+|\includegraphics|$\includegraphics[height=0.8em, totalheight=0.9em, width=0.9em, alt=KA logo]{https://cdn.kastatic.org/images/apple-touch-icon-57x57-precomposed.new.png}$|`\includegraphics[height=0.8em, totalheight=0.9em, width=0.9em, alt=KA logo]{https://cdn.kastatic.org/images/apple-touch-icon-57x57-precomposed.new.png}`|


I assume this also works with relative URLs as well.

I checked this on https://deploy-preview-1620--katex.netlify.com/docs/next/support_table.html#i and it isn't rendering. I think updating this branch should fix it since #1631 was merged.

Disregard my comment about this not working on netlify, I was looking in the wrong spot. 😅

k4b7 · 2018-09-05T01:48:45Z

src/functions/includegraphics.js

+    props: {
+        numArgs: 1,
+        numOptionalArgs: 1,
+        argTypes: ["text", "text"],


I forgot that we have a url arg type that you could use.

When I first wrote this, I tried to specify a url arg type but got an error. I'll try again and see if I can identify the problem.

@ronkok The url argument is hardcoded in the Lexer and the Parser:

https://github.com/Khan/KaTeX/blob/f71f469d4b3ca97654702a0807e736d176f4c4c5/src/Lexer.js#L49-L52

https://github.com/Khan/KaTeX/blob/9b7e10e4f753786f125254a79d7cd88acc08afd4/src/Parser.js#L916-L917

You'll need to update regexes.

Regarding a url arg type. There really isn't any such thing in KaTeX. I've experimented a bit with href.js and replaced argTypes: ["url"] with argTypes: ["text"] and argTypes: ["chaos"]. They both run fine. It doesn't matter what arg type is specified in href.js because Parser.js ignores that specification.

What is really happening is that Lexer.js is preempting the usual logic for functions \href and \url. That urlFunctionRegexString captures both the function name and the first argument group. Parser.js then handles the result as an exception to the regular run of parsing code.

That RegEx, by the way, is not detailed enough to ensure that the argument is a valid url. It seems to be just enough to capture the function name and its first argument group.

So I question the usefulness of changing the code in this PR. The \href example lexes some arbitrary text then parses it as an exception to regular input. This PR does the same thing; it just does it in a different file. And, I would argue, by placing this exception code in the includegraphics.js file, I am grouping exeption code together in manner similar to the way we currently use defineFunction.

@ronkok In Parser, there are escaping routines and allowed protocol checks:
https://github.com/Khan/KaTeX/blob/9b7e10e4f753786f125254a79d7cd88acc08afd4/src/Parser.js#L926-L938

@ylemkimon That's an excellent point. I'll move that code into a utility function so that it is available to includegraphics.

If someone wants to refactor Lexer and Parser in an way that creates an actual url arg type, that would be great, but that's not for me.

Sounds good.

argTypes: ["chaos"]. They both run fine.

That feels like a bug. @ronkok could you file an issue for this?

src/functions/includegraphics.js

k4b7 · 2018-09-05T01:57:07Z

src/functions/includegraphics.js

+                if (keyVal.length === 2) {
+                    const str = keyVal[1].trim();
+                    switch (keyVal[0].trim()) {
+                        case "alt":


Thanks for adding this as an option.

k4b7 · 2018-09-05T02:08:38Z

src/functions/includegraphics.js

+        if (group.totalheight.number > 0) {
+            depth = calculateSize(group.totalheight, options) - height;
+            depth = Number(depth.toFixed(2));
+        }


I see, totalheight can be used to vertically shift the image within the group.

k4b7 · 2018-09-05T02:10:23Z

src/functions/includegraphics.js

+
+        const style: CssStyle = {height: height + depth + "em"};
+        if (width > 0) {
+            style.width = width + "em";


I forgot what calculateSize did for a second and was confused why we threw away the supplied units, but then remember.

k4b7 · 2018-09-05T02:10:50Z

src/functions/includegraphics.js

+        if (width > 0) {
+            style.width = width + "em";
+            // Over-rule any max-width: 100% that the img may inherit.
+            style.maxWidth = width + "em";


Should we do the same for maxHeight? It seems less likely to be an issue.

Or we could add a few lines to the KaTeX CSS that insulate a .katex .img from the inherited .img CSS. Something like what PR #1083 did for SVGs. Then this line of code would be unnecessary.

k4b7 · 2018-09-05T02:12:30Z

src/functions/includegraphics.js

+        return node;
+    },
+    mathmlBuilder: (group, options) => {
+        const node = new mathMLTree.MathNode("mglyph", []);


This seems like an odd choice since it's not really a glyph. I assume this what other people are doing when they add images to MathML.

This seems like an odd choice since it's not really a glyph.

Agreed. I used it because all the other MathML elements seemed even less suited than <mglyph>.

I assume this what other people are doing when they add images to MathML.

I haven't found any examples where people add general images to MathML. For glyphs specfically, then yes, the MathML spec does have a section. But general images? We may be breaking new ground.

k4b7 · 2018-09-05T02:18:27Z

test/katex-spec.js

+    it("should produce mords", function() {
+        expect(getBuilt(img)[0].classes).toContain("mord");
+    });
+});


Can you add some failing tests to check the exceptions thrown in certain errors cases. Maybe try https://jestjs.io/docs/ro/next/expect#tothrowerrormatchinginlinesnapshot apparently it will update the source code of the test file with a string version of the exception being thrown when you run yarn jest -u.

I'll work something up.

@kevinbarabash In order for jest to use inline snapshots, a project must have prettier installed. [ Reference]. I'll look for another way to get some confidence in the exceptions.

I didn't know that. In that case disregard my suggestion.

ronkok · 2018-09-11T23:52:01Z

In the latest commit, I've changed the way that the function matches the url argument. Instead of specifying a text arg type, the PR now uses the same Lexer RegEx as used by the \href and \url functions. Now, URL strings can contain a % character, useful for URL escapes. As an added bonus, we re-use the earlier code that checks the URL protocol.

The PR still does not do anything to address @edemaine's security concerns but I think it is otherwise ready to go.

ronkok · 2018-10-23T04:01:23Z

I've updated this PR to coordinate with the recently added "raw" ArgType. (Thanks!) The graphics are running again and I've eliminated all but one of the flow errors, which is better than my past performance.

I still am unsure about how to address the security issue. I suspect that's a little above my pay grade.

k4b7 · 2018-10-29T04:50:09Z

@ronkok I'll look into this next week.

k4b7 · 2018-10-29T05:15:33Z

Looks like the netlify issue fixed itself. The only thing left an issue with the flow typing.

ronkok · 2018-10-29T14:10:05Z

No worries.

k4b7 · 2018-10-30T03:22:43Z

src/Parser.js

-                return {type: "raw", mode: "text",
-                    string: this.parseStringGroup("raw", optional, true).text};
+            case "raw": {
+                const token = this.parseStringGroup("raw", optional, true);


@ronkok the issue was that the return type for parseStringGroup is ?Token so a null check was required.

k4b7 · 2018-10-30T03:23:26Z

src/Parser.js

+                        string: token.text,
+                    };
+                } else {
+                    throw new ParseError("Expected raw group", this.nextToken);


@ronkok is it possible to construct a test case where this would happen?

@kevinbarabash Nothing comes readily to mind. I'll give it a try tomorrow.

@kevinbarabash I can trigger this parse error by writing a Unicode combining character as the first character in the raw string. Do you want me to write that test into katex-spec.js?

Awesome! Yes please.

Test is added. I think I'm done here. Thanks again to @ylemkimon for adding the raw arg type.

…o includegraphics

k4b7 · 2018-11-01T22:43:20Z

@ronkok thanks for adding this feature. I'm sure people will find some really great uses for it.

edemaine · 2018-11-01T22:59:35Z

As I wrote above:

I feel like embedding arbitrary images is also a potential security hole, in that it could be used for tracking users. This is a third situation (I believe) where we might want some kind of security option. (The others are #1437 and #1596.)

I think we really need some kind of security option (like we did with strict) before this should be released. Links and embedded images are quite different, so our existing URL restrictions don't suffice.

k4b7 · 2018-11-01T23:09:08Z

Agree. We should make sure we have a security option before the next release.

This reverts commit f713f32.

ronkok added 2 commits August 15, 2018 12:10

Support \includegraphics

a3e95d1

Merge branch 'master' into includegraphics

1e864c0

ronkok added 4 commits August 15, 2018 12:34

Update screenshots

24a2cd9

Fix lint errors

cd640f8

Fix more lint errors

2b8493f

Fix lint errors take 3

2d77207

ronkok and others added 7 commits August 17, 2018 08:13

Merge branch 'master' into includegraphics

083c895

Add documentation, error messages

7c4433f

Improve CSS

6a2ce63

Resolve merge conflicts

8a569ac

Update domTree to coordinate with PR #1633

c242aa7

Update screenshots

e0d7e6d

fix flow errors

20b3e86

k4b7 reviewed Aug 30, 2018

View reviewed changes

k4b7 added the GH Review: review-needed label Aug 30, 2018

k4b7 reviewed Aug 30, 2018

View reviewed changes

ronkok mentioned this pull request Sep 4, 2018

Can \equiv be made less blurred? #1321

Open

k4b7 requested changes Sep 5, 2018

View reviewed changes

k4b7 assigned ronkok Sep 5, 2018

k4b7 added GH Review: needs-revision and removed GH Review: review-needed labels Sep 5, 2018

ronkok added 2 commits September 6, 2018 08:38

Merge branch 'master' into includegraphics

c653073

Use same RegEx as href

6d6f0f4

ronkok added the blocked label Sep 13, 2018

ylemkimon mentioned this pull request Sep 21, 2018

Refactor Parser #1723

Merged

fix merge conflicts

868d843

ronkok removed the blocked label Oct 23, 2018

ronkok added 2 commits October 22, 2018 20:52

Accept raw ArgType

8e315f1

Fix lint error

8d2f98d

Merge branch 'master' into includegraphics

807848a

fix flow error

9da2df3

k4b7 reviewed Oct 30, 2018

View reviewed changes

ronkok added 4 commits November 1, 2018 10:42

Merge branch 'master' into includegraphics

fee34b2

Merge branch 'master' into includegraphics

d41e4e4

Merge branch 'includegraphics' of https://github.com/ronkok/KaTeX int…

5c3e80f

…o includegraphics

Add test

3df6573

k4b7 added GH review: accepted and removed GH Review: needs-revision labels Nov 1, 2018

k4b7 approved these changes Nov 1, 2018

View reviewed changes

k4b7 merged commit f713f32 into KaTeX:master Nov 1, 2018

edemaine mentioned this pull request Nov 1, 2018

Security setting #1771

Closed

ronkok deleted the includegraphics branch November 7, 2018 19:18

edemaine mentioned this pull request Nov 24, 2018

support \includegraphics #898

Closed

ylemkimon added a commit that referenced this pull request Feb 21, 2019

Revert "Support \includegraphics (#1620)"

d4383d0

This reverts commit f713f32.

ylemkimon mentioned this pull request Jul 17, 2020

picture environment #1346

Open

Uh oh!

Conversation

ronkok commented Aug 15, 2018

Uh oh!

ronkok commented Aug 15, 2018

Uh oh!

k4b7 commented Aug 17, 2018

Uh oh!

edemaine commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edemaine commented Aug 17, 2018

Uh oh!

k4b7 commented Aug 17, 2018

Uh oh!

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

Uh oh!

Codecov Report

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 left a comment • 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!

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!

ronkok commented Sep 11, 2018

Uh oh!

ronkok commented Oct 23, 2018

Uh oh!

k4b7 commented Oct 29, 2018

Uh oh!

edemaine commented Aug 17, 2018 •

edited

Loading

codecov bot commented Aug 30, 2018 •

edited

Loading

k4b7 left a comment •

edited

Loading