Shrinkwrap vlists in table-like CSS. #768

kohler · 2017-07-18T22:58:06Z

This PR ~~is not ready to merge, but I'd appreciate feedback on the approach~~ is ready. @ronkok @kevinbarabash

Relevant commit message:

TeX and CSS treat line heights in fundamentally different ways. In
TeX, every character is treated as a box of its precise height and
depth; the line height (\baselineskip) applies after characters have
been assembled into lines. In CSS, in contrast, every character
creates a "line box" corresponding to the accompanying font. When
characters of different fonts and sizes are placed into the same
span, the resulting line box contains the line boxes of all children.

This is unfortunate because, for example, we want \frac{1}{2} to
behave in vertical spacing contexts like it is exactly as tall and
deep as the visible fraction (which is the TeX behavior). Given CSS
constraints, though, in most contexts the fraction has extra vertical
space: the line boxes for the numerator and denominator create
padding. For small boxes, this isn't so bad. To really see the
problem put a tall rule in the denominator of a fraction, or check
out the VerticalSpacing screenshotter test, which has way more space
than it should.

Solving this problem in CSS is difficult. There is no easy way to get
rid of the extra line boxes.

But there is a way, namely tables. A table-cell with vertical-align
top, bottom, or middle is ignored for the purposes of line height
calculation.

So in this commit, makeVList puts its contents into a
vertical-align:bottom table-cell (to clear unwanted line boxes), with
an extra row used to represent depth.

Many Chrome screenshotter tests change. This is because Chrome rounds
table dimensions to integral numbers of pixels, while it uses
sub-pixel positioning for non-table displayed tabs. That makes many
vlists a fraction of a pixel wider than they used to be.

ronkok · 2017-07-19T01:34:44Z

Wow. Enormously impressive.

Of course this needs lots of cross-browser testing, so I'll help with that. This solves a tough problem.

Now I have a question. There are couple of lines that contain a constant of 0.91:

child.style.top = (-child.strutSize * 0.91 + totalSize - child.bottom) + "em";

    vtable.style.top = (0.91 - vlist.height) + "em";

I expected that value to be 0.9, not 0.91, because the KaTeX fonts have an ascend value of 0.9. Is there any particular reason you used 0.91?

I'm glad you mentioned the extra space in the VerticalSpacing screenshotter test. When I was testing PR #670, I repeatedly saw this kind of extra space, and I'm glad that someone else has noticed it.

In an earlier thread, @gagern mentioned that any use of tables should be careful that the CSS resets table style. I don't think your CSS would conflict, but it is a good thing to keep in mind.

kohler · 2017-07-19T11:57:19Z

The 0.91 was determined empirically, by comparing baseline-aligned rules in and out of vlists. I'd think it would be 0.9 (the line-height is 1.2em and the typographic ascender 0.8em, so you'd think 0.1em at the top + 0.8 ascender = 0.9em), but it's not.

I also added some comments, and thanks, added the table-layout: fixed I expect @gagern was referring to.

Compatibility could be an issue here. In particular, the vertical positioning in this checkin makes assumptions about (1) the ascender of a particular font, (2) the way line-boxes are calculated. These assumptions are new: the old code did produce different results on different browsers, but only in terms of the extra whitespace around KaTeX math, rather than within the math itself.

According to CSS 2.1 the ascender “should” be calculated using particular font metrics: sTypoAscender, sTypoDescender. But there might be differences on different platforms. So curious what happens on modern, KaTeX-supported browsers.

edemaine · 2017-07-19T14:00:17Z

Sounds very cool! I've always been confused/annoyed by line boxes, so it's great to see a possible fix in this context.

Regarding @gagern's table comment, he may have also been referring to CSS code like this (taken from my own project), which changes the default style of all tables in a certain context. Both Markdown tables and KaTeX output could appear in this context (of a Markdown + LaTeX formatted message). But these rules are relevant only to tags like <table>, <tr>, <td>, not CSS-style tables. So I think no issue here either.

ronkok · 2017-07-20T15:53:34Z

I've tested this code on my box, on my phone. and on BrowserStack with various styles, sizes, and images. It works great on Edge, IE 9-11, iOS Safari, Mac Safari, Windows Chrome, Android Chrome, Opera, and Firefox.

Terrific stuff.

kohler · 2017-07-20T16:17:40Z

Thanks @ronkok!!

I had, though, an idea to make the mechanism less sensitive to font ascender dimensions, namely a tall overflow:hidden strut. That makes many Firefox images match the screenshotter versions exactly and seems more robust.

