Skip to content

Conversation

@edemaine
Copy link
Member

@edemaine edemaine commented Jul 16, 2019

Fix #1788 by removing display:inline-block rule from .sizing, as suggested by @ronkok. I don't see why a sizing change cares about the display mode, and it fixes the interaction between makeVList and sizing commands, and didn't affect any screenshots beyond three invisible changes specific to Chrome. It should also allow me to remove a .sizing { width:min-content } rule that I had to add in #1787.

Here's how the example from #1788 now renders (without #1787): all Bs properly descend.

A\raisebox{-1ex}{B}
\large A\raisebox{-1ex}{B}
\huge A\raisebox{-1ex}{B}
\large {A\raisebox{-1ex}{B}}
\huge {A\raisebox{-1ex}{B}}

image

Here are screenshot diffs. As there are no red or green pixels, the changes are invisible:

MathChoice-chrome-diff

ModScript-chrome-diff

Sizing-chrome-diff

Fix KaTeX#1788 by removing `display:inline-block` rule from `.sizing`.
I don't see why a sizing change cares about the display mode, and it
fixes the interaction between `makeVList` and sizing commands.
It should also allow me to remove a `.sizing { width:min-content }` rule
that I had to add in KaTeX#1787.
@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #2044 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2044   +/-   ##
=======================================
  Coverage   93.34%   93.34%           
=======================================
  Files          79       79           
  Lines        4992     4992           
  Branches      876      876           
=======================================
  Hits         4660     4660           
  Misses        291      291           
  Partials       41       41
Flag Coverage Δ
#screenshotter 89.07% <ø> (ø) ⬆️
#test 86.51% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45539c2...c55a20c. Read the comment docs.

Copy link
Collaborator

@ronkok ronkok left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I can't think of a reason why a font-size change would require inline-block. This line of code has been here since KaTeX v0.6.0. That predates the vertical alignment changes in PR #768. If I had to speculate why the inline-block is there, I might look at the old vertical alignment code.

In any case, the inline-block now is a positive harm to vertical alignment. It's good that we are getting rid of it.

@edemaine edemaine merged commit 403126e into KaTeX:master Jul 16, 2019
edemaine added a commit to edemaine/KaTeX that referenced this pull request Jul 16, 2019
@kevinbarabash
Copy link
Member

It's interesting that only the Chrome screenshots were affected. Different browsers different rendering engines I guess. 🤷‍♂️

kevinbarabash pushed a commit that referenced this pull request Jul 18, 2019
…TeX, \LaTeX, \KaTeX (#1787)

* Remove forced \normalsize in \raisebox

Fix #1786 (incorrect size of "E" in `\TeX`) by fixing `\raisebox` to
keep the current font size.  Previously, `\raisebox` was rendering its
second argument as if it were in `\normalsize\textrm{...}`; now,
the argument is just as if it were in `\textrm{...}`.

* Fix screenshots

* New "hbox" argument type to convert box-like arguments

* Review comments

* Fix \fbox to use hbox argument

* Render A in \LaTeX and \KaTeX in \scriptstyle (not \scriptsize)

* Add screenshot test

* Revert .sizing { width: min-content} thanks to #2044
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.

makeVList fails immediately in sizing group

4 participants