Skip to content

fix(android): report image span height during measurement - #933

Open
eliotgevers wants to merge 1 commit into
software-mansion:mainfrom
eliotgevers:fix/android-image-span-font-metrics
Open

eliotgevers wants to merge 1 commit into
software-mansion:mainfrom
eliotgevers:fix/android-image-span-font-metrics

Conversation

@eliotgevers

Copy link
Copy Markdown
Contributor

What/Why?

Fixes #932.

ImageSpan.getSize() returns the drawable width but omits its font metrics. Android can then measure mixed image content too short, letting its trailing text overlap the next component.

Report the drawable height through FontMetricsInt, matching Android's DynamicDrawableSpan.getSize(). Width, drawing, and the JavaScript API stay unchanged.

Testing

  • Minimal reproduction and before/after screenshots: fresh Android 16 native builds with enriched-markdown 1.1.0, Expo 57.0.26 and React Native 0.86.3. main reproduces the overlap; fixed uses this exact Kotlin change and keeps the trailing paragraph above the next component.
  • Current main's affected Kotlin file is identical to the unpatched 1.1.0 file. The fuller mixed-content case also reproduces on React Native 0.88.0-rc.3.
  • Real-content Android checks cover inline/block/repeated images, image taps, linked-image precedence, text selection and scrolling.
  • ktlint 1.8.0 and git diff --check pass.

The upstream example app and Maestro suite were not run. iOS code is unchanged.

PR Checklist

  • Code compiles and runs on iOS (unchanged; no upstream example build)
  • Code compiles and runs on Android
  • Updated documentation/README if applicable (no API change)
  • Ran example app to verify changes (used the linked standalone reproduction)
  • E2E tests are passing
  • Required E2E tests have been added (if applicable)

@eszlamczyk eszlamczyk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the repro — it's what made this diagnosable.

I don't think this is the correct solution, though. Instrumenting ImageSpan and running your repro shows measurement is already correct — it's the view that's wrong.

Pixel 9 (density 2.625): maxHeight: 320 = 840px, fitted = 339px.

wrapAndAssign hasView=true targetWidth=954  boxHeight 840 -> 339   ← fits correctly
chooseHeight  boxHeight=840                 lineH 49 -> 840        ← already ran at 840
draw          lineBoxH=840  drawableH=339                          ← stale line box

applyStyledText assigns the text (building the layout at 840) before registerTextView fits boxHeight (SegmentViewCreators.kt:102-103). On image load requestReflow() only calls setSpan, so TextView keeps its cached Layout and the 840 line heights survive. The image then draws at 339 inside an 840 box: 501px of dead space.

Reporting drawable bounds from getSize hides this because the measure-pass drawable is built at targetWidth = 0 — bounds are 840, so measurement starts agreeing with the view's bug.

The cost is visible in your own after.png: the image-only component gains ~198dp of blank space below its image. Component 1 is pixel-identical between before.png and after.png — the patch doesn't move any content, it only grows the measured box around the gap that was already there. It also explains why image-only "can pass": with no trailing text the dead space falls outside the clipped component and is simply invisible.

Where it goes wrong: ImageSpan.requestReflow()

private fun requestReflow() {
  val view = viewRef?.get() ?: return
  val text = view.text
  if (text is Spannable) {
    ...
    text.setSpan(this, start, end, Spannable.SPAN_EXCLUSIVE_EXCLUSIVE)
  } else {
    view.invalidate()
    view.requestLayout()   // ← never runs: the text is always Spannable here
  }
  notifyBoxHeightMayHaveChanged(view)
}

Re-setting a span doesn't make TextView rebuild its Layout, so the line heights chooseHeight produced at the old boxHeight persist.

Fix (ImageSpan.kt:165):

private fun requestReflow(boxHeightChanged: Boolean = false) {
  ...
  if (boxHeightChanged) {
    view.text = text       // force TextView to discard its cached Layout
  }
  view.invalidate()
  view.requestLayout()
  notifyBoxHeightMayHaveChanged(view)
}

plus threading boxHeightChanged from the two sites that change boxHeight (wrapAndAssignDrawable, updateWidthAndRecreate). getSize untouched. Both defects are gone on your repro, with no gap on the image-only control.

I'll open a PR with that shortly and link it here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Android mixed images overlap following components

2 participants