Upgrade \sqrt zoom and width #890

ronkok · 2017-09-17T18:13:26Z

This PR does two things:

It evades a Safari bug which improperly zooms nested SVGs. The fix applies to \sqrt and to single-ended stretchy arrows. These SVGs are now single level. Their tails are sliced off using CSS overflow: hidden. Double-ended stretchy arrows and horizontal braces are not fixed. Fixes part of \sqrt doesn't scale properly when browser is zoomed in or out on Safari #883.
It adds a min-width to \sqrt so that a \sqrt{} with an empty argument will still render an SVG whose width matches the font glyph width. Fixes \sqrt{} doesn't render anything #888.

This PR evades a Safari bug which causes nested SVGs to zoom improperly. `\sqrt` and single-ended arrow SVGs have been modified. These have been converted from nested SVGs to single level. Their long tails are now sliced off using CSS `overflow: hidden`. Safari will still improperly zoom any double-ended stretchy arrows and horizontal braces.

Even if the function argument is empty, still render an SVG whose width equals the surd glyph.

ronkok · 2017-09-17T18:40:14Z

I've updated the test page.

ronkok · 2017-09-17T19:10:57Z

I've added a commit which fixes #889.

update screenshots affected by sqrt fixes

kevinbarabash · 2017-09-17T21:05:41Z

I assumed that DisplayStyle and Smash screenshots hadn't actually changed b/c the screenshots were different for Chrome only. I should've looked at travis-ci output before making the P. @ronkok I'll send you another PR later this evening with these screenshots.

more screenshots after changes to fix sqrt

kevinbarabash

Nice solution using overflow: hidden to avoid nested SVGs.

kevinbarabash · 2017-09-18T01:46:58Z

src/buildHTML.js

-    // still get a line. So, we use a simple heuristic to decide if we
-    // should omit the body entirely. (note this doesn't work for something
-    // like `\sqrt{\rlap{x}}`, but if someone is doing that they deserve for
-    // it not to work.


Lol. I missed this comment in the previous review.

I take no credit for that comment. It was there before I got here.

That's explains why I missed in previous reviews. 😅

kevinbarabash · 2017-09-18T02:49:01Z

src/delimiter.js

@@ -329,14 +329,12 @@ const sqrtSvg = function(sqrtName, height, viewBoxHeight, options) {
    }
    const pathNode = new domTree.pathNode(sqrtName, alternate);

-    let attributes = [["width", "100%"], ["height", height + "em"]];
+    const attributes = [["width", "400em"], ["height", height + "em"]];
    attributes.push(["viewBox", "0 0 400000 " + viewBoxHeight]);


It'd be good to compute the width attribute in terms of the viewBox's width (or vice versa) so that they stay in sync.

I've consistently used 400 ems for all the wide paths. I suppose that might not be obvious to someone looking at this cold. I'll look at it.

The comment is good enough for now.

The comment is good enough for now.

Let's suppose that someone other than me comes along and writes a PR with an SVG in it. They will need to coordinate viewBox geometry and document geometry. They will need to realize this early, when they are arranging the <path> geometry. So I am trying to leave multiple comments regarding the 1000:1 ratio.

Such is my reasoning for writing 400em. Reasoning with which you may disagree, but there it is.

Would it make sense to define a const hugeWidth = '400em' somewhere and use that variable repeatedly, instead of repeating a constant like this? (And one comment could explain why this choice is sensible.) Sorry if this suggestion doesn't make sense, coming from out of context.

A constant for 400em would do no harm. But the more critical concept is the 1000:1 ratio between ems and viewBox geometry. Some future contributor could waste a lot of time if they don't realize that early in their work. So any constant, defined in one place, would be improved by adding multiple comments, scattered widely.

kevinbarabash · 2017-09-18T02:57:59Z

src/delimiter.js

