-
Notifications
You must be signed in to change notification settings - Fork 319
KaTeX (2/n): Support for vertical offsets in spans #1698
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
16bec6a
967a498
74b08db
c4503b4
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 |
---|---|---|
|
@@ -209,11 +209,122 @@ class _KatexParser { | |
debugHtmlNode: debugHtmlNode); | ||
} | ||
|
||
if (element.className == 'vlist-t' | ||
|| element.className == 'vlist-t vlist-t2') { | ||
final vlistT = element; | ||
if (vlistT.nodes.isEmpty) throw _KatexHtmlParseError(); | ||
if (vlistT.attributes.containsKey('style')) throw _KatexHtmlParseError(); | ||
|
||
final hasTwoVlistR = vlistT.className == 'vlist-t vlist-t2'; | ||
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. There's also a CSS rule for .vlist-t2 in the KaTeX CSS: .vlist-t2 {
margin-right: -2px;
} What's the net effect of that? On its face it looks like something we should have to take account of. 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. This will need support for negative margins, so will add a commit in the next PR for this. (It will also need changes for supporting non- 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, sounds good. Please file a brief follow-up issue for that, then; and then this code can have a TODO comment mentioning that issue. |
||
if (!hasTwoVlistR && vlistT.nodes.length != 1) throw _KatexHtmlParseError(); | ||
|
||
if (hasTwoVlistR) { | ||
if (vlistT.nodes case [ | ||
_, | ||
dom.Element(localName: 'span', className: 'vlist-r', nodes: [ | ||
dom.Element(localName: 'span', className: 'vlist', nodes: [ | ||
dom.Element(localName: 'span', className: '', nodes: []), | ||
]) && final vlist, | ||
]), | ||
]) { | ||
// In the generated HTML the .vlist in second .vlist-r span will have | ||
// a "height" inline style which we ignore, because it doesn't seem | ||
// to have any effect in rendering on the web. | ||
// But also make sure there aren't any other inline styles present. | ||
final vlistStyles = _parseSpanInlineStyles(vlist); | ||
if (vlistStyles != null | ||
&& vlistStyles.filter(heightEm: false) != const KatexSpanStyles()) { | ||
throw _KatexHtmlParseError(); | ||
} | ||
} else { | ||
throw _KatexHtmlParseError(); | ||
} | ||
} | ||
|
||
if (vlistT.nodes.first | ||
case dom.Element(localName: 'span', className: 'vlist-r') && | ||
final vlistR) { | ||
if (vlistR.attributes.containsKey('style')) throw _KatexHtmlParseError(); | ||
|
||
if (vlistR.nodes.first | ||
case dom.Element(localName: 'span', className: 'vlist') && | ||
final vlist) { | ||
// Same as above for the second .vlist-r span, .vlist span in first | ||
// .vlist-r span will have "height" inline style which we ignore, | ||
// because it doesn't seem to have any effect in rendering on | ||
// the web. | ||
// But also make sure there aren't any other inline styles present. | ||
final vlistStyles = _parseSpanInlineStyles(vlist); | ||
if (vlistStyles != null | ||
&& vlistStyles.filter(heightEm: false) != const KatexSpanStyles()) { | ||
throw _KatexHtmlParseError(); | ||
} | ||
|
||
final rows = <KatexVlistRowNode>[]; | ||
|
||
for (final innerSpan in vlist.nodes) { | ||
Comment on lines
+263
to
+265
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. Hmm, what's meant by calling these "rows"? In CSS terms, the element that's a "row" here is the 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. IIRC, I called them "rows" because they seem like rows in a column with different vertical offset. 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. OK, we can go with that. |
||
if (innerSpan case dom.Element( | ||
localName: 'span', | ||
nodes: [ | ||
dom.Element(localName: 'span', className: 'pstrut') && | ||
final pstrutSpan, | ||
...final otherSpans, | ||
], | ||
)) { | ||
if (innerSpan.className != '') { | ||
throw _KatexHtmlParseError('unexpected CSS class for ' | ||
'vlist inner span: ${innerSpan.className}'); | ||
} | ||
Comment on lines
+274
to
+277
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. I guess this points at a different, sometimes-applicable downside of if-case syntax (other than the fact that it puts the failure case way at the bottom on the other side of the main code, which we can hope to someday have a "guard let" feature to address): it doesn't make room for emitting specific errors for specific types of failures to match. |
||
|
||
var styles = _parseSpanInlineStyles(innerSpan); | ||
if (styles == null) throw _KatexHtmlParseError(); | ||
if (styles.verticalAlignEm != null) throw _KatexHtmlParseError(); | ||
final topEm = styles.topEm ?? 0; | ||
|
||
styles = styles.filter(topEm: false); | ||
|
||
final pstrutStyles = _parseSpanInlineStyles(pstrutSpan); | ||
if (pstrutStyles == null) throw _KatexHtmlParseError(); | ||
if (pstrutStyles.filter(heightEm: false) | ||
!= const KatexSpanStyles()) { | ||
throw _KatexHtmlParseError(); | ||
} | ||
final pstrutHeight = pstrutStyles.heightEm ?? 0; | ||
|
||
rows.add(KatexVlistRowNode( | ||
verticalOffsetEm: topEm + pstrutHeight, | ||
debugHtmlNode: kDebugMode ? innerSpan : null, | ||
node: KatexSpanNode( | ||
styles: styles, | ||
text: null, | ||
nodes: _parseChildSpans(otherSpans)))); | ||
} else { | ||
throw _KatexHtmlParseError(); | ||
} | ||
} | ||
|
||
// TODO(#1716) Handle styling for .vlist-t2 spans | ||
return KatexVlistNode( | ||
rows: rows, | ||
debugHtmlNode: debugHtmlNode, | ||
); | ||
} else { | ||
throw _KatexHtmlParseError(); | ||
} | ||
} else { | ||
throw _KatexHtmlParseError(); | ||
} | ||
} | ||
|
||
final inlineStyles = _parseSpanInlineStyles(element); | ||
if (inlineStyles != null) { | ||
// We expect `vertical-align` inline style to be only present on a | ||
// `strut` span, for which we emit `KatexStrutNode` separately. | ||
if (inlineStyles.verticalAlignEm != null) throw _KatexHtmlParseError(); | ||
|
||
// Currently, we expect `top` to only be inside a vlist, and | ||
// we handle that case separately above. | ||
if (inlineStyles.topEm != null) throw _KatexHtmlParseError(); | ||
} | ||
|
||
// Aggregate the CSS styles that apply, in the same order as the CSS | ||
|
@@ -224,7 +335,9 @@ class _KatexParser { | |
// https://github.com/KaTeX/KaTeX/blob/2fe1941b/src/styles/katex.scss | ||
// A copy of class definition (where possible) is accompanied in a comment | ||
// with each case statement to keep track of updates. | ||
final spanClasses = List<String>.unmodifiable(element.className.split(' ')); | ||
final spanClasses = element.className != '' | ||
? List<String>.unmodifiable(element.className.split(' ')) | ||
: const <String>[]; | ||
String? fontFamily; | ||
double? fontSizeEm; | ||
KatexSpanFontWeight? fontWeight; | ||
|
@@ -492,6 +605,7 @@ class _KatexParser { | |
if (stylesheet.topLevels case [css_visitor.RuleSet() && final rule]) { | ||
double? heightEm; | ||
double? verticalAlignEm; | ||
double? topEm; | ||
double? marginRightEm; | ||
double? marginLeftEm; | ||
|
||
|
@@ -510,6 +624,10 @@ class _KatexParser { | |
verticalAlignEm = _getEm(expression); | ||
if (verticalAlignEm != null) continue; | ||
|
||
case 'top': | ||
topEm = _getEm(expression); | ||
if (topEm != null) continue; | ||
|
||
case 'margin-right': | ||
marginRightEm = _getEm(expression); | ||
if (marginRightEm != null) { | ||
|
@@ -537,6 +655,7 @@ class _KatexParser { | |
|
||
return KatexSpanStyles( | ||
heightEm: heightEm, | ||
topEm: topEm, | ||
verticalAlignEm: verticalAlignEm, | ||
marginRightEm: marginRightEm, | ||
marginLeftEm: marginLeftEm, | ||
|
@@ -578,6 +697,8 @@ class KatexSpanStyles { | |
final double? heightEm; | ||
final double? verticalAlignEm; | ||
|
||
final double? topEm; | ||
|
||
final double? marginRightEm; | ||
final double? marginLeftEm; | ||
|
||
|
@@ -590,6 +711,7 @@ class KatexSpanStyles { | |
const KatexSpanStyles({ | ||
this.heightEm, | ||
this.verticalAlignEm, | ||
this.topEm, | ||
this.marginRightEm, | ||
this.marginLeftEm, | ||
this.fontFamily, | ||
|
@@ -604,6 +726,7 @@ class KatexSpanStyles { | |
'KatexSpanStyles', | ||
heightEm, | ||
verticalAlignEm, | ||
topEm, | ||
marginRightEm, | ||
marginLeftEm, | ||
fontFamily, | ||
|
@@ -618,6 +741,7 @@ class KatexSpanStyles { | |
return other is KatexSpanStyles && | ||
other.heightEm == heightEm && | ||
other.verticalAlignEm == verticalAlignEm && | ||
other.topEm == topEm && | ||
other.marginRightEm == marginRightEm && | ||
other.marginLeftEm == marginLeftEm && | ||
other.fontFamily == fontFamily && | ||
|
@@ -632,6 +756,7 @@ class KatexSpanStyles { | |
final args = <String>[]; | ||
if (heightEm != null) args.add('heightEm: $heightEm'); | ||
if (verticalAlignEm != null) args.add('verticalAlignEm: $verticalAlignEm'); | ||
if (topEm != null) args.add('topEm: $topEm'); | ||
if (marginRightEm != null) args.add('marginRightEm: $marginRightEm'); | ||
if (marginLeftEm != null) args.add('marginLeftEm: $marginLeftEm'); | ||
if (fontFamily != null) args.add('fontFamily: $fontFamily'); | ||
|
@@ -653,6 +778,7 @@ class KatexSpanStyles { | |
return KatexSpanStyles( | ||
heightEm: other.heightEm ?? heightEm, | ||
verticalAlignEm: other.verticalAlignEm ?? verticalAlignEm, | ||
topEm: other.topEm ?? topEm, | ||
marginRightEm: other.marginRightEm ?? marginRightEm, | ||
marginLeftEm: other.marginLeftEm ?? marginLeftEm, | ||
fontFamily: other.fontFamily ?? fontFamily, | ||
|
@@ -666,6 +792,7 @@ class KatexSpanStyles { | |
KatexSpanStyles filter({ | ||
bool heightEm = true, | ||
bool verticalAlignEm = true, | ||
bool topEm = true, | ||
bool marginRightEm = true, | ||
bool marginLeftEm = true, | ||
bool fontFamily = true, | ||
|
@@ -677,6 +804,7 @@ class KatexSpanStyles { | |
return KatexSpanStyles( | ||
heightEm: heightEm ? this.heightEm : null, | ||
verticalAlignEm: verticalAlignEm ? this.verticalAlignEm : null, | ||
topEm: topEm ? this.topEm : null, | ||
marginRightEm: marginRightEm ? this.marginRightEm : null, | ||
marginLeftEm: marginLeftEm ? this.marginLeftEm : null, | ||
fontFamily: fontFamily ? this.fontFamily : 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.
Hmm indeed, good to catch this!
This meant it wasn't appearing in either the actual or expected tree, so it just got missed.
I went and scanned through all the other node classes in this file, and I think this is the only such omission.