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

Support highlight, overline, underline, and strike for math equations #3953

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

LuxxxLucy
Copy link
Contributor

@LuxxxLucy LuxxxLucy commented Apr 18, 2024

Regarding #2200, it turns out the highlight is supported for normal text but not for math equations. This PR implement the highlight for math equations.

Like highlight, other line-based decorations are supported now as well, including overline, underline, strikethrough. Result

image

(tests added in deco.typ)

Old

This is the second of two PR for solving #2200 (and shoud be easier to review if #3941 is merged first) (or Just merge this PR directly)

It continues on #3941 that add support for overline underline and strike for math equations. Some refactoring and corresponding tests are added as well.

crates/typst/src/math/row.rs Outdated Show resolved Hide resolved
}
last_frame_index = Some(index);
}
if let Some(index) = first_frame_index {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand what's going on here. Why are we only background-decorating the first and foreground-decorating the last MathParItem::Frame?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, wait, are you creating an overlarge decoration for all the items, but applying it to the first one? That seems problematic when the equation breaks over multiple lines. We should probably rather create individual decorations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Now each frame is decorated on its own.

Special case will be any empty space between two frames, in that case we need to fill the void and decorate the space as well. For such space in between frames, we use the last frame and append the decorations to it.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, interesting. I tried the following new test case, where the equation breaks over multiple lines:

--- highlight-inline-math-multiline ---
#line(length: 100%)
#h(1fr)
#highlight[$a + b + c + d + e + f + g$]

It results in this image.
highlight-inline-math-multiline

I think the trailing space after the "+" shouldn't be there. However, this is not a problem with your PR, but rather the math equation line breaking. However, if that bug was fixed, the solution of decorating the space together with the previous frame wouldn't work anymore, as the highlighting would continue into the margin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not figured out a good way of doing highlight for the trailing space. It seems that in order to do that we need to do decoration after the linebreak, which would require us to do the decoration in commit (just like the normal text case).

In order to do this, we need to associate the style to the frame (just like how a style is asscoited with Item::Text(shaped). It seems a little bit tricky and honestly I am not so sure this might be a good way.

Or another possibility is to just do not care about the space, which would create some void between two or more MathParItems).

BTW, what is this bug with math equation line breaking and how is it going to be solved?

Copy link
Member

Choose a reason for hiding this comment

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

The bug is that there is a space at the end of the first line in the picture above. I think the spacing should be "weak" and collapse at the end of the line, just like normal text spaces.

Copy link
Contributor Author

@LuxxxLucy LuxxxLucy May 7, 2024

Choose a reason for hiding this comment

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

The bug is that there is a space at the end of the first line in the picture above. I think the spacing should be "weak" and collapse at the end of the line, just like normal text spaces.

Sorry for the late update. While I have not figured out a good solution, I added a predecessor PR that eats white space introduced by the math equation in #4087 I think I will find a new way for the decoration

Copy link
Member

@laurmaedje laurmaedje May 7, 2024

Choose a reason for hiding this comment

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

It's really quite tricky. Ideally, we'd also not couple math and paragraph layout too tightly.

Maybe we should apply the highlighting completely in paragraph layout rather than math, on a per-line base (i.e. highlight consecutive stuff together with a larger bounding box, no matter whether it is text, math, h or whatever)? This could also fix #1716.

crates/typst/src/math/equation.rs Outdated Show resolved Hide resolved
crates/typst/src/math/equation.rs Outdated Show resolved Hide resolved
crates/typst/src/text/deco.rs Outdated Show resolved Hide resolved
crates/typst/src/text/deco.rs Outdated Show resolved Hide resolved
crates/typst/src/text/deco.rs Outdated Show resolved Hide resolved
@LuxxxLucy LuxxxLucy changed the title Support Line decoration for Math [2/2]: Support overline. underline and strike for math equation Support Line decoration for Math equations: Support highlight, overline. underline and strike for math equation Apr 19, 2024
@laurmaedje laurmaedje added the waiting-on-review This PR is waiting to be reviewed. label May 6, 2024
@laurmaedje laurmaedje removed the waiting-on-review This PR is waiting to be reviewed. label May 7, 2024
@laurmaedje laurmaedje marked this pull request as draft May 13, 2024 09:32
@laurmaedje laurmaedje changed the title Support Line decoration for Math equations: Support highlight, overline. underline and strike for math equation Support highlight, overline, underline, and strike for math equations May 13, 2024
@laurmaedje laurmaedje added the waiting-on-design This PR or issue is blocked by design work. label May 13, 2024
@LuxxxLucy LuxxxLucy force-pushed the lucy-decoration-refactor-br branch from bf92854 to 3446de0 Compare May 24, 2024 02:37
@freddyholms
Copy link

Can this format individual symbols inside math mode and keep them in the same font? For instance, the second example "unitalicizes" the $n(n+1)$ term.

@LuxxxLucy
Copy link
Contributor Author

LuxxxLucy commented May 27, 2024

Can this format individual symbols inside math mode and keep them in the same font? For instance, the second example "unitalicizes" the n(n+1) term.

@freddyholms Okay this is interesting, it seems that the n(n+1) (as long as it is wrapped by a []) is considered as normal text instead of math, that being said, this is a bug that is orthogonal to the changes made in this PR, which would have its own issue. Specifically, in the main branch (without the changes in this PR), if you try this sample

$ sum_(k=1)^n k = #highlight[(n(n+1))] / 3 $

it would already result in this

image

@LuxxxLucy
Copy link
Contributor Author

Okay I just picked up this PR again recently and have made a new solution (albeit being very complicated) but breaks many other tests, mainly because in equation.rs, the size of the equations' frame is updated manually to consider a "slack", which will be used in determining line height, but this also means the original size is lost so we cannot do the decoration based on the correct frame size. I will push the changes once I found a good solution.

@laurmaedje
Copy link
Member

I think the best solution is to reconsider highlighting from scratch and do it at the paragraph level. That way, for instance, adjacent text and math could also get a shared bounding box. If we want to go down that route, I'd like to wait with that until after I'm done with my layout refactoring though (or take a stab at it during it).

@LuxxxLucy
Copy link
Contributor Author

I think the best solution is to reconsider highlighting from scratch and do it at the paragraph level. That way, for instance, adjacent text and math could also get a shared bounding box. If we want to go down that route, I'd like to wait with that until after I'm done with my layout refactoring though (or take a stab at it during it).

Yes I am working towards this direction, a principled approach that handles text, math (and other frames) properly.

Now other than some refactoring ( am sure there will be some ways), one other thing that I am struggling with is the issue of slack adjust as in math/equation.rs

            ...
            let font_size = scaled_font_size(&ctx, styles);
            let slack = ParElem::leading_in(styles) * 0.7;
            let top_edge = TextElem::top_edge_in(styles).resolve(font_size, &font, None);
            let bottom_edge =
                -TextElem::bottom_edge_in(styles).resolve(font_size, &font, None);

            let ascent = top_edge.max(frame.ascent() - slack);
            let descent = bottom_edge.max(frame.descent() - slack);
            frame.translate(Point::with_y(ascent - frame.baseline()));
            frame.size_mut().y = ascent + descent;
            ...

so this slackness adjustment would modify in place of the frame height/width, so if we do decoration of the math frames in line commit (after line-break is done), the frame.size() does not truthfully corresponds to the box where we want to do highlight. It would be great if the line height adjust can keep the math frames as it is, and only do the line height adjustment in line commit.

Recently I discovered that #4236 seems to tackling the same issue, but it seems difficult to preserve and pass the existing tests...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-design This PR or issue is blocked by design work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants