Add LaTeX math support (v3) #4005
Conversation
src/webview/css/css.js
Outdated
| The naïve solution of simply giving some part of the KaTeX fragment itself an | ||
| `overflow-x: auto` style breaks terribly: | ||
| * Margin collapsing no longer works, causing rendering artifacts. (This is | ||
| particularly visible on integral signs, which are truncated into near- | ||
| illegibility.) |
There was a problem hiding this comment.
(Continuing from #3679 (comment) .)
I appreciate the extra few words here, "which are truncated into near-illegibility".
I still don't understand what is meant by "margin collapsing no longer works", though, and I don't understand what goes wrong if you do this "naive" solution.
One reason I appreciate the extra words is in fact that they provide something concrete, so that I can tell if I'm seeing the same thing you're describing! I did the following experiment:
- Checked out this branch just before this commit -- so there's KaTeX, but not this scrolling fixup.
- Opened the app in the Android emulator, and went to a message list with a display-math integral in it.
- The integral looked fine.
- Opened the Chrome devtools on the webview, and edited the stylesheet so that the
.katex-displayelement would haveoverflow-x: auto. - Result: the integral still looked fine and normal. I didn't see anything truncated.
Variations:
- Another display formula, this one horizontally long so it overflowed the screen, also looked fine, and scrolling it worked as expected.
- I made one of the integral endpoints unreasonably tall, in case that was the situation you had particularly in mind. Still showed up fine:
The ℵ (aleph) is potentially hard to read because it's small, but that seems expected given where it is in the formula; and in particular nothing appears to be truncated.
There was a problem hiding this comment.
I still don't understand what is meant by "margin collapsing no longer works", though
Describing this as being caused by margin collapsing [1] no longer working [2], is, I vaguely recall, based on a comment in a KaTeX GitHub issue. (Which, naturally, I can no longer find. 😞)
- Checked out this branch just before this commit -- so there's KaTeX, but not this scrolling fixup.
- Opened the app in the Android emulator, and went to a message list with a display-math integral in it.
- The integral looked fine.
- Opened the Chrome devtools on the webview, and edited the stylesheet so that the
.katex-displayelement would haveoverflow-x: auto.
I followed this procedure, and got this:
Which is about what I remember seeing. (This would probably not be a bad image to provide a link to (a thousand words, etc.), if GitHub is a reliable-enough imagehost.)
You're probably not seeing it because you're on the latest WebView, where I couldn't reproduce it in this way. (I customarily test on API 21, for essentially this reason.) I think it also breaks on Safari, or at least that it did three months ago; but I'm not sure, and can't presently test it.
There was a problem hiding this comment.
Ah! Very interesting, thanks -- both the screenshot (a thousand words, etc.), and the repro detail about what platform you tried it on.
I'll go try this on some older versions. I think it's also possible to describe the situation in the comment, and then link to this screenshot as a helpful bonus -- something like: "Some formulas have rendering errors on older browsers (before Chrome NN and Safari MM); in particular, \int signs get clipped halfway through." (edit: and perhaps add something like: "A comment in the KaTeX tracker which I can't find right now suggested that the problem was in faulty handling of margin-collapsing.")
Another key aspect of this behavior is that it's apparently a browser bug. From the earlier descriptions, I couldn't tell if the issue (as you understood it) was that doing this triggered browser bugs, or that it just interacted badly, through the legitimately-specced arcanities of CSS and of margin-collapsing in particular, with what KaTeX was doing with its CSS.
When the behavior is faithful to the spec, I tend to feel we should understand what's going on better before doing a hack to work around it, because it's likely there's a cleaner way which the spec authors (or people giving them feedback) had in mind. But when it's a browser bug -- and when some broken-seeming behavior is fixed in newer browser versions, that's prima facie evidence it's a browser bug -- then a hack is much more likely to be the best solution available.
There was a problem hiding this comment.
API 21
Looking this up, I think you're referring to Android 5.0 Lollipop.
Here's what we have in the "supported platforms" comment in js.js:
/*
* Supported platforms:
*
* * We support iOS 10. So this code needs to work on Mobile Safari 10.
* Graceful degradation is acceptable below iOS 12 / Mobile Safari 12.
*
* * For Android, core functionality needs to work on Chrome 44.
* Graceful degradation is acceptable below Chrome 58.
*
* * These versions are found in stock images for Android 6 Marshmallow
* and Android 8 Oreo, respectively, for convenient testing.
*
* * (Note that Android's Chrome auto-updates independently of the OS, and
* the large majority of Android users have a fully-updated Chrome --
* more recent than the Safari on most iOS devices.)
*
* * Below Chrome 44, it's possible (but rare) for a user to be on a
* version as old as Chrome 37, which shipped with Android 5 Lollipop.
* We sometimes fix issues affecting those versions, only when trivial.
*
* * See docs/architecture/platform-versions.md for data and discussion
* about our version-support strategy.
*/
So the Chrome 37 that shipped with that release (and that appears in stock emulator images for it) is in the status of "we sometimes fix issues affecting those versions, only when trivial".
I think it'd be quite reasonable to test routinely on Chrome 44, which you can get in an Android 6.0 Marshmallow (aka API 23) stock emulator image. If you test on Chrome 37 and find something broken, it'd be wise to test on Chrome 44 before spending more than a few minutes seeking to fix it.
This particular misrendering, I wouldn't describe as "core functionality" failing to work, either -- it looks ugly, clearly something's broken, but you can still pretty much read everything. So by our policy, it's OK to have (and doesn't justify a complex hack to fix) if it appears only below Chrome 58 (found on an Android 8.0 Oreo stock image) and iOS 12.
There was a problem hiding this comment.
Another key aspect of this behavior is that it's apparently a browser bug. From the earlier descriptions, I couldn't tell if the issue (as you understood it) was that doing this triggered browser bugs, or that it just interacted badly, through the legitimately-specced arcanities of CSS and of margin-collapsing in particular, with what KaTeX was doing with its CSS.
Let me clarify then: at the time, I understood it to be the latter, and I am not certain even now that it's not. (The fact that Chrome's behavior has changed is certainly evidence in favor, but – CSS being, as you note, CSS – not proof.)
Even if it's not, though – as long as Safari v${latest - 1} (which, as I mentioned, I can't test right now) and stock Android 8.0 (which I can and will) are happy with overflow: auto, I expect I will be, too.
My initial tests have something else going wrong, though, so there may be a bit of a delay there. (This is on Android 10, on a pristine commit 0b3d3b9, which I'm pretty sure worked on Saturday. 😞)
So the Chrome 37 that shipped with that release [...] is in the status of "we sometimes fix issues affecting those versions, only when trivial".
Hmm. I'd thought our support for Lollipop was more firm than that.
That said, I typically do test interesting bugs against one or more of a small collection of Android images; I just didn't have them to hand, for obvious reasons. If this breaks on Chrome 37 but not Chrome 44, it'll be the first thing I can recall which does.
There was a problem hiding this comment.
Cool. Certainly worth studying more, then. I didn't get to testing this issue today as much as I'd hoped, but I did just now try this:
even on Oreo, the second issue in that comment – the fact that the equations also become vertically scrollable – is still a problem.
and I don't reproduce. Same procedure as above, this time with a \sum... \int... \frac... that looks like one of your examples in the screenshot above, and (on an Android 8 Oreo emulator image) it looks normal and scrolls horizontally but not vertically, as desired.
So, definitely interested in pinning that down.
... Hm! But navigator.userAgent in the dev console there tells me it's Chrome 72. Must have auto-updated :-/ -- normally a good thing, but not for this kind of testing. I'll have to revert that, or if nothing else then make a fresh image.
Any comments on the branch before this point? I think even without horizontal scrolling of display math, this is a major improvement over the status quo, both for display math and (what I think is more common) inline math. So I would be very pleased to merge those n-2 commits soon, and then have a followup task to get horizontal scrolling for wide display formulas.
There was a problem hiding this comment.
and I don't reproduce. Same procedure as above, this time with a
\sum... \int... \frac...that looks like one of your examples in the screenshot above, and (on an Android 8 Oreo emulator image) it looks normal and scrolls horizontally but not vertically, as desired.
The exact text was:
```math
\lim_{x \to \infty} \int_{0}^{1} \frac{1 + 2x + 3x^2 + 4x^3 + 5x^4 + 6x^5 \cdots}{e^x} \; dx
```
Here's a shot of it with the equation scrolled upwards slightly. Note the truncated exponents.
(The problem is not so much that the exponents are truncated; it's that the equations are all wobbly to the touch. It's not a good look.)
Any comments on the branch before this point?
A few things. I had to cancel my review to respond here, but I have them saved; I'll finish them up and post them shortly.
I think even without horizontal scrolling of display math, this is a major improvement over the status quo, both for display math and (what I think is more common) inline math. So I would be very pleased to merge those n-2 commits soon, and then have a followup task to get horizontal scrolling for wide display formulas.
Wait, what? Why not the last two? The existing horizontal-scrolling code is functional everywhere without issues, as far as I can tell. If investigation shows that something simpler is feasible, we can swap it out later without affecting users.
Besides which, I don't think leaving them out leaves the app in a good state. It's possible for an embedded equation to be cut off at a point where it's not obvious that it's not all there.
There was a problem hiding this comment.
Great -- thanks for the repro, and for the comments on the rest!
Besides which, I don't think leaving them out leaves the app in a good state. It's possible for an embedded equation to be cut off at a point where it's not obvious that it's not all there.
The webapp has the same behavior. Here's two messages with the same content 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 + 16 + \cdots, except one is in an inline math formula and the other in display math:
We've heard very clearly from users that the current state in the mobile app is that LaTeX doesn't work, that they're very happy they can use LaTeX in the webapp, and that they want it in the mobile app too. It'll be good to fix this edge case, but I am not going to block on it for shipping the feature.
There was a problem hiding this comment.
(Meant to reply directly to this part too, and forgot.)
Why not the last two? The existing horizontal-scrolling code is functional everywhere without issues, as far as I can tell.
Complexity. The app has had a number of complex hacks in it that were made without clearly understanding, or simply without clearly explaining, what problem they were solving and how and why. And it's suffered from that.
There's a place for complex hacks, and there may well be a place for this one. But we don't have a clear enough understanding at this point to be ready to make it.
There was a problem hiding this comment.
Filed zulip/zulip#14422 for the issue in the existing webapp.
tools/build-webview
Outdated
| --no-sanity-checks) | ||
| # disables target-directory sanity checks, for testing's sake | ||
| shift; sanity_checks=0;; | ||
| --script) |
There was a problem hiding this comment.
It's a good idea at its core; but --script is not a meaningful name, even with the documentation added in the following commit.
(Why not just --sanity-checks?)
There was a problem hiding this comment.
Yeah, I don't love the name. I wanted something non-specific about what the checks are, basically a name that summarizes the description in the new comment / in the subsequent usage message, but "script" isn't very clear in doing that.
I didn't go for --sanity-checks because I don't like the implication there's something wild or wrong about a different destination path. In particular when you ask some code to do something in conditions that it has "sanity checks" that would reject if you enabled them, that feels like the code is free to do wacky things that don't correspond to what it would do in proper conditions. Here, that would undermine the usefulness of being able to pass a convenient nonce directory and inspect the output there.
How about --check-path-name? Revised to that. (I like "name" a bit better than just --check-path or --check-dest, because either of the latter sound like they might check things like what's inside the dest directory.)
tools/build-webview
Outdated
| # Sync the directory structure, preserving metadata. (We ignore any files named | ||
| # 'README.md'; these should be strictly informative.) | ||
| # | ||
| # We use `rsync` here, as it will skip unchanged files. | ||
| mkdir -p "$dest" | ||
| rsync -a --no-D --delete --delete-excluded --exclude=README.md "$staging/." "$dest" | ||
| rsync -a --no-D --delete --delete-excluded \ |
There was a problem hiding this comment.
The staging directory was never intended as a speed optimization; the first two paragraphs in this commit are attacking a strawman.
(It was primarily intended for transparency and verifiability, as part of a check against stale files being left in the build directories. The details are probably no longer relevant enough to discuss here.)
There was a problem hiding this comment.
Interesting -- that reason is news to me 🙂 I guess the sharing was the best explanation I could reverse-engineer for it -- as the message said, there are circumstances not too different from these where something like it would be helpful.
Revised to:
This "staging" step was intended as a way to help the developer
hand-inspect the output for stale files. Better to prevent them
automatically, by continuing to use tools like `rsync --delete`.
Meanwhile, the extra indirection complicates the code and
complicates reasoning about it. So, simplify it away.
| @@ -6,6 +6,7 @@ import cssNight from './cssNight'; | |||
|
|
|||
| export default (theme: ThemeName) => ` | |||
| <link rel='stylesheet' type='text/css' href='./base.css'> | |||
| <link rel='stylesheet' type='text/css' href='./katex/katex.min.css'> | |||
There was a problem hiding this comment.
This is the only line of any part of this commit — summary, message, or diff — that I can reasonably be said to have written.
Please adjust its Author: field accordingly.
There was a problem hiding this comment.
OK -- changed.
Here's why I left your Author: line in place, though: this commit incorporates your work determining
- we need the CSS but not the JS, despite the install instructions just saying to use both
- we need the WOFF2 and not the other font formats, and the browser support details that make that the case
- the choice of where and how to lay out the KaTeX files
So my inclination if marking myself as author would be to have a Co-authored-by: line for you. I've left that out in this revision, because that feels most consistent with the spirit of your request here, and I want to respect your preferences on whether to claim this authorship. But I'd be glad to add such a line if you're OK with that.
There was a problem hiding this comment.
For the record, very belatedly: I do indeed feel that co-authorship would not have been appropriate here.
What I would probably write in a similar case would be something along the lines of "(Based in part on/Supersedes/Inspired by) earlier work by...", with no pseudoheader. (As a concrete example, see the commit message of 988736b.)
|
Pushed just now the revisions mentioned above, and without those last two commits. I'll go file an issue about the fact that overfull hboxes^W^W wide display-math formulas can leave part of the formula invisible. It's a zulip/zulip issue as it stands, and once this is merged it'll be a mobile issue too, which we might or might not solve in the same way. (edit: filed as zulip/zulip#14422.) |
With the "--destination" flag next to the unrelated "android" argument before it, but separated by a comment line from its own argument, the formatting of this bit of code obscured its structure. Fix that. Then to help avoid an over-long line, give the argument a name, and pull its definition out from the command line.
In a CLI, the defaults generally should favor interactive use, because adding a flag is a much bigger burden on quick command-line use than it is in a script. These checks don't make sense when using this script interactively, so that favors having them opt-in. Moreover, the content of these particular checks is really that they're checking up on whether the caller is doing the caller's own job right -- and doing so on the presumption that the caller is our actual build script for either iOS or Android. (Nothing this tool itself does depends on how the destination directory is named. It'd be pretty weird if it did, and pretty questionable.) The checks are inappropriate in any other context, so that also favors having them be opted into by the specific callers that need them.
…sage. Also describe more plainly what this script does. A long comment in src/webview/MessageList.js uses the term "webview-assets folder", and refers to this file to explain what it means. Deliver on that promise, too, by connecting to that term.
We might in the future have complex things we're doing here, which may need a complex structure which may have elements in common with the one suggested by these comments. But at present we're doing something quite simple -- just copying a few files. It isn't productive to try to decide on a complex future structure before we know what concrete needs we'll have for it. Until then, this empty scaffolding just gets in the way of reading what we're actually doing, and of extending it in the directions called for by our current known needs.
This "staging" step was intended as a way to help the developer hand-inspect the output for stale files. Better to prevent them automatically, by continuing to use tools like `rsync --delete`. Meanwhile, the extra indirection complicates the code and complicates reasoning about it. So, simplify it away. This change actually has no effect on the current code's behavior, because we're having a source directory double as the "staging" directory, and that's the *only* source directory involved in this build. The real effect comes when we add another source directory: we'll copy from that one straight into the destination, rather than first into this source directory and then again to the destination. This change drops a check that `$root/src/webview/static/` exists. That check is pretty redundant already -- the path is within our own tree -- but in any case `rsync` will give an error if the directory it's supposed to copy from is missing.
We're about to add another step here, for copying KaTeX-related files out of the KaTeX distribution. Factor out the routine bits into a shell function, so we can invoke the same function for that step too. This also makes a couple of small changes to the rsync command. These don't affect the existing behavior, but they help abstract the shared fiddly-rsync bits, which go inside the function, away from the specific sets of files to copy, which go at the call site. * Use -R aka --relative. This allows a root source directory to be specified once, and all other paths in the command to be relative to that root. * Use rsync filter rules. This allows "which files from the source should be copied" to be specified all in one place, in a well-defined format, and without taking arbitrary rsync arguments which could interact in surprising ways with those provided by the function.
This has the same effect if the directory is totally clean, containing no files other than those in the Git tree. But if if there are any extraneous files, this prevents accidentally picking them up and building them into the app. Suggested-by: Ray Kraesig <[email protected]>
This incorporates the same KaTeX library we use in the Zulip webapp. We add the dependency, add a build-webview step to copy the needed files, and apply the stylesheet in our WebView HTML. There are a couple of cases where the resulting HTML and CSS doesn't quite look right. We'll go on to take care of those in upcoming commits. Fixes: zulip#2660
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.
|
OK -- this is now merged! I've tested it back to iOS 10 and Android 6 Marshmallow (with Chrome 44), and it works beautifully, with that known issue that's the equivalent of zulip/zulip#14422. I'll go file a bug for that one. Thanks @ray-kraesig for the review, and thanks again for your work on this earlier. |
This is a successor to #3744 and #3679, and fixes #2660. It sets us up so that the message-list webview now uses KaTeX to show math formulas, just like the webapp does.
The commits that actually set up KaTeX, and then add some fixes to how it displays in certain cases, are @ray-kraesig's and taken from those previous PRs. The major difference from those PRs is that it doesn't include the complex build-script infrastructure that those PRs added; instead it simplifies our existing
tools/build-webviewscript somewhat and then extends it more modestly in order to accomplish what we need for KaTeX. The approach is based on myrsync-refactorbranch (to commit gnprice/zulip-mobile@6d98d01fb), from the #3679 thread.Smaller differences include:
I've tested the branch on Android and iOS, and it works in my testing.
I'd appreciate review from @ray-kraesig (and others) on this PR. Ray and I have discussed exhaustively the strategy question of whether to develop now an infrastructure for complex build scripts, and while he disagrees, I've made the decision that we won't. But there are plenty of more-specific choices here that I'd be glad to see comments on.
There's one commit toward the end that I still have questions about (left over from #3679), which I'll comment on below. Up to that point, and including the main commit that fixes #2660, I consider this branch ready to merge.