-
Notifications
You must be signed in to change notification settings - Fork 320
KaTeX (1.75/n): Support horizontal offset and pre-req for vertical offsets #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
Changes from all commits
5677317
b38301f
bd353ef
e8e8f41
0ec1641
85f4ecf
1fe817c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | |||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -822,41 +822,55 @@ class MathBlock extends StatelessWidget { | ||||||||||||||||||||||
children: [TextSpan(text: node.texSource)]))); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
return _Katex(inline: false, nodes: nodes); | |||||||||||||||||||||||
return Center( | |||||||||||||||||||||||
child: Directionality( | |||||||||||||||||||||||
textDirection: TextDirection.ltr, | |||||||||||||||||||||||
child: SingleChildScrollViewWithScrollbar( | |||||||||||||||||||||||
scrollDirection: Axis.horizontal, | |||||||||||||||||||||||
child: KatexWidget( | |||||||||||||||||||||||
textStyle: ContentTheme.of(context).textStylePlainParagraph, | |||||||||||||||||||||||
nodes: nodes)))); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
// Base text style from .katex class in katex.scss : | |||||||||||||||||||||||
// https://github.com/KaTeX/KaTeX/blob/613c3da8/src/styles/katex.scss#L13-L15 | |||||||||||||||||||||||
const kBaseKatexTextStyle = TextStyle( | |||||||||||||||||||||||
fontSize: kBaseFontSize * 1.21, | |||||||||||||||||||||||
fontFamily: 'KaTeX_Main', | |||||||||||||||||||||||
height: 1.2); | |||||||||||||||||||||||
/// Creates a base text style for rendering KaTeX content. | |||||||||||||||||||||||
/// | |||||||||||||||||||||||
/// This applies the CSS styles defined in .katex class in katex.scss : | |||||||||||||||||||||||
/// https://github.com/KaTeX/KaTeX/blob/613c3da8/src/styles/katex.scss#L13-L15 | |||||||||||||||||||||||
/// | |||||||||||||||||||||||
/// Requires the [style.fontSize] to be non-null. | |||||||||||||||||||||||
TextStyle mkBaseKatexTextStyle(TextStyle style) { | |||||||||||||||||||||||
return style.copyWith( | |||||||||||||||||||||||
fontSize: style.fontSize! * 1.21, | |||||||||||||||||||||||
fontFamily: 'KaTeX_Main', | |||||||||||||||||||||||
height: 1.2, | |||||||||||||||||||||||
fontWeight: FontWeight.normal, | |||||||||||||||||||||||
fontStyle: FontStyle.normal, | |||||||||||||||||||||||
textBaseline: TextBaseline.alphabetic, | |||||||||||||||||||||||
leadingDistribution: TextLeadingDistribution.even, | |||||||||||||||||||||||
decoration: TextDecoration.none, | |||||||||||||||||||||||
fontFamilyFallback: const []); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
class _Katex extends StatelessWidget { | |||||||||||||||||||||||
const _Katex({ | |||||||||||||||||||||||
required this.inline, | |||||||||||||||||||||||
@visibleForTesting | |||||||||||||||||||||||
class KatexWidget extends StatelessWidget { | |||||||||||||||||||||||
const KatexWidget({ | |||||||||||||||||||||||
super.key, | |||||||||||||||||||||||
required this.textStyle, | |||||||||||||||||||||||
required this.nodes, | |||||||||||||||||||||||
}); | |||||||||||||||||||||||
|
|||||||||||||||||||||||
final bool inline; | |||||||||||||||||||||||
final TextStyle textStyle; | |||||||||||||||||||||||
final List<KatexNode> nodes; | |||||||||||||||||||||||
|
|||||||||||||||||||||||
@override | |||||||||||||||||||||||
Widget build(BuildContext context) { | |||||||||||||||||||||||
Widget widget = _KatexNodeList(nodes: nodes); | |||||||||||||||||||||||
|
|||||||||||||||||||||||
if (!inline) { | |||||||||||||||||||||||
widget = Center( | |||||||||||||||||||||||
child: SingleChildScrollViewWithScrollbar( | |||||||||||||||||||||||
scrollDirection: Axis.horizontal, | |||||||||||||||||||||||
child: widget)); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
return Directionality( | |||||||||||||||||||||||
textDirection: TextDirection.ltr, | |||||||||||||||||||||||
child: DefaultTextStyle( | |||||||||||||||||||||||
style: kBaseKatexTextStyle.copyWith( | |||||||||||||||||||||||
style: mkBaseKatexTextStyle(textStyle).copyWith( | |||||||||||||||||||||||
color: ContentTheme.of(context).textStylePlainParagraph.color), | |||||||||||||||||||||||
child: widget)); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
@@ -874,9 +888,16 @@ class _KatexNodeList extends StatelessWidget { | ||||||||||||||||||||||
return WidgetSpan( | |||||||||||||||||||||||
alignment: PlaceholderAlignment.baseline, | |||||||||||||||||||||||
baseline: TextBaseline.alphabetic, | |||||||||||||||||||||||
child: switch (e) { | |||||||||||||||||||||||
KatexSpanNode() => _KatexSpan(e), | |||||||||||||||||||||||
}); | |||||||||||||||||||||||
// Work around a bug where text inside a WidgetSpan could be scaled | |||||||||||||||||||||||
// multiple times incorrectly, if the system font scale is larger | |||||||||||||||||||||||
// than 1x. | |||||||||||||||||||||||
// See: https://github.com/flutter/flutter/issues/126962 | |||||||||||||||||||||||
child: MediaQuery( | |||||||||||||||||||||||
data: MediaQueryData(textScaler: TextScaler.noScaling), | |||||||||||||||||||||||
Comment on lines
+895
to
+896
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: (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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this workaround does indeed fix the bug in most cases, but there is still some wierdness on Android. Here's the test message I'd used before to test this, but with all the different font scale settings on iOS and Android.
The table labels indicate the steps of font scaling available on the system, where 1 is default. As can be seen that this workaround works correctly on iOS. But on Android it looks like the font is not playing well after the second step for some reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh — very interesting that it's different between the two platforms. Please go ahead and file a follow-up issue, then, for the fact that font scaling doesn't interact correctly with our KaTeX support. That table of screenshots would be great to include there. Then this workaround can get a comment pointing at that issue, to give the reader more context. |
|||||||||||||||||||||||
child: switch (e) { | |||||||||||||||||||||||
KatexSpanNode() => _KatexSpan(e), | |||||||||||||||||||||||
KatexStrutNode() => _KatexStrut(e), | |||||||||||||||||||||||
})); | |||||||||||||||||||||||
})))); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
@@ -888,7 +909,7 @@ class _KatexSpan extends StatelessWidget { | ||||||||||||||||||||||
|
|||||||||||||||||||||||
@override | |||||||||||||||||||||||
Widget build(BuildContext context) { | |||||||||||||||||||||||
final em = DefaultTextStyle.of(context).style.fontSize!; | |||||||||||||||||||||||
var em = DefaultTextStyle.of(context).style.fontSize!; | |||||||||||||||||||||||
|
|||||||||||||||||||||||
Widget widget = const SizedBox.shrink(); | |||||||||||||||||||||||
if (node.text != null) { | |||||||||||||||||||||||
|
@@ -898,12 +919,18 @@ class _KatexSpan extends StatelessWidget { | ||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
final styles = node.styles; | |||||||||||||||||||||||
// We expect vertical-align to be only present with the | |||||||||||||||||||||||
// `strut` span, for which parser explicitly emits `KatexStrutNode`. | |||||||||||||||||||||||
// So, this should always be null for non `strut` spans. | |||||||||||||||||||||||
assert(styles.verticalAlignEm == null); | |||||||||||||||||||||||
|
|||||||||||||||||||||||
final fontFamily = styles.fontFamily; | |||||||||||||||||||||||
final fontSize = switch (styles.fontSizeEm) { | |||||||||||||||||||||||
double fontSizeEm => fontSizeEm * em, | |||||||||||||||||||||||
null => null, | |||||||||||||||||||||||
}; | |||||||||||||||||||||||
if (fontSize != null) em = fontSize; | |||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, this is a good idea for making sure we don't forget |
|||||||||||||||||||||||
|
|||||||||||||||||||||||
final fontWeight = switch (styles.fontWeight) { | |||||||||||||||||||||||
KatexSpanFontWeight.bold => FontWeight.bold, | |||||||||||||||||||||||
null => null, | |||||||||||||||||||||||
|
@@ -948,11 +975,52 @@ class _KatexSpan extends StatelessWidget { | ||||||||||||||||||||||
child: widget); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
return SizedBox( | |||||||||||||||||||||||
widget = SizedBox( | |||||||||||||||||||||||
height: styles.heightEm != null | |||||||||||||||||||||||
? styles.heightEm! * (fontSize ?? em) | |||||||||||||||||||||||
? styles.heightEm! * em | |||||||||||||||||||||||
: null, | |||||||||||||||||||||||
child: widget); | |||||||||||||||||||||||
|
|||||||||||||||||||||||
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), | |||||||||||||||||||||||
}; | |||||||||||||||||||||||
|
|||||||||||||||||||||||
if (margin != null) { | |||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: put the |
|||||||||||||||||||||||
assert(margin.isNonNegative); | |||||||||||||||||||||||
widget = Padding(padding: margin, child: widget); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
return widget; | |||||||||||||||||||||||
} | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
class _KatexStrut extends StatelessWidget { | |||||||||||||||||||||||
const _KatexStrut(this.node); | |||||||||||||||||||||||
|
|||||||||||||||||||||||
final KatexStrutNode node; | |||||||||||||||||||||||
|
|||||||||||||||||||||||
@override | |||||||||||||||||||||||
Widget build(BuildContext context) { | |||||||||||||||||||||||
final em = DefaultTextStyle.of(context).style.fontSize!; | |||||||||||||||||||||||
|
|||||||||||||||||||||||
final verticalAlignEm = node.verticalAlignEm; | |||||||||||||||||||||||
if (verticalAlignEm == null) { | |||||||||||||||||||||||
return SizedBox(height: node.heightEm * em); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
return SizedBox( | |||||||||||||||||||||||
height: node.heightEm * em, | |||||||||||||||||||||||
child: Baseline( | |||||||||||||||||||||||
baseline: (verticalAlignEm + node.heightEm) * em, | |||||||||||||||||||||||
baselineType: TextBaseline.alphabetic, | |||||||||||||||||||||||
child: const Text('')), | |||||||||||||||||||||||
Comment on lines
+1019
to
+1022
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we write a test for this? What's an example of an expression that would look wrong if we left out this Baseline widget; and what would be the effect on how it looks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The (new) offset/size widget test for If the Baseline widget is left out, the Katex content height would be larger (and incorrect) and the characters won't be on the correct alignment and may overflow the message container. For example:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, that'll do, then. I see this commit now includes a change to that test's expectations, reflecting that effect: Can you adjust the style of the test either before or after that commit so that the test isn't also changing style at the same time? That way the change to the expectations will stand out and make clear that the commit is effectively testing this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One way to do it that works before this commit and remains clean after it might be with const testCases = <(ContentExample, List<(String, Offset, Size)>, {bool? skip})>[
(ContentExample.mathBlockKatexSizing, skip: false, [
('1', Offset(0.00, 2.24), Size(25.59, 61.00)),
// … Then this commit would change a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
|||||||||||||||||||||||
); | |||||||||||||||||||||||
} | |||||||||||||||||||||||
} | |||||||||||||||||||||||
|
|||||||||||||||||||||||
|
@@ -1274,7 +1342,7 @@ class _InlineContentBuilder { | ||||||||||||||||||||||
: WidgetSpan( | |||||||||||||||||||||||
alignment: PlaceholderAlignment.baseline, | |||||||||||||||||||||||
baseline: TextBaseline.alphabetic, | |||||||||||||||||||||||
child: _Katex(inline: true, nodes: nodes)); | |||||||||||||||||||||||
child: KatexWidget(textStyle: widget.style, nodes: nodes)); | |||||||||||||||||||||||
|
|||||||||||||||||||||||
case GlobalTimeNode(): | |||||||||||||||||||||||
return WidgetSpan(alignment: PlaceholderAlignment.middle, | |||||||||||||||||||||||
|
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 one is an actual bug, right? (Not just potential.) The changes in those vertical offsets demonstrate it was having a real effect — things were sitting a little too high, and also not quite aligned correctly with each other.
It is a small bug, though: at least in the test data, the differences are all less than one logical pixel (so up to a couple of physical pixels).
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 one I guess is "potential" because I'm not sure whether the two baselines end up having a different effect with the fonts that KaTeX uses.