Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LaTeX support #3679

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
453a673
deps: add KaTeX
rk-for-zulip Nov 14, 2019
11c23b8
webview build: ignore .gitignore files
rk-for-zulip Nov 14, 2019
41b1e3d
webview build: add pre-sync build step executor
rk-for-zulip Nov 14, 2019
3649bc4
webview: bundle/package and link in KaTeX support files
rk-for-zulip Nov 14, 2019
6270550
webview: add katex block-equation scrolling support
rk-for-zulip Nov 14, 2019
cb9dc8a
webview/katex: add katex frac-line hack
rk-for-zulip Nov 14, 2019
3e00126
webview js: fold image URL rewriting into `processIncomingHtml`
rk-for-zulip Nov 14, 2019
d03307f
tools: remove now-unneeded "empty.txt"
rk-for-zulip Nov 14, 2019
a698d7b
webview build: simplify katex-css build script
rk-for-zulip Nov 14, 2019
6adc19e
katex: use minimized css
rk-for-zulip Nov 14, 2019
45783c2
webview build [nfc]: additional clarificatory comments
rk-for-zulip Nov 14, 2019
c620326
webview build: factor out rsync invocation
rk-for-zulip Nov 14, 2019
b2dc5e9
webview build [nfc]: add no-sanity-checking option
rk-for-zulip Nov 14, 2019
63a7bc9
webview build [nfc]: rename SRC to STAGING
rk-for-zulip Nov 14, 2019
dda99e9
webview build [nfc]: rename intrascript variables
rk-for-zulip Nov 14, 2019
a31cbcb
webview build [nfc]: make all variables readonly where possible
rk-for-zulip Nov 14, 2019
4f51f6a
webview: remove KaTeX js file
rk-for-zulip Nov 15, 2019
dd73b2d
webview build [nfc]: rearrange definitions / comments
rk-for-zulip Nov 18, 2019
b028728
webview build: remove dependency on $CWD being a specific value
rk-for-zulip Nov 18, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .eslintignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
**/flow-typed/**
**/__flow-tests__/**
**/__flow-tests__/**

# Third-party minified code. (Also, this is a staging directory, and is
# untracked by git.)
src/webview/static/katex
gnprice marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"blueimp-md5": "^2.10.0",
"color": "^3.0.0",
"date-fns": "^1.29.0",
"katex": "^0.10.2",
"lodash.escape": "^4.0.1",
"lodash.isequal": "^4.4.0",
"lodash.omit": "^4.5.0",
Expand Down
64 changes: 64 additions & 0 deletions src/webview/css/css.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,73 @@
/* @flow strict-local */
import { Platform } from 'react-native';
import type { ThemeName } from '../../types';
import cssPygments from './cssPygments';
import cssEmojis from './cssEmojis';
import cssNight from './cssNight';

/** CSS fragment to support scrolling of KaTeX formulae. */
/*
By default, KaTeX renders (non-inline) math into a div of fixed precomputed
width. This will be cut off by the edge of the screen when the formula is too
long -- and on a mobile device, that's very nearly always.

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.)
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow this. What does it look like?

Or: Huh, how would you give it to some part inside the fragment?

... Oh, I see, the answer to the latter question is in fixup-katex.js.

I would not call that a "naïve solution". 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's understandable; I seem to have left out an entire paragraph. 😞 The bit in fixup-katex.js is not the naïve solution; it's the one that actually works.

(One would give scrollability to some part inside the fragment by adding a CSS rule .katex-display > .katex { overflow-x: auto; } or somesuch.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(One would give scrollability to some part inside the fragment by adding a CSS rule .katex-display > .katex { overflow-x: auto; } or somesuch.)

Ah, I see.

I still don't quite follow the description of the problem/symptom. What does it look like when margin collapsing no longer works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few words here, but there's really no way I can get this across without including an image – which I doubt would be helpful to future readers or debuggers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* `overflow-y: hidden` isn't respected. If KaTeX has used negative-position
struts in its rendering (which it does frequently), there will always be
vertical scrollability. (This may be a Chrome bug.)

Instead, we modify the provided DOM to wrap each `.katex-display` div with two
additional elements: the outer element is scrollable and `display: block`,
while the inner element is fixed and `display: inline-block`. This suffices to
insulate the KaTeX elements from the deleterious effects of scrollability.

The inner of these elements also receives a border, to act as a UI hint
indicating that scrolling is necessary: the right border will be cut off when
the rendered element is too large. (The KaTeX itself will also be truncated,
of course, but this may not be apparent if the cutoff falls between two
symbols.)

We also cut the KaTeX-provided margin somewhat. (Since the KaTeX fragment is
isolated in the new divs, margin-collapsing can no longer occur.)
*/
const katexScrollStyle = `<style id="katex-mobile-scroll">
.zulip-katex-outer {
display: block;
overflow-x: auto;
text-align: center;
}
.zulip-katex-inner {
display: inline-block;
border: 1px solid hsla(187, 35%, 51%, .5);
border-radius: 3px;
}
/* adjust/override KaTeX-provided CSS */
.zulip-katex-inner .katex-display { margin: 0.5em 0.25em; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use px values, following our style elsewhere; preferably multiples of 4px.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this one, I'd prefer to follow KaTeX's style, because this overrides a KaTeX CSS definition.

(That's worth a note, though.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Started that writeup as I entered this comment (just before the holiday), and went back and finished it just now: 78d57a2 or https://github.com/zulip/zulip-mobile/blob/master/docs/background/webview.md . PTAL!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Any followup discussion is probably best in a chat thread, rather than attempting to squeeze it in here.)

</style>`;

