Add LaTeX support #3679

rk-for-zulip · 2019-11-06T02:11:54Z

Adds LaTeX support via KaTeX.

Fixes #2660.

rk-for-zulip · 2019-11-12T00:23:19Z

(Rebased to fix conflicts.)

gnprice

Thanks @ray-kraesig ! Glad to see this. 😄

Comments below, on various aspects.

Strategy-wise:

I would really like to get the essential parts of this in soon -- to deliver the feature for our users, and to reduce the complexity of stuff outstanding.
Some of the later commits here (e.g. doctype) look like they're solving problems we can live with, and the solutions in turn cause complications. So I'd like to move those out of this branch, into followup issues.
A number of the comments are on the bits of shell script.
- Some are simple to do, or potentially affect correctness, and it'd be good to just do them.
- For the more overall style aspects... after looking closer to try to understand how the rsync commands work, and work together, I found a way of rewriting them that I think makes this simpler. So rather than ask you to rework this in a different style, I think the most effective strategy might be:
  - I show you my sketch of that rewrite, and we agree on broad direction of how the style might go.
  - We merge this as is. (Apart from the more-specific fixes as mentioned above.)
  - I follow up by (finishing and) sending that rewrite of the rsync logic, which will make a lot of the style comments moot.

package.json

gnprice · 2019-11-11T19:15:05Z

src/webview/static/.gitignore

