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

Support stretchy wide elements. #670

Merged
merged 22 commits into from
Jun 16, 2017
Merged

Support stretchy wide elements. #670

merged 22 commits into from
Jun 16, 2017

Conversation

ronkok
Copy link
Collaborator

@ronkok ronkok commented Mar 28, 2017

This will fix issue #629 and possibly #529. Currently, Chrome will fail to render the arrow in \vec when CSS contains text-rendering: optimizeLegibility;.

@khanbot
Copy link

khanbot commented Mar 28, 2017

CLA signature looks good 👍

@ronkok ronkok changed the title Fix \vec rendering bug. Support stretchy wide elements. Apr 4, 2017
@ronkok
Copy link
Collaborator Author

ronkok commented Apr 4, 2017

I have resolved all the lint errors. The error messages I'm getting now are, to me, cryptic. Any help would be appreciated.

@sthoch
Copy link

sthoch commented Apr 5, 2017

Cool. I feel like the arrows should be a tad thicker, but other than my gut, I have no reference atm.

The errors from here: https://travis-ci.org/Khan/KaTeX/builds/218513949 don't seem cryptic to me. What errors are you refering to?

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 5, 2017

Yes, I've looked the link you have provided. Yesterday, I cleaned up all the lint errors. The errors that remain start at line 858 of https://travis-ci.org/Khan/KaTeX/builds/218513949#L858. The typical remaining error says something such as:

npm ERR! Linux 4.4.0-51-generic

And though some may find that statement to be very informative, I confess that I find it cryptic.

@edemaine
Copy link
Member

edemaine commented Apr 5, 2017

Can you push your fixes to the lint errors, which will trigger a rerun of travis? Those npm ERR messages are just a summary saying that lint failed, and should disappear once the lint errors are gone.

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 5, 2017

Well, @edemaine, I've made another commit, titled "Fix lint errors". I hope that qualifies as the "push" that you alluded to. If not, let me know. And I appreciate the help.

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 5, 2017

Well, that's progress. The lint corrections have passed. Travis now fails on Sreenshotter.

I have had no success running Screenshotter locally. If anyone else would like to try, that would be fine with me.

@edemaine
Copy link
Member

edemaine commented Apr 6, 2017

Hmm. I can run screenshotter, but I couldn't reproduce the error that Travis is getting on your branch. My experience is that screenshotter occasionally fails randomly (on my second try, the Colors test failed), so perhaps if Travis tried again, it would work. (But I don't know how to get Travis to try again, other than another random commit.) Anyway, probably don't need to worry about this automatic check, and instead an actual maintainer should review this PR.