/**
* Fix KaTeX frac-line elements disappearing.
*
* This is a hack, but it's probably better than not having fraction lines on
* low-resolution phones. It's only known to be useful under Chrome and Android,
* so we only include it there.
*
* See, among others:
* https://github.com/KaTeX/KaTeX/issues/824
* https://github.com/KaTeX/KaTeX/issues/916
* https://github.com/KaTeX/KaTeX/pull/1249
* https://github.com/KaTeX/KaTeX/issues/1775
*/
const katexFraclineHackStyle = `<style id="katex-frac-line-hack">
.katex .mfrac .frac-line { border-bottom-width: 1px !important; }
</style>`;

export default (theme: ThemeName) => `
<link rel='stylesheet' type='text/css' href='./base.css'>
<link rel='stylesheet' type='text/css' href='./katex/katex.min.css'>
<style>
${theme === 'night' ? cssNight : ''}
${cssPygments(theme === 'night')}
Expand All @@ -16,5 +78,7 @@ ${cssEmojis}
display: none;
}
</style>
${katexScrollStyle}
${Platform.OS === 'android' ? katexFraclineHackStyle : '<!-- Safari -->'}
<style id="generated-styles"></style>
`;
35 changes: 35 additions & 0 deletions src/webview/js/fixup-katex.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/* @flow strict */

// Auxiliary function, for brevity.
const makeDiv = (className: string): Element => {
const element = document.createElement('div');
element.classList.add(className);
return element;
};

/**
* Adjust non-inline KaTeX equation element structure.
*
* This surrounds existing `.katex-display` elements (the KaTeX wrapper for
* non-inline equations) with shim `<div>`s to allow horizontal scrolling.
*
* (The actual functionality of these elements is provided by CSS rules.)
*/
const fixupKatex = (root: Element) => {
Array.from(root.querySelectorAll('.katex-display')).forEach(kd => {
// nodes returned by `querySelectorAll` will always have a valid parent node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const parent: Node = ((kd.parentNode: ?Node): $FlowFixMe);

// Nest each `.katex-display` element within two new <div> elements.
// Notionally:
// s/ (.katex-display) / div.z-k-outer > div.z-k-inner > $1 /
const outer = makeDiv('zulip-katex-outer');
const inner = makeDiv('zulip-katex-inner');

outer.appendChild(inner);
parent.replaceChild(outer, kd);
inner.appendChild(kd);
});
};