+# KaTeX directory will be [re]built by tools/built-webview
+/katex/*
+


I think it's worked well to keep these all in the one root .gitignore file. Separate files can be helpful for organization, but when the separate file is so tiny like this there isn't much of that benefit.

Here, this would very naturally go in the stanza

# Intermediate directories for webview assets /ios/webview # /android/app/build/... is already covered above

It's tiny today, but I expect it to grow soon.

I also don't think the root .gitignore file is working very well, and that third line you posted is evidence of it: android/app/build is only covered by chance, due to a line in the section labeled # Xcode! If not for the comment in there about upgrading React Native, I'd already be inclined to break out the ios/- and android/-specific elements into subfiles.

It's tiny today, but I expect it to grow soon.

In that case we can easily split it out when it does 🙂 It's super easy to move these lines around, and meanwhile we can avoid paying the cost of having things fragmented in tiny files.

I don't think the other bit is something that would be improved by splitting the file into tiny pieces. I started writing a longer reply, but this thread isn't the right format for that. I think it'd be reasonable to assimilate the whole file to a style that's explicit about rooted vs. unrooted patterns, and to our own choice of organizing it; a good place to discuss that would be a chat thread.

No need to block this on having that discussion, though. Removed, assimilated, and amended.

No need to block this on having that discussion, though. Removed, assimilated, and amended.

Sounds good! But I think this intended change may have gone missing.

tools/build-webview

gnprice · 2019-11-11T19:19:28Z

tools/build-webview

+# Chdir to the current git repo's root.
 ROOT="$(git rev-parse --show-toplevel)"
 cd "$ROOT"


This comment doesn't add anything -- the code is just as short and says the same thing.

It does if you're fluent in git options. Not everyone is, though.

(We just talked in person, and I understand your thinking better now! I think the below, which I'd written before that, is still meaningful as a direct reply to the point about the option. But another thing you like having this kind of comment for is as an outline or narrative of the code, for skimming and navigating it. The discussion about that was informative, and I won't try to summarize it here.)

We have this command in four other scripts, all in a similar context and for the same reason. Any nontrivial shell script uses a long list of features that most programmers don't know, and which are far harder to look up than this. It's impossible to comment everything, and at some point we have to trust the reader to take responsibility for understanding what they need to understand.

tools/build-webview

gnprice · 2019-11-11T22:39:21Z

src/webview/js/fixup-katex.js

+ */
+const fixupKatex = (root: Element) => {
+  Array.from(root.querySelectorAll('.katex-display')).forEach(kd => {
+    // nodes returned by `querySelectorAll` will always have a valid parent node


Huh, is this true in general? Or do you mean for this particular query?

This is true in general! root.querySelectorAll(...) never returns root as one of its values.

(It looks like it also doesn't return nodes from shadow DOM subtrees, although I admit I hadn't considered that at the time.)

I see. Because it only returns descendants, and "descendant" is defined to exclude the ancestor itself.

As always, when I ask a question in code review because I'm confused, I would like the confusion cleared up in the branch itself. :-)

As always, when I ask a question in code review because I'm confused, I would like the confusion cleared up in the branch itself. :-)

bump

src/webview/js/js.js

src/webview/html/html.js

.eslintignore

src/webview/html/html.js

rk-for-zulip

I've done some additional work on the auxiliary-script-file interface. I am more concerned now that flattening the script will be problematic than I was previously; this should at least be a better intermediate state, if we end up stuck in it.

Most, but not all, of the changes are in successor commits.

src/webview/html/html.js

rk-for-zulip · 2019-11-12T02:15:49Z

tools/build-webview

+# Chdir to the current git repo's root.
 ROOT="$(git rev-parse --show-toplevel)"
 cd "$ROOT"


It does if you're fluent in git options. Not everyone is, though.

tools/build-webview

rk-for-zulip · 2019-11-14T02:05:29Z

src/webview/css/css.js

+  is cut off between two symbols.)
+*/
+const katexScrollCss = `<style id="katex-mobile-scroll">
+.zulip-katex-outer { display: block; overflow-x: auto; }


src/webview/css/css.js

src/webview/js/js.js

rk-for-zulip · 2019-11-14T21:01:33Z

tools/build-webview.d/katex.all.pre

+# See https://unix.stackexchange.com/a/2503 for an explanation of the rsync
+# include/exclude option list.
+${RSYNC} --delete --delete-excluded \
+    --include="*.${KATEX_FONT_TYPE}" --include='*/' --exclude='*' \
+    "${SRC}/fonts" "${DEST}/."


Because the source file specified here is ${SRC}/fonts. It'll delete anything in there that's not specified.

This is a bug, though: files in ./katex proper won't be deleted if they're wrong. (For example, the extraneous katex.js after switching to katex.min.js.)

Altered by unifying the two rsync invocations in this file, partly by stealing your heredoc-filter-rules setup. :)

src/webview/css/css.js

rk-for-zulip · 2019-11-15T21:22:36Z

Addendum #n:

So it turns out, despite the implications of the KaTeX installation guide, you don't need katex.js to display already-rendered equations; it's only needed if you want to make new ones.

However.

If I remove the <script ... katex.min.js"> tag from the head of the generated HTML, about one out of every two times, entering a stream will not cause it to scroll it to the bottom.

😞

rk-for-zulip · 2019-11-16T02:29:00Z

If I remove the <script ... katex.min.js"> tag from the head of the generated HTML, about one out of every two times, entering a stream will not cause it to scroll it to the bottom.

Addendum to the addendum: this race condition is essentially existing behavior on master. It appears that parsing the large block of code that is katex.js was causing one thread to virtually always lose the race.

Some tentative initial work involving requestAnimationFrame shows promise for a fix. At any rate, it shouldn't be relevant to this PR.

tools/build-webview

Add KaTeX (the version currently used by the server) as a dependency.

In addition to README.md files, also don't copy .gitignore files into the staging directory.

Pre-sync (and, eventually, post-sync) scripts will be located in a subdirectory of tools/, named `build-webview.d` (following the modern Linux convention of placing configuration file-fragments or auxiliary scripts in similarly named directories). (This commit doesn't add any, though.)

Add a pre-build script to copy the KaTeX fonts and other support files into the staging directory for later bundling/packaging. (Also tell eslint to ignore that directory, since it'll contain minified source.) Add links to those files in the WebView HTML head. (At this point, most inline equations and many block equations are visible with their intended formatting.)

Wrap each block KaTeX equation in a small shim allowing it to be scrolled horizontally. Since the shim doesn't come with a visible scrollbar, add a visible border in lieu thereof.

KaTeX `.frac-line`s don't show up on some of our Android test devices and emulators. (This seems to be a perennial problem for KaTeX. See the commit-internal comments for references.) Resolve this by forcing all such lines to be 1px wide, disregarding the TeXbook spec.

... and eliminate the legacy meaning of an unspecified second parameter. This potentially involves some minor functional changes to `handleInitialLoad`, in that the order of actions therewithin is changed -- but if those actions don't commute, then we should probably have been rewriting the source HTML before scrolling anyway.

... which was only present to ensure that its directory existed in the intermediate commit.

... fixing a minor bug: stray files in the `katex` directory proper would not be deleted.

This only saves about 3K, but every little bit helps.

Refactor `rsync` invocations from `tools/build-webview` and its lone auxiliary script into a function. This arguably contains a functional change: the auxiliary script will now also exclude README.md and .gitignore files. (None presently exist.)

Add option to disable sanity checks for webview directory. These checks are mostly present to raise targeted alarms if something goes wrong with the build system after a change or upgrade; there's no need to have them in place if we're invoking it manually for testing purposes.

... which is a more generally accurate name, since our auxiliary scripts will be compiling or copying assets into $STAGING.

Rename script-global variables from ALL_CAPS to snake_case. (Environment variables, including those used for argument passing, remain ALL_CAPS.)

Although the installation directions just say to include katex.[min.]js unconditionally, the KaTeX JavaScript isn't actually needed just to display existing KaTeX blocks -- only to render new ones. Which is good, because that's ~242K of (uncompressed) JS code that we can do without.

The constants currently declared after argument parsing have no dependency on the arguments. Separate constant declarations into their own section. Reword or rewrite some of their comments, for clarity.

Make all values explicitly absolute paths. Avoid assuming that $CWD is any particular value in this script (at least until the auxiliary scripts are run). This also fixes a mostly-latent bug, in which `$dest` (if supplied as a relative path by a human) would be absolutized based on the project root rather than on $CWD.

tools/build-webview

gnprice · 2019-11-19T00:40:37Z

src/webview/css/css.js

+  The added border functions as a UI hint to indicate that scrolling is
+  possible and necessary. (This may not otherwise be apparent, if the equation
+  is cut off between two symbols.)


after Safari 13.1 or so, the pseudoclasses in question don't work anymore. ¯_(ツ)_/¯

Well, that's pretty dispositive! We won't be able to use them, then.

I think the comment in the code is clear now, thanks. The key bit was that it's not the presence of the border that's supposed to mean "this is scrollable". Rather, the visibly-cut-off border signals "this is cut off and incomplete", and then maybe you try scrolling.

gnprice

I spent some time this afternoon loading much of this PR back into my head, and then we discussed in person how to move it forward. The existing state of the PR has grown past GitHub's narrow bounds of what the review UI here can really manage.

In order to get the review complexity down to something manageable, I think the plan (from what you said) is that you'll make a new PR for these changes. That will start the review threads afresh, and you'll also make some revisions to make the branch easier to review and merge. Particularly helpful will be rebasing fixes in later commits forward to squash them into earlier ones, where you can do so.

There were a handful of items where you replied saying you'd done them, but it looks like the changes didn't make it into the branch you pushed. So those would be quite helpful to take care of. I've pointed at two in the comments below, and there's one other I couldn't get into this format but it's the thread here (about a bit of formatting in CSS code):
#3679 (comment)
(It's also possible I've misunderstood one or more of these replies; in which case please clear up my confusion 🙂 )

Also a couple of other small comments below.

gnprice · 2019-11-27T01:29:38Z

src/webview/css/css.js

+    * Margin collapsing no longer works, causing rendering artifacts. (This is
+      particularly visible on integral signs.)


Bump -- I think the added few words may have gone missing :-) because this appears to be the same as of b028728 as it is in the code at the start of this subthread.

gnprice · 2019-11-27T01:30:27Z

src/webview/static/.gitignore

+# KaTeX directory will be [re]built by tools/built-webview
+/katex/*
+


No need to block this on having that discussion, though. Removed, assimilated, and amended.

Sounds good! But I think this intended change may have gone missing.

gnprice · 2019-11-27T01:31:08Z

src/webview/js/fixup-katex.js

+ */
+const fixupKatex = (root: Element) => {
+  Array.from(root.querySelectorAll('.katex-display')).forEach(kd => {
+    // nodes returned by `querySelectorAll` will always have a valid parent node


As always, when I ask a question in code review because I'm confused, I would like the confusion cleared up in the branch itself. :-)

bump

gnprice · 2019-11-27T02:23:48Z

src/webview/css/css.js

+  border: 1px solid hsla(187, 35%, 51%, .5);
+  border-radius: 0.25em;
+}
+.zulip-katex-inner .katex-display { margin: 0.5em 0.25em; }


That's informative, thanks.

Thinking more about this, I really do want it to follow our style. (Policy? I said "style" originally but as I'm rereading what we have on this I realize that's inadequate -- it makes a visible difference, and overuse of em is an accessibility issue.) There are good reasons for that style^W policy, and we can't (within reasonable effort) control how third-party libraries work but we can keep things 100% consistent within our own code.

And in this case, to the extent there's a clash between our usage and the one in KaTeX's CSS, I don't think the result is materially worse if this rule is on our side of the clash rather than KaTeX's; I expect it works out better, for the same reasons that our usage is preferable in general.

I'm also finding as I look at this that the writeups we have of the reasoning here are a bit scattered and not entirely satisfying. So I'll go and pull those together into a better doc, which hopefully will be helpful.

rk-for-zulip · 2019-11-30T01:43:48Z

In order to get the review complexity down to something manageable, I think the plan (from what you said) is that you'll make a new PR for these changes.

Yes indeed. As it turns out, I've actually squashed relatively few of them: a fair number were uncontroversial [nfc] changes (plus one bugfix). I'll be submitting those as a separate leading PR.

There were a handful of items where you replied saying you'd done them, but it looks like the changes didn't make it into the branch you pushed. So those would be quite helpful to take care of.

That's only because the branch I pushed predates those items – both your comments and my replies – by several hours. 🙂

(I'd mentioned offline at the end of that day that I had marked a few things as done, but was still working on others and so hadn't pushed. In retrospect, that should have been written down here somewhere.)

rk-for-zulip requested a review from gnprice November 6, 2019 02:11

rk-for-zulip force-pushed the katex-css branch 2 times, most recently from 00bb80b to 45e5f8e Compare November 12, 2019 00:22

gnprice reviewed Nov 12, 2019

View reviewed changes

gnprice mentioned this pull request Nov 12, 2019

webview: Get HTML out of "quirks mode" #3098

Open

3 tasks

rk-for-zulip commented Nov 15, 2019

View reviewed changes

rk-for-zulip requested a review from gnprice November 15, 2019 02:45

rk-for-zulip force-pushed the katex-css branch from 45e5f8e to 6487e68 Compare November 15, 2019 18:28

gnprice reviewed Nov 16, 2019

View reviewed changes

tools/build-webview Outdated Show resolved Hide resolved

rk-for-zulip added 18 commits November 18, 2019 11:03

deps: add KaTeX

453a673

Add KaTeX (the version currently used by the server) as a dependency.

webview build: ignore .gitignore files

11c23b8

In addition to README.md files, also don't copy .gitignore files into the staging directory.

webview: add katex block-equation scrolling support

6270550

Wrap each block KaTeX equation in a small shim allowing it to be scrolled horizontally. Since the shim doesn't come with a visible scrollbar, add a visible border in lieu thereof.

tools: remove now-unneeded "empty.txt"

d03307f

... which was only present to ensure that its directory existed in the intermediate commit.

webview build: simplify katex-css build script

a698d7b

... fixing a minor bug: stray files in the `katex` directory proper would not be deleted.

katex: use minimized css

6adc19e

This only saves about 3K, but every little bit helps.

webview build [nfc]: additional clarificatory comments

45783c2

webview build [nfc]: rename SRC to STAGING

63a7bc9

... which is a more generally accurate name, since our auxiliary scripts will be compiling or copying assets into $STAGING.

webview build [nfc]: rename intrascript variables

dda99e9

Rename script-global variables from ALL_CAPS to snake_case. (Environment variables, including those used for argument passing, remain ALL_CAPS.)

webview build [nfc]: make all variables readonly where possible

a31cbcb

webview build [nfc]: rearrange definitions / comments

dd73b2d

The constants currently declared after argument parsing have no dependency on the arguments. Separate constant declarations into their own section. Reword or rewrite some of their comments, for clarity.

rk-for-zulip force-pushed the katex-css branch from 522ece8 to b028728 Compare November 18, 2019 21:32

gnprice reviewed Nov 19, 2019

View reviewed changes

gnprice reviewed Nov 27, 2019

View reviewed changes

rk-for-zulip closed this Nov 30, 2019

rk-for-zulip mentioned this pull request Dec 31, 2019

Add LaTeX support (v2) #3744

Closed

gnprice mentioned this pull request Mar 27, 2020

Add LaTeX math support (v3) #4005

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LaTeX support #3679

Add LaTeX support #3679

rk-for-zulip commented Nov 6, 2019

rk-for-zulip commented Nov 12, 2019

gnprice left a comment

gnprice Nov 11, 2019

rk-for-zulip Nov 12, 2019

gnprice Nov 16, 2019

rk-for-zulip Nov 19, 2019

gnprice Nov 27, 2019

gnprice Nov 11, 2019

rk-for-zulip Nov 12, 2019

gnprice Nov 16, 2019

gnprice Nov 11, 2019

rk-for-zulip Nov 12, 2019

gnprice Nov 19, 2019

gnprice Nov 27, 2019

rk-for-zulip left a comment

rk-for-zulip Nov 12, 2019

rk-for-zulip Nov 14, 2019

rk-for-zulip Nov 14, 2019

rk-for-zulip commented Nov 15, 2019

rk-for-zulip commented Nov 16, 2019

gnprice Nov 19, 2019

gnprice left a comment •

edited

Loading

gnprice Nov 27, 2019

gnprice Nov 27, 2019

gnprice Nov 27, 2019

gnprice Nov 27, 2019

rk-for-zulip commented Nov 30, 2019

		# KaTeX directory will be [re]built by tools/built-webview
		/katex/*

		* Margin collapsing no longer works, causing rendering artifacts. (This is
		particularly visible on integral signs.)

Add LaTeX support #3679

Add LaTeX support #3679

Conversation

rk-for-zulip commented Nov 6, 2019

rk-for-zulip commented Nov 12, 2019

gnprice left a 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

rk-for-zulip left a 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

rk-for-zulip commented Nov 15, 2019

rk-for-zulip commented Nov 16, 2019

Choose a reason for hiding this comment

gnprice left a comment • edited Loading

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

rk-for-zulip commented Nov 30, 2019

gnprice left a comment •

edited

Loading