-
Notifications
You must be signed in to change notification settings - Fork 308
content: Handle KaTeX math, with a rough preview #472
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
Conversation
I assume we want to resolve this CZO thread before I review this. (edit: as discussed in the office, I'll read the code; we don't expect the visual design changes to change much of it.) |
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! Here's a review from reading the code and some manual testing.
lib/model/content.dart
Outdated
if (greatgrand.localName != 'semantics') return null; | ||
|
||
if (greatgrand.nodes.isEmpty) return null; | ||
final child4 = greatgrand.nodes.last; |
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 order to name the last child child4
, do we want to assert that there are four children? (er, or 5 children, if we count from 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.
Hmm, that's not the meaning I intended with this name. Definitely could use some changes to make it clearer, then!
I don't expect a fixed number of children here, of the <semantics>
element. In particular if you look at your example below, or at the example in the test cases, it has two children — an <mrow>
and then the <annotation>
— but I figure it's probably not always an <mrow>
, and may not always be just one sibling before the <annotation>
.
Maybe a better name would be descendant4
. It's the child of greatgrand
, which is the great-grandchild of katexElement
, the third generation down from katexElement
; so this one is the fourth generation down.
The first name I started typing for it was greatgreat
, but I figured that looks too similar to greatgrand
. Could also say greatgreatgrand
, I guess — or maybe greatGrand
and greatGreatGrand
would be the proper camel-casing.
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 a better name would be
descendant4
.
Sure, that sounds good.
lib/model/content.dart
Outdated
@@ -792,6 +894,16 @@ class _ZulipContentParser { | |||
} | |||
|
|||
if (localName == 'p' && classes.isEmpty) { | |||
// Oddly, the way a math block gets encoded in Zulip HTML is inside a <p>. | |||
if (element.nodes case [dom.Element(localName: 'span') && var child]) { |
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, I'm seeing a block-quoted math block with more than one child inside the <p>
. The HTML has the expected span and then <br>\n
.
https://chat.zulip.org/#narrow/stream/7-test-here/topic/Chris/near/1714716
````quote
```math
1 + 1
```
````
"<blockquote>\n<p><span class=\"katex-display\"><span class=\"katex\"><span class=\"katex-mathml\"><math xmlns=\"http://www.w3.org/1998/Math/MathML\" display=\"block\"><semantics><mrow><mn>1</mn><mo>+</mo><mn>1</mn></mrow><annotation encoding=\"application/x-tex\">1 + 1</annotation></semantics></math></span><span class=\"katex-html\" aria-hidden=\"true\"><span class=\"base\"><span class=\"strut\" style=\"height:0.7278em;vertical-align:-0.0833em;\"></span><span class=\"mord\">1</span><span class=\"mspace\" style=\"margin-right:0.2222em;\"></span><span class=\"mbin\">+</span><span class=\"mspace\" style=\"margin-right:0.2222em;\"></span></span><span class=\"base\"><span class=\"strut\" style=\"height:0.6444em;\"></span><span class=\"mord\">1</span></span></span></span></span><br>\n</p>\n</blockquote>"
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.
Good find, thanks! I'll handle that, and also take this as a sign I should do some broader experimenting.
Revision pushed! PTAL. In addition to the review items above, I changed the visual design to the one more people preferred in that thread. (Screenshots updated.) |
Hmm, CI shows a test failure in a MathBlock content widget test. |
Oops, thanks — fixed. |
Thanks, LGTM! Merging. |
Fixes: #359
See discussion on visual design: https://chat.zulip.org/#narrow/stream/48-mobile/topic/TeX.20in.20Flutter.20app/near/1710839
Screenshots (of these messages):