export default fixupKatex;
33 changes: 28 additions & 5 deletions src/webview/js/generatedEs3.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ var compiledWebviewJs = (function (exports) {

var inlineApiRoutes = ['/user_uploads/', '/thumbnail?', '/avatar/'];

var rewriteImageUrls = function rewriteImageUrls(auth) {
var element = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : document;
var rewriteImageUrls = function rewriteImageUrls(auth, element) {
var realm = new URL(auth.realm);
var imageTags = [].concat(element instanceof HTMLImageElement ? [element] : [], Array.from(element.getElementsByTagName('img')));
imageTags.forEach(function (img) {
Expand Down Expand Up @@ -71,6 +70,23 @@ var compiledWebviewJs = (function (exports) {
});
};

var makeDiv = function makeDiv(className) {
var element = document.createElement('div');
element.classList.add(className);
return element;
};

var fixupKatex = function fixupKatex(root) {
Array.from(root.querySelectorAll('.katex-display')).forEach(function (kd) {
var parent = kd.parentNode;
var outer = makeDiv('zulip-katex-outer');
var inner = makeDiv('zulip-katex-inner');
outer.appendChild(inner);
parent.replaceChild(outer, kd);
inner.appendChild(kd);
});
};

if (!Array.from) {
Array.from = function from(arr) {
return Array.prototype.slice.call(arr);
Expand Down Expand Up @@ -404,7 +420,15 @@ var compiledWebviewJs = (function (exports) {
window.scrollBy(0, newBoundRect.top - prevBoundTop);
};

var processIncomingHtml = function processIncomingHtml(auth, root) {
rewriteImageUrls(auth, root);
fixupKatex(root);
};

var handleUpdateEventContent = function handleUpdateEventContent(uevent) {
var contentNode = document.createElement('div');
contentNode.innerHTML = uevent.content;
processIncomingHtml(uevent.auth, contentNode);
var target;

if (uevent.updateStrategy === 'replace') {
Expand All @@ -424,8 +448,7 @@ var compiledWebviewJs = (function (exports) {
target = findPreserveTarget();
}

documentBody.innerHTML = uevent.content;
rewriteImageUrls(uevent.auth);
documentBody.innerHTML = contentNode.innerHTML;

if (target.type === 'bottom') {
scrollToBottom();
Expand All @@ -445,8 +468,8 @@ var compiledWebviewJs = (function (exports) {
document.addEventListener('message', handleMessageEvent);
}

processIncomingHtml(auth, documentBody);
scrollToAnchor(anchor);
rewriteImageUrls(auth);
sendScrollMessageIfListShort();
scrollEventsDisabled = false;
};
Expand Down
26 changes: 22 additions & 4 deletions src/webview/js/js.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
import type { MessageListEvent } from '../webViewEventHandlers';

import rewriteImageUrls from './rewriteImageUrls';
import fixupKatex from './fixup-katex';

/*
* Supported platforms:
Expand Down Expand Up @@ -476,7 +477,22 @@ const scrollToPreserve = (msgId: number, prevBoundTop: number) => {
window.scrollBy(0, newBoundRect.top - prevBoundTop);
};

/**
* Fix up supplied HTML as needed for display.
*
* The root itself must not need fixups.
*/
const processIncomingHtml = (auth: Auth, root: Element) => {
rewriteImageUrls(auth, root);
fixupKatex(root);
};

const handleUpdateEventContent = (uevent: WebViewUpdateEventContent) => {
// Perform preprocessing on the webview content.
const contentNode: HTMLDivElement = document.createElement('div');
contentNode.innerHTML = uevent.content;
processIncomingHtml(uevent.auth, contentNode);

let target: ScrollTarget;
if (uevent.updateStrategy === 'replace') {
target = { type: 'none' };
Expand All @@ -492,9 +508,11 @@ const handleUpdateEventContent = (uevent: WebViewUpdateEventContent) => {
target = findPreserveTarget();
}

documentBody.innerHTML = uevent.content;

rewriteImageUrls(uevent.auth);
// TODO: eliminate the needless reserialization and deserialization
// step here, via something along the lines of
// documentBody.replaceChild(oldContentNode, contentNode)
// (which currently breaks our touch event handling).
documentBody.innerHTML = contentNode.innerHTML;

if (target.type === 'bottom') {
scrollToBottom();
Expand All @@ -519,8 +537,8 @@ export const handleInitialLoad = (platformOS: string, anchor: number, auth: Auth
document.addEventListener('message', handleMessageEvent);
}

processIncomingHtml(auth, documentBody);
scrollToAnchor(anchor);
rewriteImageUrls(auth);
sendScrollMessageIfListShort();
scrollEventsDisabled = false;
};
Expand Down
5 changes: 1 addition & 4 deletions src/webview/js/rewriteImageUrls.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@ const inlineApiRoutes: string[] = ['/user_uploads/', '/thumbnail?', '/avatar/'];
* than the document location.
* 2. If the source URL names an endpoint known to require authentication,
* inject an API key into its query parameters.
*
* DEPRECATED: If no parent element is specified, transform every <img> in the
* entire document.
*/
const rewriteImageUrls = (auth: Auth, element: Element | Document = document) => {
const rewriteImageUrls = (auth: Auth, element: Element) => {
// The realm, parsed.
const realm: URL = new URL(auth.realm);

Expand Down
3 changes: 3 additions & 0 deletions src/webview/static/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# KaTeX directory will be [re]built by tools/built-webview
/katex/*

Comment on lines +1 to +3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading