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

Fixed a spacing issue involving sizing commands and operators #136

Closed
wants to merge 4 commits into from

Conversation

kevinbarabash
Copy link
Member

This addresses issue #30.

TODO:

  • rebase
  • add screenshotter tests

@xymostech
Copy link
Contributor

This doesn't really fix the general problem, just a specific case of it. For example, something like 1 + \Huge x still doesn't work. (Perhaps I should have been more clear in my issue, it was mainly meant as a reminder to myself :P)

@kevinbarabash
Copy link
Member Author

I investigate that scenario as well but it looked like it was doing the right thing. There's an mord space to the left the "x". I tried creating an empty mbin as a sibling before the "x", but it added too much space (compared to MathJax's output).

screen shot 2014-09-29 at 3 58 42 pm

This screenshot compares 1 + \Huge x without and then with the extra mbin.
comparison

What would you like me to do?

@xymostech
Copy link
Contributor

Okay, sorry. That was a bad example, because it does what you want, but not for the reason you want. Here's a better example:

{)\Huge x}{)\Huge +x}

In MathJax, it (correctly) puts space to the left of the +, and none next to the x:

image

This doesn't, because the interaction here is a close-bin interaction:

image

I'm not sure what the solution to this is. It might be something like "put an empty element of the previous type inside". I'm also not sure whether the want the space to be modified by the \Huge (like it is now), or to be of regular size? Obviously, things completely outside of \Huge should have non-\Huge spaces, and things completely in \Huge should have \Huge spaces, but I'm not sure about the stuff along the border. Also, we need to make sure that if the space at the beginning is \Huge, then the space at the end is also \Huge, or vice versa.

@kevinbarabash
Copy link
Member Author

I looked at the MathJax rendering of this scenario and variations on it. It appears that if there is a space between something \Huge and something non-\Huge then the space is smaller. In order to fix this, I got rid of the empty mord and instead set the <span> containing the \Huge to be a mbin instead of an mord.

screen shot 2014-09-30 at 1 45 26 pm

Here's MathJax's rendering for comparison:
screen shot 2014-09-30 at 1 48 22 pm

I also tried {)\Huge x}{)\Huge +}x because I wanted see what spacing would like on the other side of the operator. Here are screenshots of that in KaTeX (with the fix) and MathJax.
screen shot 2014-09-30 at 1 46 34 pm

screen shot 2014-09-30 at 1 46 49 pm

…r span to be a "mbin" only if the first child is an "mbin"
expect("\\Huge x + 1").toBuild();
});
});

describe("A text parser", function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I took the other tests out because the string I would've needed to match would've been quite long making the test prone to failure from unrelated changes. I was thinking about using jQuery in the tests to verify the structure of some of the markup that's being produced. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this isn't something that should be tested with code, but should probably use a Huxley test instead. Generally, it's not helpful to verify that the code we write produces the structure we tell it to produce, because the test will just ensure that we produce the code tells it to, and not that the produced output is useful. The Huxley tests ensure that the code we write produces something that looks correct.

Did you ever get huxley tests to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't spent much time looking at them. I did install docker though. I'll work on getting them running today. Are you happy with the current solution? If so, I'll create some Huxley tests.

@xymostech
Copy link
Contributor

How are you testing MathJax? If you go here, you should get something that uses the same fonts as KaTeX does, so it's easier to tell the difference.

prev.type === "close") {
type = "mbin";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really just turning in to special casing for all of the different interactions, which is annoying to deal with (and there are a whole lot that aren't captured here). As @spicyj points out, MathJax doesn't actually do spacing with \Huge correctly, and we're not entirely sure how we want to do spacing overall, so maybe we should put this on hold until we can discuss what we want exactly to happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Line 778 is totally wrong. The intent is right, but the implementation is wrong. Ugh. Fix coming shortly then I'll leave it there while we discuss what to do.

@kevinbarabash
Copy link
Member Author

I was using http://www.mathjax.org/demos/scaling-math/. I tried the fiddle, but it's using the same fonts as the MathJax site.

kohler added a commit to kohler/KaTeX that referenced this pull request Nov 23, 2016
This is so the size commands don't hide the types of their enclosed
atoms. Addresses KaTeX#136.

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.
kohler added a commit to kohler/KaTeX that referenced this pull request Nov 23, 2016
This is so the size commands don't hide the types of their enclosed
atoms. Addresses KaTeX#136.

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.
kohler added a commit to kohler/KaTeX that referenced this pull request Nov 26, 2016
This is so the size commands don't hide the types of their enclosed
atoms. Addresses KaTeX#136.

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.
kohler added a commit to kohler/KaTeX that referenced this pull request Nov 26, 2016
This is so the size commands don't hide the types of their enclosed
atoms. Addresses KaTeX#136.

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.
@kohler kohler mentioned this pull request Nov 26, 2016
kohler added a commit to kohler/KaTeX that referenced this pull request Nov 27, 2016
This is so the size commands don't hide the types of their enclosed
atoms. Addresses KaTeX#136.

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.
kohler added a commit to kohler/KaTeX that referenced this pull request Nov 28, 2016
This is so the size commands don't hide the types of their enclosed
atoms. Addresses KaTeX#136.

This slightly changes the vertical position of the Sizing test. Not
sure the vertical position matters, so change the test.
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.

None yet

2 participants