@@ -353,21 +351,24 @@ const sqrtSpan = function(height, delim, options) {
        sizeMultiplier = newOptions.sizeMultiplier / options.sizeMultiplier;
        spanHeight = 1 * sizeMultiplier;
        span = sqrtSvg("sqrtMain", spanHeight, viewBoxHeight, options);
+        span.style.minWidth = "0.781em";
        span.surdWidth = 0.833 * sizeMultiplier;   // from the font.


Why is minWidth different from surdWidth?

It's not the best choice of variable names. For the CSS minWidth, I used the actual width of the ink in the glyph. What I have called surdwidth is actually the horizontal advance value from the font. Horizontal advance value is of course, a little wider than the ink.

I might adjust the surdwidth name and re-commit.

I will also adjust the values of min-width. The paths in these surds have the xMin value baked into the SVG paths. So I'll increase each min-width by that amount.

kevinbarabash · 2017-09-18T03:00:19Z

src/delimiter.js

        span.surdWidth = 1.0 / sizeMultiplier; // from the font

    } else {
        // Tall sqrt. In TeX, this would be stacked using multiple glyphs.
        // We'll use a single SVG to accomplish the same thing.
        spanHeight = height / sizeMultiplier;
-        viewBoxHeight = Math.floor(1000 * spanHeight);
+        viewBoxHeight = Math.floor(1000 * height);


This fixes the issue with the tall \sqrt being the wrong size when sizing functions are involved?

kevinbarabash · 2017-09-18T03:03:34Z

src/delimiter.js

@@ -329,14 +329,12 @@ const sqrtSvg = function(sqrtName, height, viewBoxHeight, options) {
    }
    const pathNode = new domTree.pathNode(sqrtName, alternate);

-    let attributes = [["width", "100%"], ["height", height + "em"]];
+    const attributes = [["width", "400em"], ["height", height + "em"]];


Why does this have to be 400em as opposed to 100%?

I'll give 100% a try, but I don't think it will work. I'm now defining the viewBox dimensions in the same SVG as the width attribute. The height has a factor of 1000, so I think the width needs consistent treatment.

I was wrong. It does work to use width='100%'. Not only that but when width='100%', I can remove the CSS oveflow:hidden and it still works. So consider that done.

I wonder if that will cause sub-pixel changes in all the screenshots.

I retract my last statement. When I use width='100%, IE does not slice off the long tail. I going back to width='400em' and CSS overflow:hidden.

Final word. It turns out that it doesn't matter if I write width='400em' or width='100%'. It would matter, except that IE will not work unless I include CSS overflow:hidden. Once I include the CSS condition, both widths work. I'm going with 400em.

kevinbarabash · 2017-09-18T23:14:29Z

src/delimiter.js

-        span.style.minWidth = "0.781em";
-        span.surdWidth = 0.833 * sizeMultiplier;   // from the font.
+        span.style.minWidth = "0.853em";
+        span.advanceWidth = 0.833 * sizeMultiplier;   // from the font.


This is clearer. Thank you.

kevinbarabash

LGTM

kevinbarabash · 2017-09-18T23:37:08Z

@ronkok thanks for the PR.

ronkok added 2 commits September 17, 2017 10:16

Fix \sqrt{}

506a0a8

Even if the function argument is empty, still render an SVG whose width equals the surd glyph.

ronkok changed the title ~~Upgrade \sqrt~~ Upgrade \sqrt zoom and width Sep 17, 2017

Fix tall \sqrt when scaled

81aa124

kevinbarabash and others added 2 commits September 17, 2017 15:46

update screenshots affected by sqrt fixes

d650009

Merge pull request #1 from Khan/sqrt_fixes_screenshots

0f7d41e

update screenshots affected by sqrt fixes

kevinbarabash and others added 2 commits September 17, 2017 21:48

more screenshots after changes to fix sqrt

7b2089e

Merge pull request #2 from Khan/fix_sqrt_screenshots_2

f553ff1

more screenshots after changes to fix sqrt

ronkok mentioned this pull request Sep 18, 2017

\sqrt doesn't scale properly when browser is zoomed in or out on Safari #883

Closed

kevinbarabash reviewed Sep 18, 2017

View reviewed changes

ronkok added 2 commits September 18, 2017 11:57

Pick up review comments

35c9b9f

Merge branch 'master' of https://github.com/ronkok/KaTeX

e1a9233

kevinbarabash added the GH Review: review-needed label Sep 18, 2017

kevinbarabash reviewed Sep 18, 2017

View reviewed changes

kevinbarabash approved these changes Sep 18, 2017

View reviewed changes

kevinbarabash merged commit fd45669 into KaTeX:master Sep 18, 2017

kevinbarabash removed the GH Review: review-needed label Sep 18, 2017

ronkok mentioned this pull request Sep 29, 2017

Convert nested SVGs to single-level SVGs #909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade \sqrt zoom and width #890

Upgrade \sqrt zoom and width #890

ronkok commented Sep 17, 2017 •

edited

Loading

ronkok commented Sep 17, 2017

ronkok commented Sep 17, 2017 •

edited

Loading

kevinbarabash commented Sep 17, 2017

kevinbarabash left a comment

kevinbarabash Sep 18, 2017

ronkok Sep 18, 2017

kevinbarabash Sep 18, 2017 •

edited

Loading

kevinbarabash Sep 18, 2017

ronkok Sep 18, 2017

kevinbarabash Sep 18, 2017

ronkok Sep 19, 2017

edemaine Sep 19, 2017

ronkok Sep 19, 2017

kevinbarabash Sep 18, 2017

ronkok Sep 18, 2017

ronkok Sep 18, 2017

kevinbarabash Sep 18, 2017

ronkok Sep 18, 2017

kevinbarabash Sep 18, 2017

ronkok Sep 18, 2017

ronkok Sep 18, 2017

ronkok Sep 18, 2017

ronkok Sep 18, 2017

kevinbarabash Sep 18, 2017

kevinbarabash left a comment

kevinbarabash commented Sep 18, 2017

Upgrade \sqrt zoom and width #890

Upgrade \sqrt zoom and width #890

Conversation

ronkok commented Sep 17, 2017 • edited Loading

ronkok commented Sep 17, 2017

ronkok commented Sep 17, 2017 • edited Loading

kevinbarabash commented Sep 17, 2017

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash Sep 18, 2017 • edited Loading

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

kevinbarabash left a comment

Choose a reason for hiding this comment

kevinbarabash commented Sep 18, 2017

ronkok commented Sep 17, 2017 •

edited

Loading

ronkok commented Sep 17, 2017 •

edited

Loading

kevinbarabash Sep 18, 2017 •

edited

Loading