Implemented `\href' command by konn · Pull Request #923 · KaTeX/KaTeX

konn · 2017-10-09T12:28:51Z

Implemented \href command in LaTeX hyperref package.
It seems that LaTeX simply ignores special commands for hyper-linking and processes spacing as usual.
Since I don't make out how to simulate this spacing behaviour in KaTeX, I adopt the following strategy:

If it has the only element, then use its math-type;
if it has more than two elements, and the types of both ends of elements coincides, then use it as a class for the anchor tag;
otherwise, we inject empty span-tag aside both ends of generated anchor tag, both with the same classes as the first and the last ones respectively.

khanbot · 2017-10-09T12:29:06Z

CLA signature looks good 👍

edemaine · 2017-10-09T16:40:43Z

Thanks @konn for working on this. \href is definitely something we should support.

I don't understand what you mean by your three rules though. \href should not be setting class, should it? Isn't that the purpose of \class (see #90)?

Isn't the second argument of \href treated as if it's in \text? You should be able to use code similar to \text, \raisebox, etc. which switch to text mode.

konn · 2017-10-10T08:46:40Z

I don't understand what you mean by your three rules though. \href should not be setting class, should it? Isn't that the purpose of \class (see #90)?

Well, if you don't specify any math-type via class of an anchor tag, you get the different spacing behaviour than latex's default.

Consider the following input:

\documentclass[a4]{article}
\usepackage{hyperref}
\usepackage{amsmath,amssymb}
\begin{document}
$0 \href{http://en.wikipedia.org/wiki/Equals\_sign}{=} 1$.

$0 = 1$.
\end{document}

If you typeset the above by xelatex, lualatex (or latex + dvipdfmx with dvipdfmx argument for hyperref package), you get the following result, with the upper equals-sign linked to Wikipedia:

Here, we see that both \href'ed version of = and plain = result in the same spacing.
On the other hand, if we drop the math-type handling in KaTeX, we get the following result:

So, to get the same spacing adjustment, we have to set a class of anchor tag to mord, mrel or so.

Isn't the second argument of \href treated as if it's in \text? You should be able to use code similar to \text, \raisebox, etc. which switch to text mode.

As above, LaTeX's (more precisely, hyperref's) \href commands accept a math as its second argument, not only text. So we can't treat them as if they're in \text.

edemaine · 2017-10-10T16:43:49Z

Ah, now I understand where you're getting the classes. (I was worried that they were being specified by the user, but rather they're grabbed from the rendered arguments.) Now this seems like the right approach to me. Cool!

k4b7 · 2017-10-14T22:44:39Z

src/domTree.js

+
+        // Add the href
+        markup += " anchor=\"";
+        markup += utils.escape(this.href);


I see that you're following the existing code style, but you can use template strings to simplify things a bit:

markup += ` anchor="${utils.escape(this.href)}"`;

Since we're adding href to attributes in the constructor shouldn't the loop below that adds all other attributes also add this one?

Since we're adding href to attributes in the constructor shouldn't the loop below that adds all other attributes also add this one?

Ah, yes, that's right.
And I mistyped anchor instead of href above, and this mistakenly prevents the duplication of href attribute. I must fix this.

k4b7 · 2017-10-14T22:46:51Z

src/domTree.js

+        this.height = 0;
+        this.depth = 0;
+        this.maxFontSize = 0;
+        this.style = {};


I didn't realize how much boilerplate there was. We may want to add a base class for these DOM classes at some point in the future. No action required for this PR.

k4b7 · 2017-10-14T22:51:26Z

src/domTree.js

+
+        // Add the markup of the children, also as markup
+        for (let i = 0; i < this.children.length; i++) {
+            markup += this.children[i].toMarkup();


We also support for-of loops which might be simpler:

for (const child of this.children) { markup += child.toMarkup(); }

Neat! I will adopt it.

k4b7 · 2017-10-14T22:55:23Z

src/domTree.js

+
+        let styles = "";
+
+        // Add the styles, after hyphenation


I noticed that we're duplicating this as well as the adding of attributes. I've created a separate issue for it: #932.

k4b7 · 2017-10-14T22:57:12Z

src/functions/href.js

@@ -0,0 +1,81 @@
+// @flow


Thanks for using the new pattern. Let me know if you had any issues with it.

k4b7 · 2017-10-14T22:59:03Z

src/functions/href.js

+
+        let cls = []; // null if the type of both ends differs.
+        let fst; // mathtype of the first child
+        let lst; // mathtype of the last child


No need to keep to short... we minify the code so it should make these even shorter. Maybe classes, first, and last.

Oh sorry. This naming convention is one of the bad habit of Haskellers... I will make them more readable.

k4b7 · 2017-10-14T22:59:28Z

src/functions/href.js

+        let cls = []; // null if the type of both ends differs.
+        let fst; // mathtype of the first child
+        let lst; // mathtype of the last child
+        // Invariants: both fst and lst must be non-null if cls is null.


Thanks for calling this out. It looks like getTypeOfDomTree can return null so it seems like it's possible that fst could be null and lst could be non-null and cls could end up being null which would break the invariant. Maybe we should have code that checks for that case.

I didn't notice that. In that case. I will add the code to handle that case and default to mord.

k4b7 · 2017-10-14T23:07:55Z

src/functions/href.js

+            const elts = [new buildCommon.makeSpan([fst], [], options),
+                anc,
+                new buildCommon.makeSpan([lst], [], options),
+            ];


Super minor nit, the following formatting might better reflect the new span sandwich we got going on here:

return new buildCommon.makeFragments([ new buildCommon.makeSpan([fst], [], options), anchor, new buildCommon.makeSpan([lst], [], options), ]);

This seems much more appropriate. I will reflect it.

k4b7 · 2017-10-14T23:13:31Z

src/functions/href.js

+    names: ["\\href"],
+    props: {
+        numArgs: 2,
+        argTypes: ["original", "original"],


Should we maybe be using "text" for the href so that if we're in math mode and we include a URL with spaces in it, the spaces don't get eaten.

Since #912, I don't think the space behavior will be different, but "text" does feel slightly more correct. I'm guessing, in LaTeX, the argument is treated like a verbatim. For example, are two spaces preserved as two spaces or merged into one? (In KaTeX, they will currently be merged.) Does \foo bar have a space (I guess so) or remove the space (as it will probably currently behave). This might require a new argument type of "url"...

It seems that the first argument of \href will be treated as if it's inside verbatim, but both \href{http://en.wikipedia.org/wiki/Equals\_sign}{=} and \href{http://en.wikipedia.org/wiki/Equals_sign}{=} generate the same link to Wikipedia.

In the current implementation, it rejects \href{http://en.wikipedia.org/wiki/Equals_sign}{=} and accepts \href{http://en.wikipedia.org/wiki/Equals\_sign}{=}, but it links to http://en.wikipedia.org/wiki/Equals/_sign instead of http://en.wikipedia.org/wiki/Equals_sign. So it might be appropriate to create new argument type url for proper parsing, as @edemaine said.

* Added `functions/href.js`. * Added `domTree.anchor` and `buildCommon.makeAnchor` functions. * Make `buildHTML.getTypeOfDomTree` exported.

* Create new argType "url" to treat link string in math appropriately * Stop using too shorten variable names * Start using template strings

edemaine · 2017-10-18T20:28:24Z

@konn Just checking: Is this ready for review again?

konn · 2017-10-19T03:44:02Z

@edemaine Yes. I think it’s ready.

k4b7 · 2017-10-19T13:20:21Z

I'll have another look this weekend.

k4b7

This could also use some unit tests to verify that \hrefs parse and don't parse in the expected way, see katex-spec.js. Also, it'd be nice to have a snapshot test for the MathML that we're outputting, see mathml-spec.js (running npm test -- -u will update the snapshot file).

k4b7 · 2017-11-11T22:29:42Z

src/Parser.js

+        const raw = res.text;
+        // hyperref package allows backslashes alone in href, but doesn't generate
+        // valid links in such cases; we interpret this as "undefiend" behaviour,
+        // and keep them as-is. In some environment, they're replaced by slashes


Are the backslashes being replaced by forward slashes? In which environments does this happen?

At least in Safari 11.0.1 (13604.3.5) on macOS High Sierra 10.13.1.

Ah... I was getting confused between LaTeX environments and browser environments. Maybe reword that last sentence to be: Some browser will replace backslashes with forward slashes.

Oh yes, it was really confusing expression. I will replace them.

k4b7 · 2017-11-11T22:31:57Z

src/functions/href.js

+         * 2. if it has more than two elements, and the classes
+         *    of its first and last elements coincide, then use it;
+         * 3. otherwise, we will inject an empty <span>s at both ends,
+         *    with the same classes of both ends of elements.


Sneaky. In this third case does the element in between the empty spans have no class on it?

Oh, I should elaborate on it. The third case we insert a kind of glue, with the first one having the same class as the first element of body and the second one the same as the last. I will add this description.

k4b7 · 2017-11-11T22:35:19Z

src/functions/href.js

+        }
+        if (!classes) {
+            const anc = buildCommon.makeAnchor(href, [], elements, options);
+            return new buildCommon.makeFragment([


This works, but I'd probably move this up to where you set classes = null so that we can remove an extra if statement.

Oh, yes. It makes program much simpler. Thanks.

k4b7 · 2017-11-11T22:36:12Z

src/functions/href.js

+                classes = null;
+            }
+        } else { // No elements at all, just ignore.
+            classes = [];


classes is already [] from let classes = [] so this else block isn't necessary.

Exactly. I will drop this clause.

k4b7 · 2017-11-11T22:38:38Z

src/domTree.js

+        // Add the class
+        if (this.classes.length) {
+            markup += " class=\"";
+            markup += utils.escape(createClass(this.classes));


This could be simplified to

markup += ` class="${utils.escape(createClass(this.classes))}"`;

Thanks. Template literal looks nicer!

k4b7 · 2017-11-11T22:39:26Z

src/domTree.js

+            if (attr !== "href" &&
+                Object.prototype.hasOwnProperty.call(this.attributes, attr)) {
+                markup += " " + attr + "=\"";
+                markup += utils.escape(this.attributes[attr]);


markup += ` ${attr}="${utils.escape(this.attributes[attr])}"`

I will adopt template literal also here.

k4b7 · 2017-11-11T22:43:11Z

@konn sorry about the delay. Please let me know if you have any questions/concerns about any of the feedback.

konn · 2017-11-12T02:59:38Z

@kevinbarabash No problem, thank you for your review!

This could also use some unit tests to verify that \hrefs parse and don't parse in the expected way, see katex-spec.js. Also, it'd be nice to have a snapshot test for the MathML that we're outputting, see mathml-spec.js (running npm test -- -u will update the snapshot file).

OK, I will try to write some unit-tests.

k4b7 · 2017-11-12T03:14:16Z

OK, I will try to write some unit-tests.

@konn let me know if you need help with this.

konn · 2017-11-12T05:00:09Z

OK, I've just added unit-tests and snapshot tests. I also found that the previous version doesn't treat nested braces correctly, so I fixed this issue.

k4b7 · 2017-11-12T05:18:53Z

test/katex-spec.js

+        const url = "http://example.org/~bar/#top?foo=$}foo{&bar=bar^r_boo%20baz";
+        const input = url.replace(/([#$%&~_^{}])/g, '\\$1');
+        const ae = getParsed(`\\href{${input}}{\\alpha}`)[0];
+        expect(ae.value.href).toBe(url);


k4b7 · 2017-11-12T05:19:26Z

test/mathml-spec.js

+        expect(getMathML("\\href{http://example.org}{\\alpha}")).toMatchSnapshot();
+        expect(getMathML("p \\Vdash \\beta \\href{http://example.org}{+ \\alpha} \\times \\gamma"))
+            .toMatchSnapshot();
+    });


I didn't realize that you could have two snapshots inside the same it block, cool!

k4b7

I want to have a second look at parseStringGroupWithBalancedBraces when I'm fresh before merging.

konn · 2017-11-22T12:12:25Z

@kevinbarabash Is there any time to look into this?

k4b7 · 2017-11-24T04:14:33Z

src/Parser.js

+        }
+        this.mode = outerMode;
+        this.expect(optional ? "]" : "}");
+        return firstToken.range(lastToken, str);


I understand why we're tracking lastToken now and why a recursive solution isn't as easy as I thought it might be.

k4b7 · 2017-11-24T04:16:21Z

@konn I've reviewed parseStringGroupWithBalancedBraces, looks good. I will merge this after the tests pass. Sorry for taking so long with this.

konn · 2017-11-24T06:10:48Z

Thanks!

k4b7 added the GH Review: review-needed label Oct 14, 2017

k4b7 reviewed Oct 14, 2017

View reviewed changes

k4b7 added GH Review: needs-revision and removed GH Review: review-needed labels Oct 14, 2017

konn added 2 commits October 17, 2017 22:46

Implements `\href' command.

93100f7

* Added `functions/href.js`. * Added `domTree.anchor` and `buildCommon.makeAnchor` functions. * Make `buildHTML.getTypeOfDomTree` exported.

Reflects the code reviews

9509047

* Create new argType "url" to treat link string in math appropriately * Stop using too shorten variable names * Start using template strings

konn force-pushed the href branch from 0664f53 to 9509047 Compare October 17, 2017 14:08

k4b7 self-assigned this Oct 19, 2017

Merge branch 'master' into href

4d37ccb

k4b7 requested changes Nov 11, 2017

View reviewed changes

konn added 3 commits November 12, 2017 11:36

Merge branch 'master' into href

2fd6fa7

Adopts template literal

286525b

Elaborates on glueing

782e98e

If-clause restructuring

cebe64a

konn added 4 commits November 12, 2017 12:15

Solved confusing explanation

b16eef4

Allow balanced braces in url

1cb622a

Adds unit-test for \href

f754959

Adds snapshot tests

093b324

k4b7 reviewed Nov 12, 2017

View reviewed changes

k4b7 approved these changes Nov 12, 2017

View reviewed changes

k4b7 added GH review: accepted GH Review: review-needed and removed GH Review: needs-revision GH review: accepted labels Nov 12, 2017

k4b7 reviewed Nov 12, 2017

View reviewed changes

cmc333333 mentioned this pull request Nov 15, 2017

Footnotes in math 18F/omb-eregs#625

Closed

konn added 2 commits November 20, 2017 16:38

Merge branch 'master' into href

c89d86b

Merge branch 'master' into href

4689769

Merge branch 'master' into href

da9824e

k4b7 reviewed Nov 24, 2017

View reviewed changes

Merge branch 'master' into href

dd336f2

k4b7 merged commit fd82c4f into KaTeX:master Nov 24, 2017

konn deleted the href branch November 24, 2017 06:10

Uh oh!

Conversation

konn commented Oct 9, 2017

Uh oh!

khanbot commented Oct 9, 2017

Uh oh!

edemaine commented Oct 9, 2017

Uh oh!

konn commented Oct 10, 2017

Uh oh!

edemaine commented Oct 10, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edemaine commented Oct 18, 2017

Uh oh!

konn commented Oct 19, 2017

Uh oh!

k4b7 commented Oct 19, 2017

Uh oh!

k4b7 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

konn commented Nov 12, 2017 •

edited

Loading