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

Set maxFontSize on rules. #744

Merged
merged 4 commits into from
Jul 9, 2017
Merged

Set maxFontSize on rules. #744

merged 4 commits into from
Jul 9, 2017

Conversation

kohler
Copy link
Collaborator

@kohler kohler commented Jun 28, 2017

This is needed in vlists, as in #646, to make fontsize-ensurer
nodes big enough.

@ronkok
Copy link
Collaborator

ronkok commented Jun 29, 2017

Rules can be set to any arbitrary height. This should be tested with some really tall ones. I haven't got time to test this myself this myself at the moment, but when I was working on PR #670, I ran into trouble when I tried to accommodate tall images by a method similar to this.

If I recall correctly, makeVList applied the maxFontSize not only to the element that needed it, but to all the members of the vlist. I ended up in some cases with unexpectedly large spacing above the vlist. I.e., the vlist pushed the whole line downward on the screen and left a large gap above the math.

@ronkok
Copy link
Collaborator

ronkok commented Jun 29, 2017

For \fbox, my solution was to (1) set maxFontSize to 1 em because that did not distort the rest of the vlist, and (2) adjust the position of the fbox after running makeVList. The adjustment looks like:

if (img.height > vlist.maxFontSize) {
        // Correct for an issue in makeVList. It placed the image top at
        // the top of the line box created by a 1 em maxFontSize.
        vlist.children[1].style.top = -(inner.height + pad - 0.9 / scale)
            + "em";
        // The 0.9 in the previous line is there because the KaTeX fonts
        // have an ascent = 0.9 em. We're setting the top of the image
        // relative to the top of that line box.
    }

Just a suggestion.

@kohler
Copy link
Collaborator Author

kohler commented Jun 29, 2017

Hi @ronkok, that doesn't work when sizes change, even for \fbox. E.g. (on master):

image

Or (again on master) try \fbox{\rule{0.1em}{2em}}.

The fontsize-ensurer nodes are weird and suboptimal, but given the current CSS coding they appear necessary.

I think this commit should be merged (I've tested with very large rules—thanks!—and they work, although they do, as you suspected, introduce large gaps) and the vlist/fontsize-ensurer issues pushed to another project.

@ronkok
Copy link
Collaborator

ronkok commented Jun 29, 2017

@kohler, Thank you for pointing out the issue with a size change inside an fbox. It looks like I have more work to do. Sigh.

This is needed in vlists, as in KaTeX#646, to make fontsize-ensurer
nodes big enough.
@edemaine
Copy link
Member

Maybe it would be good to add some more test(s) to illustrate, such as the \fbox{\huge} example and large \rules?

@kohler
Copy link
Collaborator Author

kohler commented Jun 29, 2017

I'd like this PR to concentrate on bug #646, and there could be another PR or issue on issues surrounding fontsize-ensurer?

@kevinbarabash
Copy link
Member

@kohler could you post a screenshot of \frac{\textcolor{blue}{\rule{2em}{2em}}}{2} from the original issue?

@kevinbarabash
Copy link
Member

screen shot 2017-06-29 at 7 01 53 pm

Curious got the better of me. It'd be good to have this as a screenshot test as it's more obvious what the problem is in this example as compared to the existing screenshots that change.

One concern I have about adding more screenshot tests is that the tests and taking longer and longer to run. I hope that the new headless modes for Chrome and Firefox will make it easier to take screenshots.

@kohler
Copy link
Collaborator Author

kohler commented Jun 30, 2017

Screenshotter test added. Re: test speed, PR #752 speeds up screenshotter tests 6x

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding the additional screenshot test case.

@kevinbarabash kevinbarabash merged commit 782484e into KaTeX:master Jul 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants