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

fix(math): Math font size must follow the document font size #2149

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Omikhleia
Copy link
Member

Closes #2145

Math formula in block quotes, footnotes, and even headers
shall use the current document font size, not some hard-coded
10pt default.
These tests use document font size at 11pt, but formulas would be
typeset at 10pt as the math.font.size default was hard-coded.
It now follows the current document font size, so enforce it back
to 10pt in order for the test to pass unchanged.
@Omikhleia Omikhleia added the bug Software bug issue label Oct 23, 2024
@Omikhleia Omikhleia added this to the v0.15.6 milestone Oct 23, 2024
@Omikhleia Omikhleia self-assigned this Oct 23, 2024
@Omikhleia Omikhleia requested review from a team and alerque as code owners October 23, 2024 21:18
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

This definitely needs fixing, but just hard coding a link to font.size seems too aggressive and might trip up in some cases.

@@ -42,9 +42,13 @@ function package.declareSettings (_)
})
SILE.settings:declare({
parameter = "math.font.size",
type = "integer",
default = 10,
type = "number or integer",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider whether we ought to allow relative values here such that formulas would track changes to the document font size in a relative way.

})
SILE.settings:registerHook ("font.size", function (size)
-- Follow document font size changes
SILE.settings:set("math.font.size", size)
Copy link
Member

Choose a reason for hiding this comment

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

If we do allow a relative value (e.g. 1.2em) then this would need to adjust to compute the difference (or just not change it at all, but we would have to review if we are using :absolute() where necessary) not just hard code a value.

Even if not, we probably want to figure out how to remove this hook if the user resets font.size after having set math.font.size at any point.

Copy link
Member Author

@Omikhleia Omikhleia Oct 29, 2024

Choose a reason for hiding this comment

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

BTW there's a whole lot of issues hiding behind math.font.size eventually.

Consider legit MathML <mpadded width="3em" lspace="0.7em">...</mpadded> = the unit is evaluated with respect to font.size but math.font.size might have been the intent... The MathML spec is not very precise here (it's even unclear whether the scale-down should be taken into account.) (N.B. Basic mpadded support is in-progress in #2151)

My guts feelings (after I hit the problem under a different angle in mpadded) is that math.font.size should always be in a font relative unit, and affect font.size inside the math equation.
Or should be wholly dimissed - after all we don't really do such things for verbatim/monospace fonts either, and the question of scaling appropriately different fonts is more general (= on which basis? em-size ? ex-height ? capital-height? etc.) - CSS has some good pointers here, for once.

Copy link
Member Author

Choose a reason for hiding this comment

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

The more I think about it, the more I tend to believe that math.font.size should be killed and we use just the current font size.
Let me know you feeling (whether we address it in 0.15.8, whether we don't, and what we foresee) -- So I can document it (or not) in an upcoming booklet.

Copy link
Member

Choose a reason for hiding this comment

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

The more I think about it, the more I tend to believe that math.font.size should be killed and we use just the current font size.

I don't see how that would work. We're automatically switching font faces for Math and it may or may not be from the same font family or appropriately sized to be matched with the body text. I don't see how we can get away with not allowing separate control over the size when we are changing the face.

Let me know you feeling (whether we address it in 0.15.8, whether we don't, and what we foresee) -- So I can document it (or not) in an upcoming booklet.

I'm happy to move this forward as soon as we agree on how it should be handled. I just don't want to push out a change that we know has just a different set of downside to the current one. If perchance the solution we come up with ends up being a significant breaking change that might make it more complicated and delay things as well, but as long as this is a setting issue we can probably shim it in the retrograde package and not worry about it some reflows being breaking.

Copy link
Member Author

@Omikhleia Omikhleia Dec 2, 2024

Choose a reason for hiding this comment

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

Noted!
I removed my assignment and shifted the original issue to 0.x.y -- It might be reconsidered indeed, but then that won't occur as part of my on-going 2024 effort on math which is soon reaching its end.

@Omikhleia Omikhleia marked this pull request as draft November 2, 2024 01:59
@alerque alerque modified the milestones: v0.15.6, v0.15.7 Nov 2, 2024
@alerque alerque modified the milestones: v0.15.7, v0.15.8 Nov 25, 2024
@Omikhleia Omikhleia removed this from the v0.15.8 milestone Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Software bug issue
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

Math font size does not follow the current normal text size
2 participants