However, Chrome versions don't match—in the horizontal direction. This was a surprise to me. It appears that Chrome rounds table-cell dimensions up to integral numbers of pixels, but does not do this for divs. Example diff attached. Maybe this is a blocker, maybe it's not.

edemaine · 2017-07-20T16:40:03Z

Annoying. Here's the Chrome issue.

edemaine · 2017-07-20T16:42:31Z

Personally, I think this approach is still worth persuing. It's not a huge loss, and Chrome will hopefully eventually fix, while we gain... a lot, I think? Can you show some examples where this new approach to vlists is a significant improvement over the current one?

ronkok · 2017-07-20T17:54:09Z

Oh, I think this PR is a big win. The problem is inherent anytime a tall element is passed as an argument to makeVList. Below are two screen shots of the same HTML. The first screenshot is from the existing KaTeX master, and the second shot is from the current PR.

That gap in the first shot is the problem. My work used a hack to mitigate it only for \fbox and \cancel. This PR resolves the problem entirely.

kevinbarabash · 2017-07-20T19:09:09Z

@edemaine thanks for digging up that issue. It sounds like they want to fix it and are prioritizing it so that's promising.

@kohler since it looks like this bug is going to be fixed in Chrome and it doesn't exist in other browsers the rounding issues probably shouldn't be a block, but it'd be good to do some testing at smaller font sizes to see how much of an impact this has on layouts at those sizes.

kohler · 2017-07-20T21:23:39Z

@kevinbarabash I've done some tests with 16px fonts, and the rounding issues are worse, but not dramatically worse. Three examples from screenshotter tests:

kohler · 2017-07-21T16:45:30Z

I further updated the CSS. By using two table-rows, and a table-cell with vertical-align: bottom, all reliance on line-height and font ascent can be avoided, and the code is simpler.

Only a small number of firefox test cases change. Most chrome test cases change, because of the rounding issue.

Assuming that the updated CSS works on other browsers—it appears to, but @ronkok, if you could help check that would be amazing—this PR is in my opinion ready to merge.

ronkok · 2017-07-21T18:14:43Z

I am now seeing problems in Safari, on both iOS and Mac. All other browsers (IE, Edge, Chrome, Firefox, Opera) look good. I do my Safari testing on BrowserStack, so it should be checked by someone with native equipment. Here's a sample of the problems:

kohler · 2017-07-21T18:59:40Z

Thanks @ronkok! That was a real problem: Safari needed an extra table-cell to position the table correctly. I've fixed that problem and things display well on local Safari, at least.

kohler · 2017-07-21T19:05:54Z

@ronkok, did you know that accent placement was off on Safari? The \vec’s are rightwards-displaced even on master. The commit I just pushed is correct vertically though.

ronkok · 2017-07-21T19:09:27Z

Yes, the \vec horizontal problem is issue #735. It will need its own solution.

ronkok · 2017-07-21T20:16:37Z

@kohler, That last commit did the trick. This PR now works in Chrome, Firefox, Edge, IE 9-11, Safari, and Opera.

This problem has caused me many hours of frustration, so I'm very glad that you have solved it.

kohler · 2017-07-27T12:21:47Z

Ping—can a girl get a review? @kevinbarabash

kevinbarabash · 2017-07-27T14:42:55Z

@kohler sorry for the delay. I'll review this this weekend.

kevinbarabash · 2017-07-29T23:04:12Z

These assumptions are new: the old code did produce different results on different browsers, but only in terms of the extra whitespace around KaTeX math, rather than within the math itself.

I noticed this when reviewing the screenshots. It seems like there is generally less extra padding which is a good thing imo.

But there might be differences on different platforms.

Awesome article.

Three examples from screenshotter tests:

I was concerned about vertical/horizontal rules getting too close to numbers/letters. The examples in the screenshot look alright.

kevinbarabash · 2017-07-29T23:07:33Z

src/buildCommon.js

        }
    }
-    const fontSizer = makeFontSizer(options, maxFontSize);
+    pstrutSize += 2;


This is so that it's a little taller than any of the children?

Yep, taller than any child—or the height of any child.

kevinbarabash · 2017-07-29T23:08:21Z

src/buildCommon.js

        }
    }
-    const fontSizer = makeFontSizer(options, maxFontSize);
+    pstrutSize += 2;
+    const pstrut = makeSpan(["pstrut"], []);


I'm curious about the name pstrut... what does the p represent?

“positioning strut”.

kevinbarabash · 2017-07-29T23:11:29Z

src/buildCommon.js

    for (i = 0; i < children.length; i++) {
        if (children[i].type === "elem") {
-            maxFontSize = Math.max(maxFontSize, children[i].elem.maxFontSize);
+            const child = children[i].elem;
+            pstrutSize = Math.max(pstrutSize, child.maxFontSize, child.height);


I'm guessing this will have to be updated to support \rasiebox eventually.

kevinbarabash · 2017-07-29T23:28:12Z

src/buildCommon.js

-            childWrap.depth += shift;
-            childWrap.style.top = shift + "em";
+            const childWrap = makeSpan([], [pstrut, child]);
+            childWrap.style.top = (-pstrutSize - currPos - child.depth) + "em";


Just so I understand this correctly, the pstrut in the childWrap is taller than the child so that we avoid issues with the baseline alignment and can align the bottom of the child's glyph bounding box with the bottom of the pstrut? And then childWrap is position vertically so that child appears where we want it to.

kevinbarabash · 2017-07-29T23:35:15Z

src/buildCommon.js

+        const topStrut = makeSpan(["vlist-s"], [new domTree.symbolNode("\u200b")]);
+
+        rows = [makeSpan(["vlist-r"], [vlist, topStrut]),
+            makeSpan(["vlist-r"], [depthStrut])];


In what situations is the depthStrut necessary? I tried removing it from some fraction layouts and didn't notice a difference.

The depthStrut signals to the browser how deep the box is. This isn't that relevant in normal layouts; KaTeX understands heights & depths. But it matters for layout of surrounding lines. That also might not matter given the current coding (which has separate struts done at the very outermost level), but I think it's good and useful to have the vlist coding produce a box of the correct height and depth.

kevinbarabash · 2017-07-29T23:38:14Z

src/buildCommon.js

+
+    const vtable = makeSpan(["vlist-t"], rows);
+    vtable.height = maxPos;
+    vtable.depth = Math.max(-minPos, 0);


I didn't realize that depth couldn't be negative. Why is this?

In TeX it can be negative but usually isn't. The current code works, and I thought that it was producing the same results as the old code—should I check without the max-0?

kevinbarabash · 2017-07-29T23:41:24Z

static/katex.less

@@ -564,16 +581,14 @@
    }

    // Lengthen the extensible arrows via padding.
-    .x-arrow > span > span {
-        text-align: center;


I see, this got moved down to line 591.

kevinbarabash · 2017-07-29T23:42:23Z

static/katex.less

@@ -564,16 +581,14 @@
    }

    // Lengthen the extensible arrows via padding.
-    .x-arrow > span > span {


Why don't we have to be as specific as we were being before with these rules?

kevinbarabash · 2017-07-29T23:44:31Z

static/katex.less

+    .vlist-s {
+        display: table-cell;
+        vertical-align: bottom;
+        font-size: 0.05em;


Is the 0.05em b/c we want the impact of the font size on the base line of elements with this class to be negligible?

kevinbarabash · 2017-07-29T23:50:50Z

static/katex.less

@@ -232,8 +232,25 @@
        & + .mop.mtight { margin-left: @thinspace; }
    }

+    .vlist-t {
+        display: inline-table;
+        table-layout: fixed;


Reading https://developer.mozilla.org/en-US/docs/Web/CSS/table-layout it says that table-layout: fixed bases the width of the table on the first row. It looks like the first row is always contains the content for the vlist. Is that right?

Apparently table-layout: fixed renders faster. I'm curious if the decision to use this was motivated by perf concerns or something else.

Motivated by the fact that the other table displays in katex.less have table-layout: fixed.

TeX and CSS treat line heights in fundamentally different ways. In TeX, every character is treated as a box of its precise height and depth; the line height (\baselineskip) applies after characters have been assembled into lines. In CSS, in contrast, every character creates a "line box" corresponding to the accompanying font. When characters of different fonts and sizes are placed into the same span, the resulting line box contains the line boxes of all children. This is unfortunate because, for example, we want `\frac{1}{2}` to behave in vertical spacing contexts like it is exactly as tall and deep as the visible fraction (which is the TeX behavior). Given CSS constraints, though, in most contexts the fraction has extra vertical space: the line boxes for the numerator and denominator create padding. For small boxes, this isn't so bad. To really see the problem put a tall rule in the denominator of a fraction, or check out the VerticalSpacing screenshotter test, which has way more space than it should. Solving this problem in CSS is difficult. There is no easy way to get rid of the extra line boxes. But there is *a* way, namely tables. A table-cell with vertical-align top, bottom, or middle is ignored for the purposes of line height calculation. So in this commit, makeVList puts its contents into a vertical-align:bottom table-cell (to clear unwanted line boxes), with an extra row used to represent depth. Many Chrome screenshotter tests change. This is because Chrome rounds table dimensions to integral numbers of pixels, while it uses sub-pixel positioning for non-table displayed tabs. That makes many vlists a fraction of a pixel wider than they used to be.

Screenshotter tests don't change.

kohler · 2017-07-30T14:10:33Z

Inspired by your question, I changed the PR slightly to produce negative heights and depths when appropriate. Screenshotter tests OK.

kevinbarabash

LGTM

kevinbarabash · 2017-07-30T15:25:36Z

@kohler thanks for the PR. I believe this unblocks #685 and changes to \sqrt previewed in #60. Is that right @edemaine and @ronkok?

ronkok · 2017-07-30T15:35:11Z

It certainly unblocks the \sqrt changes. I'll work up a PR today.

ronkok · 2017-07-30T16:47:56Z

I'm finding a problem on Safari. It occurs in iOS only, not Mac. The screenshot below is from BrowserStack's rendition of an iPad Pro, iOS 10.3

That is trying to render part of the code: \( \sqrt A \, \sqrt{M^2} \, \sqrt{\vec F} \, \color{blue} \sqrt{\frac{\frac{\frac{c}{c}}{c}}{\frac{c}{c}} } + \sqrt{\frac{\frac{\frac{\frac{c}{c}}{\frac{c}{c}} }{c}}{\frac{c}{c}} } \, \sqrt{\frac{\frac{\frac{\frac{c}{c}}{\frac{c}{c}} }{c}}{\frac{c}{\frac{\frac{c}{c}}{\frac{c}{c}}}} } \)

Although I can't give you a screenshot, I am seeing the same problem on my iPod touch.

kohler · 2017-07-30T18:23:15Z

Do you pay for BrowserStack? I looked briefly and it looked painful to get free access.

…

On Jul 30, 2017, at 12:48 PM, Ron Kok ***@***.***> wrote: I'm finding a problem on Safari. It occurs in iOS only, not Mac. The screenshot below is from BrowserStack's rendition of an iPad Pro, iOS 10.3 That is trying to render: \( \color{blue} \sqrt{\frac{\frac{\frac{c}{c}}{c}}{\frac{c}{c}} } + \sqrt{\frac{\frac{\frac{\frac{c}{c}}{\frac{c}{c}} }{c}}{\frac{c}{c}} } \, \sqrt{\frac{\frac{\frac{\frac{c}{c}}{\frac{c}{c}} }{c}}{\frac{c}{\frac{\frac{c}{c}}{\frac{c}{c}}}} } \) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ronkok · 2017-07-30T18:47:05Z

I do pay for BrowserStack. For the short term, at least.

kevinbarabash · 2017-07-30T21:08:00Z

Another way to test on iOS is use the simulator that comes with Xcode.

kohler · 2017-07-30T22:31:37Z

This appears fine on master (which now has this PR). This image is from the iOS Simulator for IPad Pro (10.5-inch) – iOS 10.3 (14E8301)

ronkok · 2017-07-30T23:54:52Z

I still see the problems. I see them only when the iPad is in portrait orientation, not landscape. That opens up a line of investigation. The page I'm looking at contains some CSS media queries for screen width. Among these are:

@media all {body {font-size: 20px;}}
@media (max-width:830px){body {font-size: 18px;}}
@media (max-width:740px){body {font-size: 16px;}}
@media (max-width:670px){body {font-size: 14px;}}

I'll investigate some more and let you know if I can pin this down further. Maybe not today, though.

ronkok · 2017-08-01T15:55:43Z

@kohler, I've prepared another test page here. This page has CSS body {font-size: 18px;} regardless of screen orientation. This page also shows makeVList problems on Safari - iOS.

That page, I should add, is running today's KaTeX master.

kevinbarabash · 2017-08-01T18:05:01Z

Interestingly that test page looks good at 72px and then falls apart as the font size is decreased.

@ronkok

Some delicate surgery is required on the `vlist-s` table-cell CSS. Problem with KaTeX#768 reported by @ronkok; issue KaTeX#779. Fixes: * `font-size: 1px` (rather than `font-size: 0.05em`) solves render issues with user overrides of the `.katex` `font-size`. * Also fix a width issue.

@ronkok

Some delicate surgery is required on the `vlist-s` table-cell CSS. Problem with #768 reported by @ronkok; issue #779. Fixes: * `font-size: 1px` (rather than `font-size: 0.05em`) solves render issues with user overrides of the `.katex` `font-size`. * Also fix a width issue.

ry-randall · 2018-02-23T18:27:27Z

Just wanted to say thank you for this change. I'm working on a canvas renderer implementation, and this is a huge life saver.

ronkok mentioned this pull request Jul 19, 2017

Fix \boxed, add \overbracket #757

Closed

kohler force-pushed the shrinkwrap branch from c1a78ff to bd79e00 Compare July 21, 2017 16:43

kohler mentioned this pull request Jul 21, 2017

Supply marginLeft/marginRight as inputs to makeVList. #767

Merged

kohler force-pushed the shrinkwrap branch from bd79e00 to 92de475 Compare July 21, 2017 17:49

kohler force-pushed the shrinkwrap branch from 92de475 to 857d51a Compare July 21, 2017 18:56

kohler force-pushed the shrinkwrap branch from 857d51a to e5fba58 Compare July 27, 2017 15:16

kevinbarabash self-requested a review July 29, 2017 21:14

kevinbarabash reviewed Jul 29, 2017

View reviewed changes

kohler added 2 commits July 30, 2017 09:40

As Kevin Barabash points out, heights & depths can be negative.

33ad6e3

Screenshotter tests don't change.

kohler force-pushed the shrinkwrap branch from e5fba58 to 33ad6e3 Compare July 30, 2017 13:54

kevinbarabash approved these changes Jul 30, 2017

View reviewed changes

kevinbarabash merged commit 2da06d5 into KaTeX:master Jul 30, 2017

ronkok mentioned this pull request Jul 30, 2017

Square root line position is inconsistent (depending on its contents) #60

Closed

kohler mentioned this pull request Jul 31, 2017

Enable tall items in makeVList #746

Closed

kohler mentioned this pull request Aug 1, 2017

makeVList problems on Safari given specific CSS context #779

Closed

kohler deleted the shrinkwrap branch August 1, 2017 19:38

kohler mentioned this pull request Aug 1, 2017

Solve Safari rendering issues with font-size overrides. #780

Merged

ronkok mentioned this pull request Aug 15, 2017

vertical alignment for \rule seems off #646

Closed

edemaine mentioned this pull request Sep 8, 2017

Dividing lines for \frac and overhead lines for \sqrt sometimes disappear #824

Closed

ronkok mentioned this pull request Oct 18, 2017

Outline of formulas containing superscripts is strangely high #927

Closed

ronkok mentioned this pull request Apr 5, 2019

Fix \pmb #1924

Merged

ronkok mentioned this pull request Feb 1, 2020

Reduce redundant DOM elements #2194

Open

Shrinkwrap vlists in table-like CSS. #768

Shrinkwrap vlists in table-like CSS. #768

Conversation

kohler commented Jul 18, 2017 • edited Loading

ronkok commented Jul 19, 2017

kohler commented Jul 19, 2017 • edited Loading

edemaine commented Jul 19, 2017

ronkok commented Jul 20, 2017

kohler commented Jul 20, 2017

edemaine commented Jul 20, 2017

edemaine commented Jul 20, 2017

ronkok commented Jul 20, 2017 • edited Loading

kevinbarabash commented Jul 20, 2017

kohler commented Jul 20, 2017 • edited Loading

kohler commented Jul 21, 2017

ronkok commented Jul 21, 2017

kohler commented Jul 21, 2017

kohler commented Jul 21, 2017

ronkok commented Jul 21, 2017

ronkok commented Jul 21, 2017

kohler commented Jul 27, 2017

kevinbarabash commented Jul 27, 2017

kevinbarabash commented Jul 29, 2017

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

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

kohler commented Jul 30, 2017

kevinbarabash left a comment

Choose a reason for hiding this comment

kevinbarabash commented Jul 30, 2017

ronkok commented Jul 30, 2017

ronkok commented Jul 30, 2017 • edited Loading

kohler commented Jul 30, 2017 via email

ronkok commented Jul 30, 2017

kevinbarabash commented Jul 30, 2017

kohler commented Jul 30, 2017 • edited Loading

ronkok commented Jul 30, 2017

ronkok commented Aug 1, 2017 • edited Loading

kevinbarabash commented Aug 1, 2017

ry-randall commented Feb 23, 2018 • edited Loading

kohler commented Jul 18, 2017 •

edited

Loading

kohler commented Jul 19, 2017 •

edited

Loading

ronkok commented Jul 20, 2017 •

edited

Loading

kohler commented Jul 20, 2017 •

edited

Loading

ronkok commented Jul 30, 2017 •

edited

Loading

kohler commented Jul 30, 2017 •

edited

Loading

ronkok commented Aug 1, 2017 •

edited

Loading

ry-randall commented Feb 23, 2018 •

edited

Loading