-
Notifications
You must be signed in to change notification settings - Fork 319
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
base: main
Are you sure you want to change the base?
Conversation
@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. |
c33f5c2
to
6cfcc0d
Compare
843207d
to
d1f5d5c
Compare
There was a problem hiding this 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.
lib/widgets/katex.dart
Outdated
// Like [RenderPadding] but supports negative values. | ||
class RenderNegativePadding extends RenderShiftedBox { |
There was a problem hiding this comment.
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:
// 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 { |
lib/widgets/katex.dart
Outdated
extension on EdgeInsetsGeometry { | ||
bool get isNegative => !isNonNegative; |
There was a problem hiding this comment.
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.
92674c9
to
39d2937
Compare
39d2937
to
bf886a6
Compare
Thanks for the initial comments @gnprice. Revision pushed. |
bf886a6
to
e8cd58a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/widgets/content.dart
Outdated
final marginRight = switch (styles.marginRightEm) { | ||
double marginRightEm when marginRightEm >= 0 => marginRightEm * em, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
.
There was a problem hiding this comment.
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.
bd56117
to
dfabd4d
Compare
Thanks for the review @gnprice! Pushed an update, PTAL. |
dfabd4d
to
2bae0c2
Compare
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.
2bae0c2
to
2e60440
Compare
Stacked on top of #1698.
Related: #46