Fixes #5809: Use view text color and force block rendering to fix math fraction visibility in dark mode#6283
Conversation
- Add equationColor parameter to MathTagHandler so callers can pass the host view's currentTextColor instead of relying on the app-level theme resource. This fixes contrast when a view's background differs from the global theme (e.g. a dark card inside a light screen). - HtmlParser now passes htmlContentTextView.currentTextColor as the equation color so LaTeX adopts the same color as the surrounding text. - Force block/display rendering mode whenever a \frac expression is detected. KotliTeX renders \frac with a horizontal bar only in block mode; inline mode produces a barely visible diagonal slash. - Update existing tests to expect BLOCK_IMAGE for all \frac content and add two new tests covering the provided-equationColor code path.
- Update 2 inline-alignment tests to use x^2 (non-\frac) since \frac is now forced to block mode by the visibility fix - Add MATH_WITHOUT_FILENAME_INLINE_MARKUP constant for non-\frac test cases - Add try-import for .bazelrc.local to allow machine-specific build overrides - Gitignore .bazelrc.local, MODULE.bazel, and MODULE.bazel.lock
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@nikhilkumarpanigrahi this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes dark-mode LaTeX fraction visibility by ensuring math rendering inherits the surrounding TextView color and by forcing \frac expressions to render in block/display mode so the horizontal bar is shown.
Changes:
- Pass
htmlContentTextView.currentTextColorintoMathTagHandlerso LaTeX uses the correct theme-aware color. - Force block rendering for LaTeX containing
\fracto ensure fraction bars render. - Update and add tests for provided equation color and block rendering behavior; add Bazel local override support.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utility/src/test/java/org/oppia/android/util/parser/html/MathTagHandlerTest.kt | Updates existing expectations for \frac to block rendering and adds tests for custom equation colors. |
| utility/src/main/java/org/oppia/android/util/parser/html/MathTagHandler.kt | Adds optional equationColor override and forces block rendering for LaTeX containing \frac. |
| utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt | Passes the current TextView text color into MathTagHandler for theme-correct math rendering. |
| .gitignore | Ignores local Bazel override and Bazel module files. |
| .bazelrc | Adds try-import for an optional, gitignored .bazelrc.local. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun testParseHtml_withMathMarkup_cachingOn_withProvidedEquationColor_usesProvidedColor() { | ||
| val customColor = Color.CYAN | ||
| CustomHtmlContentHandler.fromHtml( | ||
| html = MATH_WITHOUT_FILENAME_MARKUP, | ||
| imageRetriever = mockImageRetriever, | ||
| customTagHandlers = mapOf( | ||
| CUSTOM_MATH_TAG to createMathTagHandler(cacheLatexRendering = true, equationColor = customColor) | ||
| ) | ||
| ) | ||
|
|
||
| verify(mockImageRetriever)!!.loadMathDrawable( | ||
| capture(stringCaptor), capture(floatCaptor), | ||
| capture(colorCaptor), capture(retrieverTypeCaptor) | ||
| ) | ||
| assertThat(colorCaptor.value).isEqualTo(customColor) | ||
| assertThat(retrieverTypeCaptor.value).isEqualTo(ImageRetriever.Type.BLOCK_IMAGE) | ||
| } |
There was a problem hiding this comment.
Done — switched the fixture to MATH_WITHOUT_FILENAME_INLINE_MARKUP (uses x^2, no \frac) so the color assertion is independent of the block-forcing behavior, and removed the BLOCK_IMAGE assertion.
| val equationColor = parsedHtml.getSpansFromWholeString(MathExpressionSpan::class) | ||
| assertThat(equationColor[0].equationColor).isEqualTo(customColor) |
There was a problem hiding this comment.
Done, renamed equationColor to mathExpressionSpans to accurately reflect that it holds a collection of spans, not a color value.
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 similar comment
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Three lines exceeded the 100-char limit: one overly long test name and two single-line createMathTagHandler() calls inside mapOf(). Shorten the test name and wrap the argument lists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The frac block-forcing fix causes any frac expression to render in block mode, so the inline-mode test must use a non-frac expression (x^2) to correctly exercise the inline rendering path.
The inline-mode test used \\x^2 in the markup string which decodes to \x^2 as the LaTeX value, but the assertion expected x^2. Use x^2 directly in the markup so the decoded value matches the assertion.
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
@nikhilkumarpanigrahi this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
|
@adhiamboperes PTAL when you get a chance! |
|
Hi @nikhilkumarpanigrahi, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
Hi @BenHenning if you have still not started reviewing this PR yet, would it be okay if I take a first pass on it? |
Neer-rn
left a comment
There was a problem hiding this comment.
Hi @nikhilkumarpanigrahi, thanks for working on this.
I checked this, but I am still not fully sure that this PR is fixing #5809. Could you please explain how you reproduced the issue on current develop before this fix?
From what I checked, the “Writing Ratios in Simplest Form” asset in current develop has the relevant option as plain HTML:
domain/src/main/assets/dev/tIoSb3HZFN6e.textproto -> <p>1:1/7</p>
domain/src/main/assets/dev/tIoSb3HZFN6e.json -> <p>1:1/7</p>
So I am not able to understand how this goes through the <oppia-noninteractive-math> / MathTagHandler LaTeX path changed in this PR.
Also I remember sangeeta working on #6122 and it already fixed the LaTeX color/cache path for dark mode if I am correct, and current develop must have that change. So could you please clarify what exact remaining gap this PR is fixing after #6122?
The screenshot attached in this PR only shows the after state. Could you please add a before screenshot from develop and an after screenshot from this PR for the same screen/language? Without these, I am not sure about PR. PTAL
| null | ||
| ) | ||
| // \frac renders with a horizontal bar only in block/display mode; force it so the bar is visible. | ||
| if (content is MathContent.MathAsLatex && content.rawLatex.contains("\\frac")) { |
There was a problem hiding this comment.
I am not fully sure about forcing every \frac expression into block mode here. This seems to override render-type="inline" for all fractions across the app. From checking the KotliTeX fraction code, \frac already creates a fraction line, so I am not sure the horizontal bar only exists in block/display mode.
Could you please share a small before/after screenshot showing that inline \frac loses the horizontal bar? This feels separate from the dark-mode contrast issue in #5809, and it may change inline lesson layout in other places.
Also, the old inline \frac test was changed to x^2, so we no longer have coverage for the explicit inline fraction case. Can we avoid this override, or add proof/tests that this behavior is intended? PTAL.
|
Unassigning @Neer-rn since the review is done. |
|
Hi @nikhilkumarpanigrahi, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
Thanks @Neer-rn! About reproducing it / the asset. You are correct. The bundled dev asset About #6122. I agree #6122 already made the equation color match the theme (white in dark mode, black in light mode), and it is already in this branch. But the issue screenshot is in light mode, where the color was already correct. So this is not a color or dark-mode problem. The real problem is that the fraction's horizontal bar is too thin to see at inline size. That means my color change does not help #5809, so I will remove it. About forcing block mode. You are right about KotliTeX. I checked So here is my plan:
One question before I rework this: if I can show it is readable and does not break other layouts, would forcing block size for |
|
Thanks @nikhilkumarpanigrahi for checking all this in detail, it explains the actual bug much better now. For the direction you asked: I would prefer the targeted rendering fix. Forcing block mode overrides the One place worth checking: the fraction bar drawing in the fork's This changes rendering for all lessons though, so I would suggest getting @BenHenning's thoughts on the direction as well. Your 4-step plan looks good to me otherwise, please go ahead. |
|
Hi @nikhilkumarpanigrahi, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
Thanks @Neer-rn, that pointer really helped. I went through the fork (we're pinned to The bar is drawn here: // MathExpressionSpan.kt:122
paint.strokeWidth = max(1.0f, parent.bounds.height.toFloat())So yes, there's a hard But then I traced where // RenderTreeBuilder.kt:55
line.height = thickness ?: options.fontMetrics.defaultRuleThickness
// FontMetrics.kt:98 -> defaultRuleThickness = 0.04 (em)That 0.04 em gets scaled by So the bar isn't thin because we're clamping it to 1px — it's thin because 0.04 em is just physically tiny (under about 1dp) at inline size. One other thing I noticed while I was in there — the bar's paint isn't anti-aliased, unlike the text: // MathExpressionSpan.kt:45-46
val paint = Paint() // bar - no AA
val textPaint = TextPaint(Paint.ANTI_ALIAS_FLAG) // text - AAA sub-1dp line with no AA is exactly the kind of thing that shows up as a faint, patchy hairline, which lines up with the screenshot in #5809. So I still think your suggestion is the right direction, just with two tweaks: a 1dp floor might only be slightly thicker than what's already being drawn (~0.9dp), so it may need to be a bit more (~1.5dp) to actually make a visible difference, and we should probably turn on anti-aliasing for that paint too. One catch on implementation: On this PR — I'm going to strip out the block-forcing and the @BenHenning — Neer suggested I loop you in since this would change rendering across every lesson. Would you be okay with a kotlitex fork change that makes the minimum rule thickness density-aware instead of a flat 1px, and enables anti-aliasing on the rule paint? One caveat: the 2-3px number above is me doing the math from the source, not an actual measurement. Before I settle on a specific value I'll log the real |
|
Hi @nikhilkumarpanigrahi, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Explanation
Fixes #5809
In dark mode, LaTeX math expressions containing \frac were invisible because MathTagHandler was using a hardcoded equation color instead of the surrounding text view's current color. Additionally, \frac expressions rendered in inline mode omit the horizontal fraction bar.
This PR fixes both issues:
Before: fractions such as \frac{1}{7} were invisible in dark mode (white image on dark background).
After: fractions are rendered using the view's text color and are fully visible.
Essential Checklist
For UI-specific PRs only