-
-
Notifications
You must be signed in to change notification settings - Fork 893
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
Add preliminary support for MathML #577
Conversation
Adding support for flutter_math will be very helpful in the long run. I needed support for some vertical-align in #556 which was only because I was converting tex to SVG images. This feature will render Math like a native widget which is much better than using SVGs. |
That's great @ngaurav! - since you use this feature, do you mind testing this PR against your tex strings and your |
@tneotia Sure I will be happy to help. |
I was surprised to not find any issue till now in rendering. It truly delivers. One minor issue is that the tex part is always rendered in a new line. I hope this can be fixed, as shown here. Once that is done, then it remains to be seen if the alignment with the baseline is perfect on not. Here is a sample code: Here is how it is rendered currently. It should ideally be inline. |
Simple fix, this was in place for mathML since the It seems it is aligning slightly down: It's very close though... I'll see if there's a way to improve it. Edit: This is the result with 3.5 bottom padding: This is pushed here, can you try it out again @ngaurav and see how it looks for more complex Tex? I made it so that inline Tex is rendered with text style and height in mind so the baseline looks good, while tex by itself renders in the large display font. This is not pushed here yet so let me know your thoughts trying the implementation pushed right now compared to the above screenshot. |
We need to verify if vertical alignment is solved for all kinds of tex code. Try to render the following code. If it works, we can go ahead. |
@ngaurav I don't know exactly how it should look but this is the result from the current implementation: What do you think? |
lib/src/replaced_element.dart
Outdated
case "math": | ||
return MathElement( | ||
element: element, | ||
texStr: r'', |
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: wouldn't null
make more sense than an empty string?
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 agree, I changed it. The latest commit also has some extra changes for styling, if you'd like to review that.
pubspec.yaml
Outdated
@@ -29,7 +29,10 @@ dependencies: | |||
chewie_audio: ^1.2.0 | |||
|
|||
# Plugins for rendering the <svg> tag. | |||
flutter_svg: ^0.21.0-nullsafety.0 | |||
flutter_svg: ^0.20.0-nullsafety.3 |
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.
Why do we need the downgrade 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.
flutter_math was constrained to 0.20.0, I changed the dependency to flutter_math_fork because flutter_math hasn't been maintained for a bit. It's essentially the same but has a hotfix for an error occurring on Flutter 2.0.0+.
pubspec.yaml
Outdated
flutter_svg: ^0.20.0-nullsafety.3 | ||
|
||
# Plugin for rendering MathML | ||
flutter_math: ^0.3.0-nullsafety.1 |
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.
Another external dependency... I should really start work on the modularization.
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 was watching flutter_widget_from_html and its just completed modularization... It uses widget factories (I still don't know what these are and how they work) and forks of each dependency as an "add-on" to the original lib.
@tneotia I think the baseline rendering is fine when we use a bottom padding of 3.5. But I would like to test it myself. I am using /tneotia/flutter_html currently and the |
Hm did you pull the latest changes? It should display inline. |
Yes, I did clean my pub cache, and also did flutter clean. I still don't see tex in inline. Can you point me to simple fix (commit) which enabled inline. |
Thanks, it helped. I just changed the condition |
A generic fix for this alignment issue is to allow support for vertical-align css as mentioned in #556 |
I see... So would you say it's acceptable for now (and we do vertical align in a separate PR) or should any changes be made in this PR? |
Technically this PR is sound but one change is necessary – Bottom padding of 3.5 should be removed. Also, there are two issues where discretion needs to be exercised:
|
For 2) that is strange as it should work even without the body tag, since that's how I tested. I'll investigate it and see what's going on, and remove the 3.5 padding. I wonder what it will take for vertical-align, I saw you used Transform or something like that. Maybe I'll attack that next.... |
Actually @ngaurav the behavior for 2) is intentional. My reasoning is that if somebody is using HTML: <body>
<tex>x^2</tex>
<tex>y^2</tex>
</body> The tex should not render like
(basically display as block element). When inline, then it will display inline. A simple fix for the sample html If you disagree let me know, this was just my thought process. Other than that I removed the 3.5 padding. Also I am debating, should I add a horizontally scrollable widget to |
The behaviour of So, I think to get the two lines effect |
@erickok this is ready to go now. |
I'm very late to the discussion, however, I wanted to get a really quick comment in. We've been denying a lot of custom tags, or custom tag render-ers features and although I wholeheartedly support the Don't get me wrong, we've wanted to support it for a long time, but I'm wondering if a custom element is the correct way, or if feature parity of the Tex libraries would be more correct via supporting more CSS properties would be the correct way. I'm open to whatever though, as it is a community decision, not 100% my own. |
I would suggest using the As far as I can tell, it will be quite the effort to fully and correctly support Tex, as it requires us to be able to 1) parse external CSS and 2) support quite a few more inline styles to render the Tex correctly. Adding this tag doesn't affect anyone using well-formed HTML, but it can be useful for the time-being for people like @ngaurav who need to display a combination of rich text and Tex with ease. Another thing we could do is remove |
I think the custom tex tag should be supported for two reasons:
|
I agree with @ryan-berger here. The moment we officially support a custom tag such as
|
@erickok Sounds great. It acts as a nice demo for using custom tags. |
@erickok Doesn't this still leave us open to the same issue but in a different form? We've talked about adding customRender for just any tag, but we've never done so. Currently we have a whitelist of tags that we check. I still wonder if we want to lean into that or not because it sets an interesting precedent |
If I remember correctly customRender supports custom tags, in the example html at one point we used customRender to render |
Yes, it supports custom tags. We're working with a lot of custom tags like the workaround I've mentioned here. |
Custom tags have always been supported and we should support them. It's just official support for tags that don't exist in html that we should not bring into the library, in my opinion. Html spec has no tag. |
I removed the official support for tex and added a note in the README in the two most recent commits. |
Fixes #219
I know we are trying to reduce the number of packages this library depends on, but I really think flutter_math is a good addition - I'm super pleased with how the implementation turned out!
I added support for a "custom" tag in the second commit. I know we are trying to adhere to HTML standard as close as possible, but I thought it would be a nice feature to directly add Tex support inside HTML if we are already using a package to render math using Tex strings. This is also useful for #207 as a temporary workaround until we decide to implement external CSS support (if that's even possible). If this doesn't seem like a good feature then I can revert the commit.