-
Notifications
You must be signed in to change notification settings - Fork 347
KaTeX overline underline border #1919
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
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 @MdSaifAliMolla! This will also need updates to the KaTeX widget tests, and also please update the description with screenshots comparing both web and mobile.
| case 'overline': | ||
| // .overline { border-top: 0.04em solid; } | ||
| borderStyle = KatexBorderStyle( | ||
| position: KatexBorderPosition.top, | ||
| widthEm: 0.04, | ||
| color: KatexSpanColor(0, 0, 0, 255), | ||
| ); | ||
|
|
||
| case 'underline': | ||
| // .underline { border-bottom: 0.04em solid; } | ||
| borderStyle = KatexBorderStyle( | ||
| position: KatexBorderPosition.bottom, | ||
| widthEm: 0.04, | ||
| color: KatexSpanColor(0, 0, 0, 255), | ||
| ); |
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.
These don't seem to match an existing class in katex.scss. Can please you provide the reference for these 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.
i didn't find any related class in katex.scss, but i found that the border-width is given 0.04em or 0.049em in some places.
Mostly did trial and error and whatever pleases the eye😔.
.angl { // from package actuarialangle, which is always used in a subscript. box-sizing: border-box; border-top: 0.049em solid; // defaultRuleThickness in scriptstyle border-right: 0.049em solid; // ditto margin-right: 0.03889em; // 1 mu }
also this
.fcolorbox { box-sizing: border-box; border: 0.04em solid; // \fboxrule = 0.4pt }
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, for TeX content in messages we would want to match the exact rendering of KaTeX on web and fallback to displaying the TeX source for cases that we don't handle.
For example, the relavent part in the rendered HTML in this test message #test here > Rajesh @ 💬 is:
…
<span class="underline-line" style="border-bottom-width:0.04em;"></span>
…So, we would want to handle the .underline-line class from katex.scss and also handle the border-bottom-width inline style separately.
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.
done
31d7ab8 to
4a99319
Compare
|
@rajveermalviya take a look. |
|
@rajveermalviya this needs a review. |
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.
Sorry for the delayed review @MdSaifAliMolla. Comments below.
Also, I can't reproduce the screenshot in the description:
Can you make sure forceRenderKatex flag in experimental features is off? The TeX content should render with that flag disabled, that flag is only useful for debugging where we want to ignore some KaTeX errors.
lib/widgets/katex.dart
Outdated
| class _BorderPainter extends CustomPainter { | ||
| _BorderPainter({ | ||
| required this.color, | ||
| required this.strokeWidth, | ||
| required this.isTop, | ||
| }); | ||
|
|
||
| final Color color; | ||
| final double strokeWidth; | ||
| final bool isTop; | ||
|
|
||
| @override | ||
| void paint(Canvas canvas, Size size) { | ||
| final paint = Paint() | ||
| ..color = color | ||
| ..strokeWidth = strokeWidth | ||
| ..style = PaintingStyle.stroke; | ||
| if (isTop) {canvas.drawLine(Offset(0, strokeWidth / 2 + 3),Offset(size.width, strokeWidth / 2 + 3),paint); | ||
| } else {canvas.drawLine(Offset(0, size.height - strokeWidth / 2 - 3),Offset(size.width, size.height - strokeWidth / 2 - 3),paint); | ||
| } | ||
| } | ||
| @override | ||
| bool shouldRepaint(_BorderPainter oldDelegate) { | ||
| return oldDelegate.color != color || oldDelegate.strokeWidth != strokeWidth ||oldDelegate.isTop != isTop; | ||
| }} |
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, is there a reason to use this instead of BoxDecoration with a border?
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 tried to reduce the gap between text and line, but it's not necessary. Removed it.
test/model/katex_test.dart
Outdated
| ]); | ||
|
|
||
| static final overline = KatexExample.block( | ||
| // https://chat.zulip.org/#narrow/channel/7-test-here/topic/Saif.20KaTeX/with/2278868 |
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.
The HTML and the TeX source doesn't match the linked message here. Please see the comment at the top of the file, on how to retrieve the HTML of a specific message.
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.
Also please use a simpler example for these tests, for example: #test here > Rajesh @ 💬
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.
fixed
test/model/katex_test.dart
Outdated
| ]); | ||
|
|
||
| static final underline = KatexExample.block( | ||
| // https://chat.zulip.org/#narrow/channel/7-test-here/topic/Saif.20KaTeX/with/2278868 |
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.
Same here, the HTML and TeX source doesn't match the linked message.
| case 'overline': | ||
| // .overline { border-top: 0.04em solid; } | ||
| borderStyle = KatexBorderStyle( | ||
| position: KatexBorderPosition.top, | ||
| widthEm: 0.04, | ||
| color: KatexSpanColor(0, 0, 0, 255), | ||
| ); | ||
|
|
||
| case 'underline': | ||
| // .underline { border-bottom: 0.04em solid; } | ||
| borderStyle = KatexBorderStyle( | ||
| position: KatexBorderPosition.bottom, | ||
| widthEm: 0.04, | ||
| color: KatexSpanColor(0, 0, 0, 255), | ||
| ); |
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, for TeX content in messages we would want to match the exact rendering of KaTeX on web and fallback to displaying the TeX source for cases that we don't handle.
For example, the relavent part in the rendered HTML in this test message #test here > Rajesh @ 💬 is:
…
<span class="underline-line" style="border-bottom-width:0.04em;"></span>
…So, we would want to handle the .underline-line class from katex.scss and also handle the border-bottom-width inline style separately.
345e4e7 to
e0645cf
Compare
e0645cf to
c6d8b3b
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 for the revision @MdSaifAliMolla. Comments below.
lib/model/katex.dart
Outdated
| case 'tag': | ||
| case 'eqn-num': | ||
| case 'mtable': | ||
| case 'col-align-l': | ||
| case 'col-align-c': | ||
| case 'col-align-r': | ||
| case 'delimcenter': | ||
| case 'accent': | ||
| case 'accent-body': | ||
| case 'vlist': | ||
| case 'vlist-r': | ||
| case 'vlist-s': | ||
| case 'svg-align': | ||
| case 'hide-tail': | ||
| case 'halfarrow-left': | ||
| case 'halfarrow-right': | ||
| case 'brace-left': | ||
| case 'brace-center': | ||
| case 'brace-right': | ||
| case 'root': | ||
| case 'sqrt': | ||
| case 'pstrut': | ||
| case 'arraycolsep': | ||
| case 'vertical-separator': | ||
| case 'frac-line': | ||
| case 'mfrac': |
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, can you explain the reasoning for ignoring all these classes? I see many here that we already handle in some way.
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.
sorry for lack of explanation.
with forceRenderKatex off, the katex code kept falling back to source code, so I ignored all possible warnings, but actually by just ignoring overline and underline case the fallback problem was averted. So these cases can be removed.
The reason of this:
case 'overline':
case 'underline':
break;is that \overline or \underline and \overline-line or \underline-line are parent-child constructs and by assigning borderStyle to both, the parser ends up with a redundant border on the outer wrapper, and that gives this error ---
Exception caught by widgets library ═══════════════════════════════════
'package:flutter/src/widgets/framework.dart': Failed assertion: line 6420 pos 14: '() {
// check that it really is our descendant
Element? ancestor = dependent._parent;
while (ancestor != this && ancestor != null) {
ancestor = ancestor._parent;
}
return ancestor == this;
}()': is not true.
ignoring those fixes the issue and also matches KaTeX’s expected DOM structure for \overline{AB}.
| case 'overline-line': | ||
| borderStyle = KatexBorderStyle( | ||
| position: KatexBorderPosition.bottom, | ||
| widthEm: 0.04, | ||
| ); | ||
| break; | ||
|
|
||
| case 'underline-line': | ||
| borderStyle = KatexBorderStyle( | ||
| position: KatexBorderPosition.bottom, | ||
| widthEm: 0.04, | ||
| ); |
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.
Please add the CSS source inline from katex.scss for these classes like other cases here.
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.
In katex.scss, I found this --
.overline .overline-line,
.underline .underline-line,
.hline {
display: inline-block;
width: 100%;
border-bottom-style: solid;
}so the I should add this, right?
// .overline-line { display: inline-block; width: 100%; border-bottom-style: solid; }
// Border applied via inline style: border-top-width: 0.04em;| case 'overline': | ||
| case 'underline': | ||
| break; |
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 there a reason these classes are ignored?
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.
comment #1919 (comment)
lib/widgets/katex.dart
Outdated
| final em = DefaultTextStyle.of(context).style.fontSize!; | ||
|
|
||
| return Stack(children: List.unmodifiable(node.rows.map((row) { | ||
| return IntrinsicWidth(child: Stack(children: List.unmodifiable(node.rows.map((row) { |
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 you explain the reasoning behind this change? If it is needed, then it should have it's own separate commit.
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.
In KatexWidget, it ensures the entire expression only takes as much horizontal space as needed and in vlist, it’s used to properly size vertically stacked elements like fractions, so widest child's width becomes the lines's width.
| Widget widget = _KatexNodeList(nodes: nodes); | ||
|
|
||
| return Directionality( | ||
| return IntrinsicWidth( |
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 you explain the reasoning behind this change? If it is needed, then it should have it's own separate commit.
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.
without IntrinsicWidth the line takes up width of minWidth: double.infinity, so it is used to restrict the width of over/underline. Also similar thing is already in use inside RenderNegativePadding like here -computeMaxIntrinsicWidth , computeMinIntrinsicHeight, and computeMaxIntrinsicHeight,
so I thought it is as per coding convention.
| if (styles.borderStyle case final borderStyle?) { | ||
| final currentColor = color ?? DefaultTextStyle.of(context).style.color!; | ||
| final Color borderColor = borderStyle.color != null | ||
| ? Color.fromARGB(borderStyle.color!.a, borderStyle.color!.r, borderStyle.color!.g, borderStyle.color!.b) | ||
| : currentColor; | ||
| final double borderWidth = borderStyle.widthEm * em; | ||
|
|
||
| return Container( | ||
| constraints: const BoxConstraints(minWidth: double.infinity), | ||
| height: borderWidth, | ||
| color: borderColor, | ||
| ); | ||
| } |
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.
Continued from #1919 (comment).
The return here ends up discarding the base widget, again is there a reason to not use BoxDecoration widget with a border.
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.
sorry about discarding base widget, this change should be fine I think
widget = Container(
constraints: const BoxConstraints(minWidth: double.infinity),
decoration: BoxDecoration(
border: Border(bottom: BorderSide(color: borderColor, width: borderWidth))),
child: widget,
);about BoxDecoration, I used it at first but the line was not appearing on screen, so I worked around to get a working solution. BoxConstraint method worked so I kept that. Now I figured that using BoxDecoration with BoxConstraints works too.
also BoxConstraints is already in use inside RenderNegativePadding class like in this computeDryLayout, computeDryBaseline etc places.
test/model/katex_test.dart
Outdated
| // any one platform is enough; avoid dealing with Windows file paths | ||
| ); | ||
| } | ||
| } |
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: the removal of the newline here is probably unintentional.
4c0d546 to
8768e51
Compare
8768e51 to
c2eda0c
Compare

closes #1673