-
Notifications
You must be signed in to change notification settings - Fork 315
KaTeX (2/n): Support horizontal and vertical offsets for spans #1452
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
d00e10f
to
7ff4f9e
Compare
7ff4f9e
to
c0dd0a1
Compare
sealed class KatexNode extends ContentNode { | ||
const KatexNode({super.debugHtmlNode}); | ||
} | ||
|
||
class KatexSpanNode extends KatexNode { |
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.
One point that I noticed while developing #1478 (and checking how my draft of that branch interacted with this PR): it'd be good for this commit:
e268041 content: Handle vertical offset spans in KaTeX content
to get split up like so:
- First, an NFC prep commit introduces the distinction between KatexNode and KatexSpanNode. At this stage, the latter is the only subclass of the former.
- Then another commit makes the substantive changes this commit is about, including introducing the sibling subclasses KatexVlistNode and KatexVlistRowNode.
One reason that'd be useful is that the split between KatexNode and KatexSpanNode is somewhat nontrivial in itself: some of the references to KatexNode continue to say KatexNode, while others switch to saying KatexSpanNode, so the commit is expressing meaningful information by its choices of which references go which way.
b17033a
to
16cb28f
Compare
16cb28f
to
376cfe7
Compare
It seems like this had slipped through the cracks :-) but it looks ready for review, so I applied the label. Also rebased now that #1478 is merged, so this now contains only the changes that are specific to this PR. |
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 for the PR! I think the last few commits will need some tests for the more complicated parts. I went over the changes and left some comments, but haven't extensively tested the changes manually yet.
lib/model/katex.dart
Outdated
_logError('KaTeX: Unsupported CSS property: $property of ' | ||
'type ${expression.runtimeType}'); |
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.
One other possibility for this to be logged is when _getEm
returns null. I think for debugging purpose, including the value expression
might be helpful.
if (expression is css_visitor.EmTerm && expression.value is num) { | ||
return (expression.value as num).toDouble(); | ||
} | ||
return null; |
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 perhaps have a short dartdoc on what this does and what null
means, since the if (…Em != null) continue;
's make this return value quite relevant.
lib/model/katex.dart
Outdated
final double? heightEm; | ||
final double? marginRightEm; | ||
final double? topEm; | ||
final double? verticalAlignEm; |
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 it's a good idea to separate them into groups; to add on this, how about having a short comment before each group explaining how we group them together (like we do with design variables)?
lib/widgets/content.dart
Outdated
} | ||
|
||
return Container( | ||
margin: styles.marginRightEm != null && !styles.marginRightEm!.isNegative |
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.
Hmm, !styles.marginRightEm!.isNegative
seems a bit contrived. I think isNegative
negated is not as clear as >= 0
. However, do we need the margin when marginRightEm
is 0?
styles: inlineStyles != null | ||
? styles.merge(inlineStyles) | ||
: styles, |
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.
Having tests for this (the case when there are both inline styles and others) might be useful. Not sure how common that is.
lib/widgets/content.dart
Outdated
child: RichText(text: TextSpan( | ||
children: List.unmodifiable(row.nodes.map((e) { | ||
return WidgetSpan( | ||
alignment: PlaceholderAlignment.baseline, | ||
baseline: TextBaseline.alphabetic, | ||
child: switch (e) { | ||
KatexSpanNode() => _KatexSpan(e), | ||
KatexVlistNode() => _KatexVlist(e), | ||
KatexNegativeMarginNode() => _KatexNegativeMargin(e), | ||
}); | ||
}))))); |
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.
looks like this can be replaced with _KatexNodeList(nodes: row.nodes)
lib/widgets/content.dart
Outdated
child: Text.rich(TextSpan( | ||
children: List.unmodifiable(node.nodes.map((e) { | ||
return WidgetSpan( | ||
alignment: PlaceholderAlignment.baseline, | ||
baseline: TextBaseline.alphabetic, | ||
child: switch (e) { | ||
KatexSpanNode() => _KatexSpan(e), | ||
KatexVlistNode() => _KatexVlist(e), | ||
KatexNegativeMarginNode() => _KatexNegativeMargin(e), | ||
}); | ||
}))))); |
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.
_KatexNodeList
also seems helpful here.
lib/widgets/content.dart
Outdated
} | ||
|
||
class _KatexNegativeMargin extends StatelessWidget { | ||
const _KatexNegativeMargin(this.node); |
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.
Because the name implies that the margin should be negative, maybe we should also add assertions to ensure that that is true.
lib/model/katex.dart
Outdated
List<KatexNode> _parseChildSpans(dom.Element element) { | ||
return List.unmodifiable(element.nodes.map((node) { | ||
if (node case dom.Element(localName: 'span')) { | ||
return _parseSpan(node); | ||
} else { | ||
var resultSpans = <KatexNode>[]; | ||
for (final node in element.nodes.reversed) { | ||
if (node is! dom.Element || node.localName != 'span') { | ||
throw KatexHtmlParseError(); | ||
} | ||
})); | ||
|
||
final span = _parseSpan(node); | ||
resultSpans.add(span); | ||
|
||
if (span is KatexSpanNode) { | ||
final marginRightEm = span.styles.marginRightEm; | ||
if (marginRightEm != null && marginRightEm.isNegative) { | ||
final previousSpansReversed = | ||
resultSpans.reversed.toList(growable: false); | ||
resultSpans = []; | ||
resultSpans.add(KatexNegativeMarginNode( | ||
marginRightEm: marginRightEm, | ||
nodes: previousSpansReversed)); | ||
} | ||
} | ||
} | ||
|
||
return resultSpans.reversed.toList(growable: false); | ||
} |
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.
Neat solution! A bit tricky to understand. I think we can use QueueList
(and with .addFirst
) to avoid the hassle of reversing and unreversing the list.
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.
It will be helpful to have tests for this.
lib/model/katex.dart
Outdated
final pstrutStyles = _parseSpanInlineStyles(pstrutSpan)!; | ||
final pstrutHeight = pstrutStyles.heightEm ?? 0; | ||
|
||
// TODO handle negative right-margin inline style on row nodes. |
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.
Do we consider this complete after the final commit ("content: Support negative right-margin on KaTeX spans")? I'm not too sure if something else is left to be done here.
This seems to contradict with the earlier assumption that vlist elements contain only the height
style.
376cfe7
to
d879646
Compare
49d724d
to
c917d14
Compare
c917d14
to
ec14306
Compare
Thanks for the review @PIG208! Pushed a new revision, PTAL. I removed the commit for supporting negative margins from this PR, and will create a separate PR that introduces it again. |
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! Left some comments.
While testing this, I ran into some issues with text scaling (the relevant commit is "content: Scale inline KaTeX content based on the surrounding text"); posted screenshots in one of the comments below.
Otherwise, this works pretty well!
lib/model/katex.dart
Outdated
? List<String>.unmodifiable(element.className.split(' ')) | ||
: const <String>[]; |
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.
nit: indentation
(ContentExample.mathBlockKatexVertical5, [ | ||
('a', Offset(0.0, 4.16), Size(10.88, 25.0)), | ||
('b', Offset(10.88, -0.65), Size(8.82, 25.0)), | ||
('c', Offset(19.70, 4.16), Size(8.90, 25.0)), |
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.
Interesting setup! This resembles the golden tests that use images instead of offset/size values to verify rendering results.
test/widgets/content_test.dart
Outdated
Future.value(fontFile.readAsBytesSync().buffer.asByteData()); | ||
await (FontLoader(fontName)..addFont(bytes)).load(); | ||
} | ||
} |
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.
nit: newline at the end
@@ -1406,3 +1475,21 @@ void main() { | |||
}); | |||
}); | |||
} | |||
|
|||
Future<void> _loadKatexFonts() async { |
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.
We have a similar thing in emoji_reactoins_test.dart
:
Future<void> prepare() async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
await store.addUser(eg.selfUser);
// TODO do this more centrally, or put in reusable helper
final Future<ByteData> font = rootBundle.load('assets/Source_Sans_3/SourceSans3VF-Upright.otf');
final fontLoader = FontLoader('Source Sans 3')..addFont(font);
await fontLoader.load();
}
Sharing the helper is not necessary for this PR, but it looks like something we can extract to reuse in the future.
lib/model/katex.dart
Outdated
break; | ||
|
||
// TODO handle skipped class declarations between .mspace and | ||
// .thinbox . |
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.
nit: should this be .msupsub?
@@ -850,8 +860,7 @@ class _Katex extends StatelessWidget { | |||
return Directionality( | |||
textDirection: TextDirection.ltr, | |||
child: DefaultTextStyle( | |||
style: kBaseKatexTextStyle.copyWith( | |||
color: ContentTheme.of(context).textStylePlainParagraph.color), | |||
style: mkBaseKatexTextStyle(textStyle), |
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.
@@ -850,8 +860,7 @@ class _Katex extends StatelessWidget { | |||
return Directionality( | |||
textDirection: TextDirection.ltr, | |||
child: DefaultTextStyle( | |||
style: kBaseKatexTextStyle.copyWith( | |||
color: ContentTheme.of(context).textStylePlainParagraph.color), | |||
style: mkBaseKatexTextStyle(textStyle), |
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/model/katex.dart
Outdated
rows.add(KatexVlistRowNode( | ||
verticalOffsetEm: topEm + pstrutHeight, | ||
node: KatexSpanNode( | ||
styles: styles, | ||
text: null, | ||
nodes: _parseChildSpans(otherSpans)))); |
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.
nit: indentation
lib/widgets/content.dart
Outdated
return widget; | ||
|
||
if (styles.verticalAlignEm != null && styles.heightEm != null) { | ||
assert(widget == defaultWidget); |
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 assertion is based on the broader assumption that some inline styles never co-occur to vertical-align
or height
. This seems like something we can comment on.
ec14306
to
0e00af6
Compare
|
||
fontSize = 4.976 * fontSize; | ||
checkKatexText(tester, '2', | ||
fontFamily: 'KaTeX_Main', | ||
fontSize: fontSize, | ||
fontHeight: kBaseKatexTextStyle.height!); | ||
fontHeight: baseTextStyle.height!); | ||
}); | ||
|
||
testWidgets('displays KaTeX content with different delimiter sizing', (tester) async { |
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.
+ // TODO: Re-enable this test after adding support for parsing
+ // `vertical-align` in inline styles. Currently it fails
+ // because `strut` span has `vertical-align`.
+ //
+ // testWidgets('displays KaTeX content with different delimiter sizing', (tester) async {
Instead of commenting out the whole test (which makes the diff big), pass skip: true
. For examples, see git grep -A2 skip: test/
.
The other reason skip:
is better than commenting out is that it makes the test case still exist in the test framework and get counted; the total number of skipped tests is printed after the number of passed and failed tests. That's helpful for occasionally sweeping to confirm we don't have tests accidentally left skipped that might point to live bugs.
styles: KatexSpanStyles(fontSizeEm: 0.5), // .reset-size6.size1 | ||
text: '0', | ||
nodes: null), | ||
]), | ||
]), | ||
]); | ||
|
||
static final mathBlockKatexNestedSizing = ContentExample( | ||
static const mathBlockKatexNestedSizing = ContentExample( |
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.
+ // TODO: Re-enable this test after adding support for parsing
+ // `vertical-align` in inline styles. Currently it fails
+ // because `strut` span has `vertical-align`.
+ //
+ // static const mathBlockKatexDelimSizing = ContentExample(
Similarly, use skip: true
instead of commenting.
For this test, that requires a bit of setup to make this file's helpers and the "all content examples are tested" check handle the skip:
argument. I've just pushed a small added commit at the end of this PR branch to do that:
f9a79d8 content test [nfc]: Enable skips in testParseExample and testParse
So please rebase that to earlier in the branch as needed, and then write:
testParseExample(ContentExample.mathBlockKatexNestedSizing, skip: true);
f9a79d8
to
e509efe
Compare
This is ready for review again @chrisbobbe, PTAL. |
0a66a8d
to
13d3101
Compare
To simplify managing this series of changes, I'd like to get the parts of it merged soon that are easier to merge, while we follow up with reviews on the rest. @rajveermalviya would you try reordering this branch so the commits that are refactors or otherwise simple come first, and the more complex commits come at the end? On a quick scan, I think that means these 9 commits first: d9b1be2 content test [nfc]: Use const for math block tests and so these 6 later: 3e80a0d content: Scale inline KaTeX content based on the surrounding text Then I guess send the first segment as a separate "1.5/n" PR, so it can be merged first. |
This will prevent string interpolation being evaluated during release build. Especially useful in later commit where it becomes more expensive.
And rename previous type to KatexSpanNode, also while making it a subtype of KatexNode.
And inline the behaviour for `inline: false` in MathBlock widget.
13d3101
to
36f73dd
Compare
This applies the correct font scaling if the KaTeX content is inside a header.
In KaTeX HTML it is used to set the baseline of the content in a span, so handle it separately here.
Implement handling most common types of `vlist` spans.
36f73dd
to
2c5a28c
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.
OK, in addition to the commits from #1609 (reviewed there), I've now read the first 4/6 of these commits:
0434863 content: Scale inline KaTeX content based on the surrounding text
d81008f content: Handle 'strut' span in KaTeX content
532b324 content: Handle positive margin-right and margin-left in KaTeX spans
fef8a60 content: Handle 'top' property in KaTeX span inline style
Comments below.
With that, left for another time are the last two commits, about vlists:
2c5a28c content: Handle vertical offset spans in KaTeX content
7d76da7 content: Error message for unexpected CSS class in vlist inner span
child: MediaQuery( | ||
data: MediaQueryData(textScaler: TextScaler.noScaling), |
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.
On the face of it this looks like it's doing something wrong: causing the system text-size setting to have no effect on math expressions.
This bit of code is borrowed from my suggestion at #1452 (comment). I think it's likely correct — there's a bug (same as at #735) causing that system setting to get applied repeatedly, compounding, and this is to prevent that. But it definitely looks wrong on its face, so it calls for an explanation, probably in a code comment.
Have you tried out how this looks with that system setting applied? Does it end up correctly applying the setting once, rather than zero times or multiple times?
Can this be done as a separate commit before the rest of this commit? The commit isn't obviously about the same thing:
0434863 content: Scale inline KaTeX content based on the surrounding text
(BTW I didn't actually realize until just now, as I got into reviewing this PR closely, that you'd done something to address that issue Zixuan discovered in that previous thread. I'd assumed we still had in the recent releases that issue where the system font scaling was being applied repeatedly in math expressions, and had just lucked out in that nobody had complained yet. For something complex like this it's a good idea to reply in the GitHub subthread when you apply a suggested fix; the suggestion was untested and therefore tentative, so I was figuring you'd report back your findings on whether it turned out to work.)
child: _Katex(inline: true, nodes: nodes)); | ||
child: Katex(textStyle: widget.style, nodes: nodes)); |
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.
0434863 content: Scale inline KaTeX content based on the surrounding text
Let's have a test case for this, too — one that would fail if we re-introduced the bug Zixuan found at #1452 (comment).
if (styles.filter(heightEm: false, verticalAlignEm: false) != | ||
KatexSpanStyles()) { |
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.
nit: binary operator goes after newline, not before (so it doesn't get visually lost off at the ragged right edge of the code):
if (styles.filter(heightEm: false, verticalAlignEm: false) != | |
KatexSpanStyles()) { | |
if (styles.filter(heightEm: false, verticalAlignEm: false) | |
!= KatexSpanStyles()) { |
if (styles.filter(heightEm: false, verticalAlignEm: false) != | ||
KatexSpanStyles()) { |
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.
This will allocate two fresh KatexSpanStyles objects, with all their fields, in order to check that the other fields of styles
are all null.
We can eliminate one of those allocations easily by saying const KatexSpanStyles()
.
The other one is from filter
. Let's leave that for now, I guess; it's probably not a big deal in practice.
(Definitely it's good to be doing this check, excluding unanticipated shapes of the tree.)
return KatexStrutNode( | ||
heightEm: heightEm, | ||
verticalAlignEm: verticalAlignEm); |
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.
nit: should get debugHtmlNode
@@ -941,7 +953,80 @@ class _KatexSpan extends StatelessWidget { | |||
textAlign: textAlign, | |||
child: widget); | |||
} | |||
return widget; | |||
|
|||
final marginRight = switch (styles.marginRightEm) { |
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 believe we're assuming here (in _KatexSpan.build) that styles.verticalAlignEm
will always be null.
That assumption is meant to be guaranteed by the parser. But it's a very non-local fact — it's not something one can tell by looking at this code here. So let's make it explicit here, with an assert.
That way a reader of this code, if they go consult the list of fields on KatexSpanStyles to compare to what's handled here, doesn't think "wait, what about verticalAlignEm? looks like this has a bug that it's not handling that", and then spend a bunch of time trying to exercise that bug before discovering it can't be exercised.
That way also, if there's a bug where that situation can happen after all — for example because the parser is missing a spot where it should check verticalAlignEm
is null, like in the vlist code as commented above 🙂 — then we have a way to potentially notice that.
return Container( | ||
margin: margin, | ||
height: styles.heightEm != null | ||
? styles.heightEm! * em | ||
: null, | ||
transform: styles.topEm != null |
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.
It's not obvious to me how these three Container fields interact with each other. Let's use individual widgets for these different effects — that way the nesting is clear.
(This is a general problem with the API of Container. We don't have many use sites of it, and probably most of those would be improved by using individual widgets explicitly. I think its main value is for quick hacks and demos.)
final marginRight = switch (styles.marginRightEm) { | ||
double marginRightEm => marginRightEm * em, | ||
null => null, | ||
}; | ||
final marginLeft = switch (styles.marginLeftEm) { | ||
double marginLeftEm => marginLeftEm * em, | ||
null => null, | ||
}; | ||
|
||
EdgeInsets? margin; | ||
if (marginRight != null || marginLeft != null) { | ||
margin = EdgeInsets.zero; | ||
if (marginRight != null) { | ||
assert(marginRight >= 0); | ||
margin += EdgeInsets.only(right: marginRight); | ||
} | ||
if (marginLeft != null) { | ||
assert(marginLeft >= 0); | ||
margin += EdgeInsets.only(left: marginLeft); | ||
} | ||
} |
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 this equivalent?
final marginRight = switch (styles.marginRightEm) { | |
double marginRightEm => marginRightEm * em, | |
null => null, | |
}; | |
final marginLeft = switch (styles.marginLeftEm) { | |
double marginLeftEm => marginLeftEm * em, | |
null => null, | |
}; | |
EdgeInsets? margin; | |
if (marginRight != null || marginLeft != null) { | |
margin = EdgeInsets.zero; | |
if (marginRight != null) { | |
assert(marginRight >= 0); | |
margin += EdgeInsets.only(right: marginRight); | |
} | |
if (marginLeft != null) { | |
assert(marginLeft >= 0); | |
margin += EdgeInsets.only(left: marginLeft); | |
} | |
} | |
final margin = switch ((styles.marginLeftEm, styles.marginRightEm)) { | |
(null, null) => null, | |
(null, final marginRightEm) => EdgeInsets.only(right: marginRightEm! * em), | |
(final marginLeftEm, null) => EdgeInsets.only(left: marginLeftEm! * em), | |
(final marginLeftEm, final marginRightEm) => | |
EdgeInsets.only(left: marginLeftEm! * em, right: marginRightEm! * em), | |
}; |
margin = EdgeInsets.zero; | ||
if (marginRight != null) { | ||
assert(marginRight >= 0); | ||
margin += EdgeInsets.only(right: marginRight); | ||
} | ||
if (marginLeft != null) { | ||
assert(marginLeft >= 0); | ||
margin += EdgeInsets.only(left: marginLeft); |
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.
Can we write a test for this? (Same questions as for struts.)
transform: styles.topEm != null | ||
? Matrix4.translationValues(0, styles.topEm! * em, 0) | ||
: null, |
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.
Huh — so in particular it looks like top: 0
would have no effect.
Is that true in CSS that top: 0
has no effect? Are there assumptions we're making about the structure of the HTML, or about the other CSS around, that makes that true?
I'd like to see an example of the sort of content that this appears in. A parser example would be a good start. Then ultimately, like for struts and margins as mentioned in previous comments in this batch, it'd be good to have a widget test that checks that this logic is doing the right thing.
Stacked on top of: #1609
Related: #46