Skip to content

KaTeX (3/n): Support negative margins for spans #1559

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented Jun 10, 2025

Stacked on top of #1698.

Related: #46

@gnprice
Copy link
Member

gnprice commented Jun 10, 2025

@rajveermalviya Copying here for visibility what you told me on our call today: the reason this is a draft (not ready to merge) is that you're still working on the widget test. Apart from that, you consider it ready to review.

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Jun 10, 2025
@rajveermalviya rajveermalviya marked this pull request as ready for review June 11, 2025 17:34
@rajveermalviya rajveermalviya force-pushed the pr-tex-content-3 branch 2 times, most recently from 843207d to d1f5d5c Compare June 11, 2025 19:19
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Here's small comments from a partial skim.

Comment on lines 39 to 41
// Like [RenderPadding] but supports negative values.
class RenderNegativePadding extends RenderShiftedBox {
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave a TODO tracking our intention to get a version of this upstream:

Suggested change
// Like [RenderPadding] but supports negative values.
class RenderNegativePadding extends RenderShiftedBox {
// Like [RenderPadding] but supports negative values.
// TODO(upstream): give Padding an option to accept negative padding (at cost of hit-testing not working)
class RenderNegativePadding extends RenderShiftedBox {

Comment on lines 200 to 201
extension on EdgeInsetsGeometry {
bool get isNegative => !isNonNegative;
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 this is clearer (and hardly much longer) if just inlined.

In particular there's some ambiguity in that "is negative" sounds like it might require all the sides to be negative. So being more transparent, with one fewer layer of indirection, helps mitigate that.

@rajveermalviya
Copy link
Member Author

Thanks for the initial comments @gnprice. Revision pushed.

@gnprice gnprice added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 4, 2025
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! I haven't yet re-reviewed this, as work on #1452 is still ongoing.

Would you rebase this atop the current #1452? That's helpful for me to be able to continue shipping them both in releases.

One comment below from when I tried a version of that rebase just now.

Comment on lines 961 to 962
final marginRight = switch (styles.marginRightEm) {
double marginRightEm when marginRightEm >= 0 => marginRightEm * em,
Copy link
Member

Choose a reason for hiding this comment

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

Is it intended to be possible to get to this point and have a negative marginRightEm?

I believe not — and it looks like if it did happen, this code would just ignore it, which seems like clearly a bug.

So instead of this "when" condition, let's have an assertion:

    assert((styles.marginLeftEm ?? 0) >= 0);

Similarly for right margin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated so that parser makes sure not to emit a node with negative margin styles, as it already emits a separate KatexNegativeMarginNode for handling that on the widget side. And widget side retains the assert(margin.isNonNegative);.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks — that definitely sounds cleaner.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-3 branch 2 times, most recently from bd56117 to dfabd4d Compare July 9, 2025 14:58
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

Implement handling most common types of `vlist` spans.
Negative margin spans on web render to the offset being applied
to the specific span and all the adjacent spans, so mimic the
same behaviour here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants