-
Notifications
You must be signed in to change notification settings - Fork 324
KaTeX(5/n): Handle 'position' & 'top' property in KaTeX span inline style #1627
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
d060034
to
1b2a740
Compare
1b2a740
to
71ae7f2
Compare
1e20472
to
e545476
Compare
Thanks for building this! See my comments on #1670 for a few things I'd like to see on all the open KaTeX PRs: #1670 (review), #1670 (comment) . |
7fc2368
to
f7edad5
Compare
f7edad5
to
31381d6
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! Generally this all looks good — various small comments below.
Would you also post screenshots of an example where there's a bit more going on? With these screenshots, if we just ignored top
then it'd be hard to tell from the screenshots that anything was wrong, because there's nothing next to the \int
that it's getting aligned with. So even \int dx
would help a lot.
It also looks like the value of top
in this example is quite small:
top:-0.0011em;
And in fact, that's small enough that it defeats the widget test in this PR. If I delete the logic in _KatexSpan.build
that consumes styles.topEm
, then the tests all still pass, because 1.1 milli-ems is a very small fraction of a pixel and is below the epsilon the test (appropriately) uses.
Can you find an example where the effect is bigger? Try other big operators, like \sum
; or limits above or below the operators, like \int_0^1
.
lib/model/katex.dart
Outdated
@@ -765,6 +766,34 @@ class _KatexParser { | |||
_hasError = true; | |||
return null; | |||
} | |||
|
|||
/// Remove the given property from the given style map, | |||
/// and parse as a literal value of CSS position attribute. |
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.
/// and parse as a literal value of CSS position attribute. | |
/// and parse as a CSS position value. |
Here's the relevant spec and MDN doc:
https://drafts.csswg.org/css-position/#position-property
https://developer.mozilla.org/en-US/docs/Web/CSS/position
Scanning those:
- I don't think "literal" adds anything to the meaning here.
- The CSS terminology is that
position
and things like it are "properties", not "attributes".
lib/model/katex.dart
Outdated
// Currently, we expect `top` to be inside a vlist (which we handle it | ||
// separately above), and if it's along with a `position: relative`. | ||
if (styles.topEm != null && styles.position != KatexSpanPosition.relative) { | ||
throw _KatexHtmlParseError('unsupported inline CSS property "top", when "position: ${styles.position}"'); |
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 comment doesn't parse for me.
I think the vlist part is beside the point now: we do expect a top
property, but only when accompanied by position: relative
.
And just saying that isn't useful as a comment, because it repeats what the code in the condition already says. The thing that is useful to say in a comment, though, is why. So:
// Currently, we expect `top` to be inside a vlist (which we handle it | |
// separately above), and if it's along with a `position: relative`. | |
if (styles.topEm != null && styles.position != KatexSpanPosition.relative) { | |
throw _KatexHtmlParseError('unsupported inline CSS property "top", when "position: ${styles.position}"'); | |
if (styles.topEm != null && styles.position != KatexSpanPosition.relative) { | |
// The meaning of `top` would be different without `position: relative`. | |
throw _KatexHtmlParseError( | |
'unsupported inline CSS property "top" given "position: ${styles.position}"'); |
I.e.,
The meaning of
top
would be different withoutposition: relative
.
As MDN says:
https://developer.mozilla.org/en-US/docs/Web/CSS/top
The effect of top depends on how the element is positioned (i.e., the value of the position property):
[… and then a list of 4 quite different meanings for the 5 possible values ofposition
.]
@@ -97,10 +97,6 @@ class _KatexSpan extends StatelessWidget { | |||
|
|||
final styles = node.styles; | |||
|
|||
// Currently, we expect `top` to be only present with the | |||
// vlist inner row span, and parser handles that explicitly. | |||
assert(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.
This assert still seems quite relevant — but only in the case where position
is null.
lib/widgets/katex.dart
Outdated
}; | ||
|
||
if (offset != 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.
nit: these are closely related; keep joined as one stanza
}; | |
if (offset != null) { | |
}; | |
if (offset != null) { |
lib/widgets/katex.dart
Outdated
final offset = switch (styles.topEm) { | ||
final topEm? => Offset(0, topEm * em), | ||
null => null, | ||
}; | ||
|
||
if (offset != null) { | ||
widget = Transform.translate( | ||
offset: offset, |
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.
Though actually this seems like it can be tightened further than that: check if styles.topEm
is non-null, and if so then do the Transform.translate. It's fine to have a !
null-assertion inside the body of that conditional — it's easy for the reader to see why the assertion will always hold.
- …, 31380 of them were KaTeX containing messages and 3817 of those failed.
- There were 1075 math block nodes out of which 614 failed.
- There were 164014 math inline nodes out of which 6470 failed.
- Because of hard fail: unsupported inline CSS property: top:
- 1546 messages failed.
- Because of unsupported inline css property: position:
- 1356 messages failed.
+ …, 31380 of them were KaTeX containing messages and 2801 of those failed. A very nice improvement! Will be glad to have this in. |
f2775c9
to
b5b7f80
Compare
Thanks for the review @gnprice! Pushed an update, PTAL. I wasn't able to find an example with big operators having a larger |
55b0136
to
24fb254
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! Small comments below.
In the new screenshots, there's one thing that puzzles me, though it's probably independent of this PR: in the \colonequals
message the (horizontal) spacing between the colon and the equals-sign seems to be noticeably wider in Flutter than on web:
As a follow-up, can you try to diagnose why that is? (I took a quick look at the various CSS class names in katex.scss and didn't find any obvious candidates.)
test/model/katex_test.dart
Outdated
static final bigOperators = KatexExample.block( | ||
r'big operators: \int', | ||
// https://chat.zulip.org/#narrow/channel/7-test-here/topic/Rajesh/near/2240766 | ||
r'\int dx', |
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.
Ah, I wasn't clear enough — for making this example more complex, I really did mean just the screenshots 🙂. In the test code, it's nice to keep it as simple as possible. (And this test doesn't really gain anything from having the additional elements.)
test/widgets/katex_test.dart
Outdated
(KatexExample.bigOperators, skip: false, [ | ||
('∫', Offset(0.00, 12.02), Size(11.43, 25.00)), | ||
('d', Offset(24.00, 12.03), Size(10.69, 25.00)), | ||
('x', Offset(34.70, 12.03), Size(11.76, 25.00)), | ||
]), |
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.
Maybe even leave this one out of this widget test, since the test isn't sensitive enough to pick up whether the code is working.
Allowing support for handling KaTeX HTML for big operators. Fixes: zulip#1671
24fb254
to
ee3a00b
Compare
Survey results
Fixes: #1671