-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Line breaks for inline formulas #1287
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1287 +/- ##
==========================================
+ Coverage 83.8% 83.87% +0.06%
==========================================
Files 61 61
Lines 3990 4018 +28
Branches 666 671 +5
==========================================
+ Hits 3344 3370 +26
- Misses 547 549 +2
Partials 99 99
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super exciting.
@@ -32,13 +36,6 @@ | |||
// mode, while still allowing background/foreground setting on root .katex | |||
* { -ms-high-contrast-adjust: none !important; } | |||
|
|||
.katex-html { | |||
// Making .katex inline-block allows line breaks before and after, | |||
// which is undesireable ("to $x$,"). Instead, adjust the .katex-html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to verify that in to $x$,
it's impossible to have a break between the x
and comma? In a narrow container it should break before x
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, in fact this is #1233. I've left this comment as is, though it clearly is incorrect, and I can edit it. This PR has no effect on this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. I swear this used to work.
Very, very cool stuff. I do have some questions. Will In LaTeX, there are some nuances about where line breaks can occur. Some examples:
Do you have any plan in mind for incorporating these nuances? Is it a matter of going through the existing code and setting an |
We'll have to develop some more sophisticated code for spaces. Directly after a line break, an |
@ronkok Thanks for the additional rules! I'm not convinced we can get spaces to get eaten by line breaks, using just CSS... I'll have to think about that. But the other rules you mention should be easy; we'd just need to give hskip's a CSS class of allowbreak. (Everything else is already forbidden.)
|
@ronkok Actually, I don't think those rules apply here. We're dealing with line breaks within math mode. I just did some tests, and none of $this\ is\ some\ long\ text\quad
this\ is\ some\ long\ text\quad
this\ is\ some\ long\ text\quad
...
$ However, if I add |
@edemaine You're right. I also did some testing and I can confirm that line breaks are not prevented in math mode by
Also, boxes, e.g. |
@ronkok I hope you mean "line breaks are forbidden at whitespace". The example I gave above does not get line breaks in LaTeX (or in this PR). Line breaks only seem to be allowed at This PR doesn't allow line breaks in any nested objects, which includes boxes. It only looks for breakable things at the top level. Same as TeX. |
That's odd. I get no line breaks when I try:
It seems that the I'm glad to hear about the boxes. |
Oh, I see our/my confusion. I meant that spaces/glue by themselves do not permit line breaks. You're saying that a nonbreaking space immediately after a valid line break place prevents that line break. Got it! That should be easy to fix. |
@edemaine In your most recent commit, you ensured that glue will stay with the operator. Could you make an exception for |
@ronkok I read the source code for Demo: I also fixed a bug that allowed line breaks where I didn't want them (needed to set |
Cool! This is really coming together well. |
I've successfully implemented |
I tracked down the bulk of the screenshot differences to this CSS rule: .vlist-s {
// This cell solves Safari rendering problems. It has text content, so
// its baseline is used for the table. A very small font avoids line-box
// issues; absolute units prevent user font-size overrides from breaking
// rendering. Safari refuses to make the box zero-width, so we give it
// a known width and compensate with negative right margin on the
// inline-table.
width: 2px; The placement of Now all screenshot diffs (against master) appear entirely black, meaning the differences are all subpixel, as expected, except for |
What luck! The horizontal offset in display math actually seems to be an improvement over past behavior. Here's And here's the same test on this PR: The new version has the correct bounding box on the So I'm happy with all the screenshots now. I'll clean up the code according to the comments next. |
@kevinbarabash I implemented all your comments on the code. This should be good to go now, except for probably we should add some multiline line-breaking tests. |
@edemaine thanks. This looks great. I'm so happy you were able to sort out the horizontal spacing differences. I'll do another pass this evening. |
Added a screenshot test, which should test everything, including |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. So excited to have this in the project!
@@ -205,7 +205,7 @@ export const getTypeOfDomTree = function(node, side = "right") { | |||
// 'mtight' indicates that the node is script or scriptscript style. | |||
export const isLeftTight = function(node) { | |||
node = getOutermostNode(node, "left"); | |||
return utils.contains(node.classes, "mtight"); | |||
return node.hasClass("mtight"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
* Combine an array of HTML DOM nodes (e.g., the output of `buildExpression`) | ||
* into an unbreakable HTML node of class .base, with proper struts to | ||
* guarantee correct vertical extent. `buildHTML` calls this repeatedly to | ||
* make up the entire expression as a sequence of unbreakable units. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice comment.
@@ -654,10 +654,60 @@ export default function buildHTML(tree, options) { | |||
// normal place) so we use an absolute value for vertical-align instead | |||
bottomStrut.style.verticalAlign = -body.depth + "em"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that you could pass verticalAlign
a length. It makes me wonder why the topStrut
is even necessary since the bottomStrut
's height is already body.height + body.depth
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good question! I should experiment with removing the topStrut.
export default function buildHTML(tree, options) { | ||
// buildExpression is destructive, so we need to make a clone | ||
// of the incoming tree so that it isn't accidentally changed | ||
tree = JSON.parse(JSON.stringify(tree)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize this. I wonder how we're mutating tree
. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious too! I haven't seen any, but this comment has been in the code for a long time.
i++; | ||
parts.push(expression[i]); | ||
if (expression[i].hasClass("nobreak")) { | ||
nobreak = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot cleaner.
Is anyone able able to view side-by-side images in the files tab on GitHub? All I see is |
I had a look at the screenshots. They look good. |
@kevinbarabash Check out #1267 (comment) . I was playing with the various "rich diff" options and they're pretty cool! You can even overlay the two imaged in various ways (though not as good as our red/green diffs). Thanks for the review! I excited to use this feature. |
As suggested in KaTeX#1287 (comment) the two struts were redundant; the formerly "bottom" strut suffices to implement both height and depth constraints.
* One strut instead of two As suggested in #1287 (comment) the two struts were redundant; the formerly "bottom" strut suffices to implement both height and depth constraints. * Update screenshots
Allow for line breaks in inline formulas according to TeX's rules. Display formulas are not affected. Part of #327, specifically the plan suggested in #327 (comment)
The TeXBook [p.173] says "A formula will be broken only after a relation symbol like$=$ or $<$ or $\rightarrow$ , or after a binary operation symbol like $+$ or $-$ or $\times$ , where the relation or binary operation is on the ``outer level'' of the formula (i.e., not enclosed in {...} and not part of an \over construction)."
In inline math, we break the outer
katex-html
into multiplebase
spans, splitting wherever a line break can be (in particular, right after top-levelmop
ormbin
). Eachbase
span gets its own strut, which handles vertical spacing between lines properly. This was easier than I thought it'd be!In addition, I added support for
\allowbreak
and\nobreak
which modify this behavior. By also marking them as (zero-width) spaces, I believe they don't affect anything else about layout.Examples from the test server with
display=0
set: