From a4317ba22be260c313b6c7d9a80abd87908d8441 Mon Sep 17 00:00:00 2001 From: Manas <119405883+manas-yu@users.noreply.github.com> Date: Tue, 11 Feb 2025 15:08:37 +0530 Subject: [PATCH] Fix #4859: Images in List Items Are Rendered as Block Elements (#5687) ## Explanation Fix #4859 This PR adds `formatImageSpans` to `LiTagHandler` to properly format images that appear within `
  • ` tags. The new formatImageSpans function ensures that ImageSpan elements are rendered as block images by inserting line breaks before and after them if they are not already surrounded by `\n`. ## Before ![before_rtl](https://github.com/user-attachments/assets/749f329b-56a3-4527-8d5b-ce8f17ba4d65) ![before](https://github.com/user-attachments/assets/e8ca9ed8-081a-4bf4-bc5a-ccbfe4047b26) ## After ![after](https://github.com/user-attachments/assets/975ba71d-8245-4f4d-a5a4-b46ea83c0af5) ![after_rtl](https://github.com/user-attachments/assets/b3d09c97-b7e1-4caf-b90d-b1d23397f161) ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- .../parser/html/CustomHtmlContentHandler.kt | 2 +- .../android/util/parser/html/LiTagHandler.kt | 74 +++++++++++++++++++ .../util/parser/html/LiTagHandlerTest.kt | 28 +++++++ 3 files changed, 103 insertions(+), 1 deletion(-) diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt b/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt index bca56be075e..25cc7bf29d3 100644 --- a/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/CustomHtmlContentHandler.kt @@ -202,7 +202,7 @@ class CustomHtmlContentHandler private constructor( * Called when the closing of a custom tag is encountered. This does not support processing * attributes of the tag--[handleTag] should be used, instead. * - * This function will always be called before [handleClosingTag]. + * This function will always be called before [handleTag]. * * @param output the destination [Editable] to which spans can be added * @param indentation The zero-based indentation level of this item. diff --git a/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt b/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt index 849f2e226be..28321115984 100644 --- a/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt +++ b/utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt @@ -1,9 +1,13 @@ package org.oppia.android.util.parser.html import android.content.Context +import android.graphics.Canvas +import android.graphics.Paint import android.text.Editable import android.text.Spannable import android.text.Spanned +import android.text.style.ImageSpan +import androidx.core.view.ViewCompat import org.oppia.android.util.locale.OppiaLocale import org.xml.sax.Attributes import java.util.Stack @@ -57,6 +61,76 @@ class LiTagHandler( } CUSTOM_LIST_LI_TAG -> latestPendingList?.closeItem(output) } + formatImageSpans(output) + } + + /** Formats ImageSpans to ensure they render as block images with line breaks. */ + private fun formatImageSpans(output: Editable) { + val imageSpans = output.getSpans(0, output.length, ImageSpan::class.java) + + imageSpans.sortedByDescending { output.getSpanStart(it) }.forEach { span -> + val startIndex = output.getSpanStart(span) + val endIndex = output.getSpanEnd(span) + + if (startIndex >= 0 && endIndex <= output.length) { + if (endIndex < output.length && output[endIndex] != '\n') { + output.insert(endIndex, "\n") + } + if (startIndex > 0 && output[startIndex - 1] != '\n') { + output.insert(startIndex, "\n") + } + + val currentStart = output.getSpanStart(span) + val currentEnd = output.getSpanEnd(span) + val leadingMargins = output.getSpans( + currentStart, + currentStart, + ListItemLeadingMarginSpan::class.java + ) + val totalMargin = leadingMargins.sumOf { it.getLeadingMargin(true) } + val isRtl by lazy { + displayLocale.getLayoutDirection() == ViewCompat.LAYOUT_DIRECTION_RTL + } + + output.removeSpan(span) + val customSpan = CustomImageSpan(span, totalMargin, isRtl) + output.setSpan( + customSpan, + currentStart, + currentEnd, + output.getSpanFlags(span) + ) + } + } + } + + /** + * Custom [ImageSpan] that shifts the drawing position by the total margin of parent list items. + */ + private class CustomImageSpan( + originalSpan: ImageSpan, + private val totalMargin: Int, + private val isRtl: Boolean + ) : ImageSpan(originalSpan.drawable, originalSpan.verticalAlignment) { + + override fun draw( + canvas: Canvas, + text: CharSequence, + start: Int, + end: Int, + x: Float, + top: Int, + baseline: Int, + bottom: Int, + paint: Paint + ) { + val adjustedX = if (isRtl) { + x + totalMargin + } else { + x - totalMargin + } + super.draw(canvas, text, start, end, adjustedX, top, baseline, bottom, paint) + } } private sealed class ListTag, S : ListItemLeadingMarginSpan>( diff --git a/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt b/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt index 487cda78b52..13246b7d2d8 100644 --- a/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt +++ b/utility/src/test/java/org/oppia/android/util/parser/html/LiTagHandlerTest.kt @@ -4,6 +4,7 @@ import android.app.Application import android.content.Context import android.text.Html import android.text.Spannable +import android.text.style.ImageSpan import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat @@ -113,6 +114,33 @@ class LiTagHandlerTest { .hasLength(2) } + @Test + fun testFromHtml_withImageSpanInsideList_insertsNewlinesAroundImage() { + val html = "TestImage" + val displayLocale = createDisplayLocaleImpl(US_ENGLISH_CONTEXT) + val liTaghandler = LiTagHandler(context, displayLocale) + val parsedHtml = + CustomHtmlContentHandler.fromHtml( + html = html, + imageRetriever = mockImageRetriever, + customTagHandlers = mapOf( + CUSTOM_LIST_LI_TAG to liTaghandler, + CUSTOM_LIST_UL_TAG to liTaghandler + ) + ) + + val imageSpans = parsedHtml.getSpans(0, parsedHtml.length, ImageSpan::class.java) + assertThat(imageSpans).hasLength(1) + val imageSpan = imageSpans[0] + val start = parsedHtml.getSpanStart(imageSpan) + val end = parsedHtml.getSpanEnd(imageSpan) + + assertThat(parsedHtml[start - 1]).isEqualTo('\n') + assertThat(parsedHtml[end]).isEqualTo('\n') + assertThat(parsedHtml.toString()).contains("Test") + assertThat(parsedHtml.toString()).contains("Image") + } + @Test fun testCustomListElement_betweenNestedParagraphs_parsesCorrectlyIntoNumberedListSpan() { val displayLocale = createDisplayLocaleImpl(US_ENGLISH_CONTEXT)