src/stretchy.js Outdated
}
} else {
// Add color. Use CSS mask, if possible
if (maskIsSupported()) {
Copy link
Contributor

@glebm glebm Apr 7, 2017

Choose a reason for hiding this comment

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

Currently this will not emit the mask when rendering server-side. It'd be nice to emit the mask, or both, perhaps using @supports somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I run server.js and view it in http://localhost:7936/, I get the mask when I view it in Chrome, the background-image when I view it in Firefox, and the inline background-image when I view it in Edge. That's the same behavior I get when I view the web page.

I completely agree that we want a server side rendering to emit the mask, but my experience is that we already get that. Am I doing something wrong?

Copy link
Member

Choose a reason for hiding this comment

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

The examples you describe both involve a web browser, a.k.a. a client. The point is that, if you run KaTeX from Node, it won't "support" mask, and so won't be omitted. You simply can't detect support for anything in a server-only environment. Can't you emit the mask in all cases, and it will just work when it's supported?

Copy link
Contributor

@glebm glebm Apr 7, 2017

Choose a reason for hiding this comment

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

To always emit the mask for SSR, you could do something like:

function maskIsSupported() {
  // If rendering server-side, assume the client supports CSS masks,
  // as most clients do:
  if (typeof document === "undefined") return true;
  ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@edemaine , I can omit the mask in all cases, but the non-mask solution relies on a XMLHttpRequest, which is slow.

@glebm, I would agree with your suggestion. If I interpret caniuse correctly, the upcoming version of Firefox will change from partial mask support to full support. That would leave behind only IE and Edge. However, I'm not sure that the KaTeX owners would agree with you and me. If Edge does look at the output from such a server-side installation, it will render a span-wide blob of color instead of the subject matter.

So I thank both of you for the education on server-side behavior. This affects not just \color but also \cancel, another case where I have different code for IE and Edge.

Regarding \cancel, I'm going to use background-image SVGs for server-side rendering, just as I use it now for IE and Edge. The only drawback of the SVGs in most browsers is that the line does not get thicker when giant fonts sizes are employed. A 1 px line looks good in most common font sizes, so that is a good fall-back.

On the subject of \color, I need some guidance from the KaTeX owners as to what they will accept. The options are:

  1. Do not support \color.
  2. Support \color by a (slow) XMLHttpRequest in all cases.
  3. Use mask when it is supported. Use mask for server-side installations even
    though Edge will not render it properly.
  4. Use mask when I can use feature detection to verify that it is supported. Use a XMLHttpRequest for all server-side installations.

Copy link
Member

@edemaine edemaine Apr 7, 2017

Choose a reason for hiding this comment

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

If there isn't a good one-size-fits-all solution, it seems like the best thing to do would be to add an option that lets you decide between "work on Edge" vs. "work fast", when you can't autodetect it.

I'm a little confused why you need an XMLHttpRequest, though. Is it not worth just inlining the SVG into the JavaScript code (const mySVG = "...") and using that as needed? I remember we discussed this before, but why use external files at all, when you can embed it all in the JavaScript source?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a very good point. Since our conversation, I've added some SVG files. The tactic of making composite images via CSS was giving some poor rendering issues. So the SVG code is now 32 KB. Say about 40 KB once I add the URL escape codes. By inlining that into the JavaScript, we could eliminate all XMLHttpRequest's. So now we can add option:

  1. Add 40 KB of SVG code inline in the JavaScript. No XMLHttpRequest, ever.

I still need some guidance from KaTeX owners. Which option would you prefer?

Copy link
Contributor

@glebm glebm Apr 11, 2017

Choose a reason for hiding this comment

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

@ronkok

If Edge does look at the output from such a server-side installation, it will render a span-wide blob of color instead of the subject matter.

If we go with option 3, it is possible to always render a black arrow for browsers that do not support it:

@supports not (mask-image: #000) {
  .stretchy {
    background-color: transparent !important;
  }
  .x-arrow {
    background-image: url(...) !important;
  }
  ...
}

For IE, @supports is not supported but the \9 hack can be used instead.

@@ -0,0 +1,43 @@
<svg xmlns="http://www.w3.org/2000/svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

These SVGs should be minified, ideally at build time. I recommend svgo for this, can easily shave off 30%.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 8, 2017

Here's a thought: If I inline the SVG code in the JavaScript (to enable server-side \color), that opens up another possibility. I could then write all the SVGs inline, always. That would eliminate the entire /images/ folder and all its SVG files. And KaTeX would never make a HttpRequest for an image file. So it would be faster. The downside is a lot more bloat in the HTML.

Comments?

@glebm
Copy link
Contributor

glebm commented Apr 11, 2017

So it would be faster.

Not necessarily, as the browser would need to parse and render a separate SVG image for every arrow from scratch. The giant HTML payload would also make server-side rendered downloads much slower.

I'd much prefer to always use CSS masks and render black arrows for browsers that do not support them (via @supports not + IE hacks). The SVGs can be inlined into the CSS at build time to avoid XHR requests.

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 11, 2017

@edemaine, I've created a local version with all the SVG code inline. It increases the JavaScript size by 20K, which is less than I expected. I took a pretty extreme approach to eliminating code duplication. For instance, I wrote the typical opening SVG tag only once and I wrote the path for a rightarrow only once. Then, for any specific image, I append these strings together on the fly to create the final complete SVG code. It's pretty compact code, but I suspect it would be hard to maintain and extend.

@glebm, If were to summarize your thoughts in my own words, it would go like this:

  1. Move feature detection and/or browser detection from the JavaScript to the CSS. That enables detection in server-side rendering.
  2. Use CSS mask when supported and render black images when mask is not supported. That makes inline SVG code unnecessary.

That would complicate the CSS pretty substantially, but it is in every other way very attractive. I'll work up a prototype. I'm leaning towards this approach myself.

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 11, 2017

@glebm, I now have @supports partly working. It properly controls the background-image and the mask-image in Chrome, Edge, and Firefox. Now I need to write an inline style to set the background-color. The JS is node.style.backgroundColor = options.color; It has to be inline because I don't know at build time what color the user will want.

How does one use @supports to control whether or not an inline style gets rendered?

@glebm
Copy link
Contributor

glebm commented Apr 11, 2017

@ronkok I believe the !important qualifier in the property values inside the @supports not should take priority over the inline styles (so the background-color can be set back to transparent if the browser does not support masks).

@@ -0,0 +1,43 @@
<svg xmlns="http://www.w3.org/2000/svg">
<svg viewBox="0 0 400000 549" preserveAspectRatio="xMinYMin slice">
Copy link
Member

Choose a reason for hiding this comment

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

Why the extra <svg>? Couldn't viewBox and preserveAspectRatio be set on the outer <svg>?

Copy link
Collaborator Author

@ronkok ronkok Apr 16, 2017

Choose a reason for hiding this comment

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

@kevinbarabash, Some of the SVGs are nested, so that the inner SVG can keep an undisturbed aspect ratio. For instance, we want an arrow head to be un-distorted when the arrow gets longer or shorter. In these files, each arrow SVG contains an arrow that is 400 ems long in the inner SVG. The outer SVG is set to match the dimensions defined for it by the CSS. The outer SVG clips the inner SVG; it does not stretch the inner SVG. That's the purpose of the preserveAspectRatio="xMinYMin slice" notation.

Of course, we do want some of the SVGs, like \cancel, to vary their aspect ratio. Those files do not get the nested SVG treatment.

@kevinbarabash
Copy link
Member

@ronkok awesome PR. I haven't done a close reading of the changes yet, but we'll definitely want some screenshot tests for the new functionality.

@ronkok
Copy link
Collaborator Author

ronkok commented Apr 16, 2017

@kevinbarabash, Regarding screenshots, I agree. I also have some work underway that improves the sever-side rendering, as suggested by @glebm. I'm busy on other things for a couple of days, but sometime next week I'll get you some screenshots and a squashed PR.

@ronkok
Copy link
Collaborator Author

ronkok commented May 2, 2017

@kevinbarabash, I've updated PR #670 with my latest work, improving server-side rendering and minfying the SVG files. I've also squashed the PR. It's ready for your review whenever you find it convenient. You'll find that the JavaScript and the HTML are simpler, but the CSS is more complex.

I've also added some Jasmine tests. I tried to add Screenshotter tests, but failed. I don't plan to try again. On the other hand, there is more than one way to contribute. If you want to assign some Screenshotter work to someone at KA, I would be happy to make a financial contribution that leaves KA whole. The Screenshotter code that I tried to add is:

Boxed: \boxed{F=ma} \quad \boxed{ac}\color{magenta}{\boxed{F}}\boxed{F=mg}

ExtensibleArrows: |
    \begin{array}{l}
        \xrightarrow[ab]{ABC} + \xRightarrow{ABC} + \xrightleftharpoons[ab]{ABC} + \xhookrightarrow[ab]{ABC} \\
        \xtwoheadrightarrow{ABC} + \frac{\xrightarrow[ab]{ABC}}{\xrightarrow[ab]{ABC}} + \left\lvert\xrightarrow[ab]{ABC}\right\rvert
    \end{array}

HorizontalBraces: \overbrace{\displaystyle{\oint_S{\vec E\cdot\hat n\,\mathrm d a}}}^\text{emf} = \underbrace{\frac{q_{\text{enc}}}{\varepsilon_0}}_{\text{charge}}
HorizontalBrackets: |
    \begin{array}{l}
        \overbracket{(2x+3)}^\text{factor 1}\,\underbracket{(4x+5)}_\text{factor 2} \\
        \overbracket[0.15em][1.2em]{10\rightarrow 9}^{\text{high}} \rightarrow \overbracket[0.05em]{2\rightarrow 1}^{\text{low}}
    \end{array}

LowerAccent: |
    \begin{matrix}
        \underleftarrow{AB} \quad \underrightarrow{AB} \quad \underleftrightarrow{AB} \quad \undergroup{AB} \\
        \underlinesegment{AB} \quad \undertilde{AB}  \quad \color{green}{\underrightarrow{AB}} \\
        \underrightarrow{F} + \underrightarrow{AB} + \underrightarrow{AB}^2 + \underrightarrow{AB}_2 \\
        \frac{\underrightarrow{AB}}{\underrightarrow{AB}} + \sqrt{\underrightarrow{AB}} + \left\lvert\underrightarrow{AB}\right\rvert
    \end{matrix}

StretchyAccent: |
    \begin{array}{l}
        \overrightarrow{AB} \quad \overleftarrow{AB} \quad \Overrightarrow{AB} \quad \overleftrightarrow{AB} \\
        \overgroup{AB} \quad \overlinesegment{AB} \quad \overleftharpoon{AB} \quad \overrightharpoon{AB} \quad \color{red}{\overrightarrow{AB}} \\
        \widehat{\theta} \quad \widehat{AB} \quad \widehat{ABC} \quad \widetilde{A} \quad \widetilde{AB} \quad \widetilde{ABC} \\
        \overrightarrow{F} + \overrightarrow{AB} + \overrightarrow{F}^2 + \overrightarrow{F}_2 + \overrightarrow{F}_1^2 \\
        \overrightarrow{AB}^2+\frac{\overrightarrow{AB}}{\overrightarrow{AB}} + \sqrt{\overrightarrow{AB}} + \left\lvert\overrightarrow{AB}\right\rvert + \left(\overrightarrow{AB}\right)
    \end{array}
StrikeThrough: |
    \begin{array}{l}
        \cancel x \quad \cancel{2B} + \bcancel 5 +\bcancel{5ay} \\
        \sout{5ab} + \sout{5ABC} + \xcancel{\oint_S{\vec E\cdot\hat n\,\mathrm d a}} \\
        \frac{x+\cancel B}{x+\cancel x} + \frac{x+\cancel y}{x} + \cancel{B}_1^2 + \cancel{B^2} + \left\lvert\cancel{ac}\right\rvert
    \end{array}

@kevinbarabash
Copy link
Member

kevinbarabash commented May 2, 2017

@ronkok I'm sorry that make screenshots didn't work for you. I'll check out your branch later this week when I have some time. Thanks for posting the test input you were using.

@ronkok
Copy link
Collaborator Author

ronkok commented May 2, 2017

As before, I have a web page up that describes the results of PR #670. I’ve spent some time with BrowserStack, and the page now includes accurate browser support data.

@edemaine
Copy link
Member

@ronkok It looks like you did a merge with master? There's now a lot of irrelevant commits in this PR. You might try git rebase -i b9e7b6898fd68a575d953000f4741ce2a24012f0 (or some other way to specify the master commit) and follow the instructions (carefully) for interactive rebasing.

@ronkok
Copy link
Collaborator Author

ronkok commented May 10, 2017

Yes, I needed to fix a bug -- extensible arrows should be REL, not ORD. So my attempt was to make the change, then squash all the work so that KaTeX maintainers could review it. As a new GitHub user, I have two comments:

  1. GitHub’s ability to merge, diff, etc., is impressive.
  2. It’s user interface is vile.

Obviously, things went wrong. I’ll try to follow your suggestion.

Apply CSS trickery to support functions that stretch in the horizontal
direction
@ronkok
Copy link
Collaborator Author

ronkok commented May 10, 2017

I believe that the latest commit has fixed my earlier error. @edemaine, thank you for the help.

@kevinbarabash
Copy link
Member

Not sure what happened with a couple of those screenshots. I've pulled the most recent changes to ronkok/master and am running make screenshots again.

@ronkok
Copy link
Collaborator Author

ronkok commented Jun 16, 2017

I've already done the merge. I can do more. No worries.

Sometimes the only virtue that matters is persistence.

@kevinbarabash kevinbarabash merged commit eff7653 into KaTeX:master Jun 16, 2017
@kevinbarabash
Copy link
Member

@ronkok thank you for this awesome PR. I know there's lots of people waiting for all of these new commands, including Khan Academy. We'll have to do publish soon.

@ronkok
Copy link
Collaborator Author

ronkok commented Jun 16, 2017

@kevinbarabash , I realize that this PR has been more trouble for you than most. I appreciate your help and your patience. I'm glad that the value added from the PR is worth the trouble. If you need any adjustments to stretchy wide elements, I'll be glad to help.

@edemaine , @glebm, thank you as well. Between the three of you, you pulled across the finish line someone who started this work knowing very little about JavaScript or SVG and nothing at all about Git.

@edemaine
Copy link
Member

Congrats @ronkok and @kevinbarabash on this 1600-line commit! Excited to try out the new features.

@ronkok ronkok mentioned this pull request Aug 10, 2017
@ronkok ronkok mentioned this pull request Aug 20, 2017
kevinbarabash pushed a commit that referenced this pull request Nov 24, 2017
* Change \undeertilde to \utilde

In PR #670, I made an error. The function that should have been `\utilde` I named instead `\undertilde`.

There is an `\undertilde` from the `wsuipa` package, but it is a text-mode non-stretchy function. The proper command name is `\utilde`, a math-mode, stretchy function from packge `undertilde`.

This PR fixes my error.

* Fix screenshotter test
kevinbarabash pushed a commit that referenced this pull request Nov 28, 2017
Fix a spelling mistake from PR #670.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants