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

Handle adjusting cell heights properly. #1500 #1503

Merged
merged 1 commit into from
Jun 8, 2016
Merged

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Jun 6, 2016

The actual height and depth of the row is in state.H[i] and state.D[i], but we were using LH and LD, the usual line height and depth. This PR fixes that. This works if there is actual text in the cell, but if the cell is empty, the margin-bottom doesn't fall below the baseline, it sits on top of the baseline, which spoils the alignment. Also, if the height of the cell is less than the midpoint of the font, the browser adds extra height making it the midpoint of the font, and we didn't compensate for that.

Both issues are resolved by adding a 1em strut to the cell (so it has content and is not too short), and compensating for that in the height adjustments. (Since the font in question was the surrounding font, it was not easy to tell where its midpoint was; this solution is independent of the surrounding font.)

Resolves issue #1500.

…d issues with cells containing no text and cells that are shorter than the midpoint of the font. Resolves issue mathjax#1500.
@pkra pkra added this to the MathJax v2.x.x milestone Jun 6, 2016
@zorkow
Copy link
Member

zorkow commented Jun 8, 2016

lgtm.

@dpvc dpvc merged commit e715e1d into mathjax:develop Jun 8, 2016
@pkra
Copy link
Contributor

pkra commented Jun 9, 2016

@dpvc I have trouble with this in mathjax-node (using the develop branches of both repos); the result keeps having this problem. Does that sound like it could be another jsdom bug or should I better triple check my setup? 😦

@dpvc dpvc deleted the issue1500 branch June 24, 2016 00:51
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.

3 participants