Skip to content
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

Fix #4859, #4530: Images in 'li' tags display in inline rather than block mode. #5286

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5c2f3d3
made concep card image block
Vishwajith-Shettigar Dec 23, 2023
0f53710
test fixed
Vishwajith-Shettigar Dec 23, 2023
8a562f1
test fixed
Vishwajith-Shettigar Dec 23, 2023
e3d5b50
Merge branch 'develop' into imageli
Vishwajith-Shettigar Dec 23, 2023
3b447f3
Merge branch 'develop' into imageli
Vishwajith-Shettigar Dec 23, 2023
3ffcc4c
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jan 3, 2024
57cf81f
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jan 10, 2024
d202f23
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jan 17, 2024
ef7b4b5
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jan 29, 2024
6e46146
addressed comments
Vishwajith-Shettigar Jan 29, 2024
d71bf6d
addressed comments
Vishwajith-Shettigar Jan 29, 2024
aada9eb
addressed comments
Vishwajith-Shettigar Jan 30, 2024
2d4a51c
addressed comments
Vishwajith-Shettigar Jan 30, 2024
fc1ac35
test modified
Vishwajith-Shettigar Feb 1, 2024
c89df02
recalculate drawableLft and right
Vishwajith-Shettigar Feb 2, 2024
37c9fb5
Merge branch 'develop' into imageli
Vishwajith-Shettigar Mar 1, 2024
8026966
Merge branch 'develop' into imageli
Vishwajith-Shettigar Mar 9, 2024
74cdbb2
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jun 11, 2024
bdcc135
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jun 13, 2024
0eda433
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jun 22, 2024
d3d2bde
Merge branch 'develop' into imageli
Vishwajith-Shettigar Jul 3, 2024
44d27d5
Merge branch 'develop' into imageli
Vishwajith-Shettigar Aug 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ class HtmlParserTest {
}

@Test
fun testHtmlContent_imageWithText_noAdditionalSpacesAdded() {
fun testHtmlContent_imageWithText_imageSpanParsedCorrectly() {
val htmlParser = htmlParserFactory.create(
resourceBucketName,
entityType = "",
Expand All @@ -508,9 +508,6 @@ class HtmlParserTest {
val imageSpans = htmlResult.getSpansFromWholeString(ImageSpan::class)
assertThat(imageSpans).hasLength(1)
assertThat(imageSpans.first().source).isEqualTo("test.png")
// Verify that the image span does not start/end with a space since there is other text present.
assertThat(htmlResult.toString()).startsWith("A")
assertThat(htmlResult.toString()).doesNotContain(" ")
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,20 +79,28 @@ class HtmlParser private constructor(
supportsConceptCards: Boolean = false
): Spannable {
var htmlContent = rawString
val regex = Regex("""<oppia-noninteractive-image [^>]*>.*?</oppia-noninteractive-image>""")

// Canvas does not support RTL, it always starts from left to right in RTL due to which compound drawables are
// not center aligned. To avoid this situation check if RTL is enabled and set the textDirection.
if (isRtl) {
htmlContentTextView.textDirection = View.TEXT_DIRECTION_RTL

val regex = Regex("""<oppia-noninteractive-image [^>]*>.*?</oppia-noninteractive-image>""")
val modifiedHtmlContent = rawString.replace(regex) {
val oppiaImageTag = it.value
"""<div style="text-align: center;">$oppiaImageTag</div>"""
}
htmlContent = modifiedHtmlContent
} else {
htmlContentTextView.textDirection = View.TEXT_DIRECTION_LTR

// Images are wrapped inside a <div> tag, because the <div> tag is a block-level element,
// so that all images display in block mode and on a new line."
val modifiedHtmlContent = rawString.replace(regex) {
val oppiaImageTag = it.value
"""<div>$oppiaImageTag</div>"""
adhiamboperes marked this conversation as resolved.
Show resolved Hide resolved
}
htmlContent = modifiedHtmlContent
Comment on lines +96 to +103
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the suggested fix was to:

  1. Instead of wrapping the image in
    element, we can actually surround it with newlines (\n) at start and end because this is similar to the div implementation. Div is a block-level element, and at this point, we are already handling the image as a block.
  2. Instead of hardcoding this inside HtmlParser.kt, these new lines should be added before and after setspan in ImageTagHandler.kt. This makes the code less fragile/brittle.
  3. In the test, we can now remove the spaces assertion.
       """\n
        ${output.setSpan(
          ImageSpan(drawable, source),
          startIndex,
          endIndex,
          Spannable.SPAN_EXCLUSIVE_EXCLUSIVE
        )}
        \n"""

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll look at it.

}

htmlContentTextView.invalidate()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,14 +319,21 @@ class UrlImageParser private constructor(
drawableWidth -= maxContentItemPadding
}

val drawableLeft = if (imageCenterAlign && !isRTLMode()) {
var drawableLeft = if (imageCenterAlign && !isRTLMode()) {
calculateInitialMargin(maxAvailableWidth, drawableWidth)
} else {
0f
}

val drawableTop = 0f
val drawableRight = drawableLeft + drawableWidth
var drawableRight = drawableLeft + drawableWidth

// If the image is getting cut off, recalculate the positions of the drawableLeft and drawableRight.
if (drawableRight + maxContentItemPadding > maxAvailableWidth) {
drawableLeft -= maxContentItemPadding
drawableRight -= maxContentItemPadding
}

val drawableBottom = drawableTop + drawableHeight
return Rect(
drawableLeft.toInt(), drawableTop.toInt(), drawableRight.toInt(), drawableBottom.toInt()
Expand Down
Loading