diff --git a/CHANGELOG.md b/CHANGELOG.md index 57d640f385a..0cfda7df07b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Fixes + +- Session Replay: Fix Compose text masking mismatch with weighted text ([#5218](https://github.com/getsentry/sentry-java/pull/5218)) + ### Features - Android: Add `beforeErrorSampling` callback to Session Replay ([#5214](https://github.com/getsentry/sentry-java/pull/5214)) diff --git a/sentry-android-core/build.gradle.kts b/sentry-android-core/build.gradle.kts index 1134e948226..ffd42c7d4d7 100644 --- a/sentry-android-core/build.gradle.kts +++ b/sentry-android-core/build.gradle.kts @@ -4,6 +4,7 @@ import org.jetbrains.kotlin.config.KotlinCompilerVersion plugins { id("com.android.library") alias(libs.plugins.kotlin.android) + alias(libs.plugins.kotlin.compose) jacoco alias(libs.plugins.jacoco.android) alias(libs.plugins.errorprone) @@ -108,7 +109,11 @@ dependencies { testImplementation(projects.sentryCompose) testImplementation(projects.sentryAndroidNdk) testImplementation(libs.dropbox.differ) - testRuntimeOnly(libs.androidx.compose.ui) + testImplementation(libs.androidx.activity.compose) + testImplementation(libs.androidx.compose.ui) + testImplementation(libs.androidx.compose.foundation) + testImplementation(libs.androidx.compose.foundation.layout) + testImplementation(libs.androidx.compose.material3) testRuntimeOnly(libs.androidx.fragment.ktx) testRuntimeOnly(libs.timber) } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index acc38228e6b..b325f9a3500 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -9,12 +9,27 @@ import android.graphics.Color import android.graphics.drawable.Drawable import android.os.Bundle import android.os.Looper +import android.text.TextUtils import android.view.View import android.widget.ImageView import android.widget.LinearLayout import android.widget.LinearLayout.LayoutParams import android.widget.RadioButton import android.widget.TextView +import androidx.activity.ComponentActivity +import androidx.activity.compose.setContent +import androidx.compose.foundation.background +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.material3.Text +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.text.style.TextOverflow +import androidx.compose.ui.unit.dp +import androidx.compose.ui.unit.sp import androidx.test.ext.junit.runners.AndroidJUnit4 import com.dropbox.differ.Color as DifferColor import com.dropbox.differ.Image @@ -410,6 +425,48 @@ class ScreenshotEventProcessorTest { assertNotNull(bytes) } + @Test + fun `snapshot - screenshot with ellipsized text no masking`() { + fixture.activity = buildActivity(EllipsizedTextActivity::class.java, null).setup().get() + val bytes = + processEventForSnapshots( + "screenshot_mask_ellipsized_view_unmasked", + isReplayAvailable = false, + ) + assertNotNull(bytes) + } + + @Test + fun `snapshot - screenshot with ellipsized text masking`() { + fixture.activity = buildActivity(EllipsizedTextActivity::class.java, null).setup().get() + val bytes = + processEventForSnapshots("screenshot_mask_ellipsized_view_masked") { + it.screenshot.setMaskAllText(true) + } + assertNotNull(bytes) + } + + @Test + fun `snapshot - compose text no masking`() { + fixture.activity = buildActivity(ComposeTextActivity::class.java, null).setup().get() + val bytes = + processEventForSnapshots( + "screenshot_mask_ellipsized_compose_unmasked", + isReplayAvailable = false, + ) + assertNotNull(bytes) + } + + @Test + fun `snapshot - compose text with masking`() { + fixture.activity = buildActivity(ComposeTextActivity::class.java, null).setup().get() + val bytes = + processEventForSnapshots("screenshot_mask_ellipsized_compose_masked") { + it.screenshot.setMaskAllText(true) + } + assertNotNull(bytes) + } + // endregion private fun getEvent(): SentryEvent = SentryEvent(Throwable("Throwable")) @@ -484,6 +541,189 @@ private class CustomView(context: Context) : View(context) { } } +private class EllipsizedTextActivity : Activity() { + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + val longText = "This is a very long text that should be ellipsized when it does not fit" + + val linearLayout = + LinearLayout(this).apply { + setBackgroundColor(Color.WHITE) + orientation = LinearLayout.VERTICAL + layoutParams = LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT) + setPadding(10, 10, 10, 10) + } + + // Ellipsize end + linearLayout.addView( + TextView(this).apply { + text = longText + setTextColor(Color.BLACK) + textSize = 16f + maxLines = 1 + ellipsize = TextUtils.TruncateAt.END + setBackgroundColor(Color.LTGRAY) + layoutParams = + LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT).apply { + setMargins(0, 8, 0, 0) + } + } + ) + + // Ellipsize middle + linearLayout.addView( + TextView(this).apply { + text = longText + setTextColor(Color.BLACK) + textSize = 16f + maxLines = 1 + ellipsize = TextUtils.TruncateAt.MIDDLE + setBackgroundColor(Color.LTGRAY) + layoutParams = + LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT).apply { + setMargins(0, 8, 0, 0) + } + } + ) + + // Ellipsize start + linearLayout.addView( + TextView(this).apply { + text = longText + setTextColor(Color.BLACK) + textSize = 16f + maxLines = 1 + ellipsize = TextUtils.TruncateAt.START + setBackgroundColor(Color.LTGRAY) + layoutParams = + LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT).apply { + setMargins(0, 8, 0, 0) + } + } + ) + + // Non-ellipsized text for comparison + linearLayout.addView( + TextView(this).apply { + text = "Short text" + setTextColor(Color.BLACK) + textSize = 16f + setBackgroundColor(Color.LTGRAY) + layoutParams = + LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT).apply { + setMargins(0, 8, 0, 0) + } + } + ) + + setContentView(linearLayout) + } +} + +private class ComposeTextActivity : ComponentActivity() { + + override fun onCreate(savedInstanceState: Bundle?) { + super.onCreate(savedInstanceState) + val longText = "This is a very long text that should be ellipsized when it does not fit in view" + + setContent { + Column( + modifier = + Modifier.fillMaxWidth() + .background(androidx.compose.ui.graphics.Color.White) + .padding(10.dp), + verticalArrangement = Arrangement.spacedBy(8.dp), + ) { + // Ellipsis overflow + Text( + longText, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + fontSize = 16.sp, + modifier = + Modifier.fillMaxWidth().background(androidx.compose.ui.graphics.Color.LightGray), + ) + + // Text with textAlign center + Text( + "Centered text", + textAlign = TextAlign.Center, + fontSize = 16.sp, + modifier = + Modifier.fillMaxWidth().background(androidx.compose.ui.graphics.Color.LightGray), + ) + + // Text with textAlign end + Text( + "End-aligned text", + textAlign = TextAlign.End, + fontSize = 16.sp, + modifier = + Modifier.fillMaxWidth().background(androidx.compose.ui.graphics.Color.LightGray), + ) + + // Ellipsis with textAlign center + Text( + longText, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + textAlign = TextAlign.Center, + fontSize = 16.sp, + modifier = + Modifier.fillMaxWidth().background(androidx.compose.ui.graphics.Color.LightGray), + ) + + // Weighted row with text + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.spacedBy(8.dp), + ) { + Text( + "Weight 1", + fontSize = 16.sp, + modifier = Modifier.weight(1f).background(androidx.compose.ui.graphics.Color.LightGray), + ) + Text( + "Weight 1", + fontSize = 16.sp, + modifier = Modifier.weight(1f).background(androidx.compose.ui.graphics.Color.LightGray), + ) + } + + // Weighted row with ellipsized text + Row( + modifier = Modifier.fillMaxWidth(), + horizontalArrangement = Arrangement.spacedBy(8.dp), + ) { + Text( + longText, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + fontSize = 16.sp, + modifier = Modifier.weight(1f).background(androidx.compose.ui.graphics.Color.LightGray), + ) + Text( + longText, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + textAlign = TextAlign.End, + fontSize = 16.sp, + modifier = Modifier.weight(1f).background(androidx.compose.ui.graphics.Color.LightGray), + ) + } + + // Short text (for comparison) + Text( + "Short text", + fontSize = 16.sp, + modifier = Modifier.background(androidx.compose.ui.graphics.Color.LightGray), + ) + } + } + } +} + private class MaskingActivity : Activity() { override fun onCreate(savedInstanceState: Bundle?) { diff --git a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_all.png b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_all.png index 31ba5f581bc..aa1ec41ee06 100644 Binary files a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_all.png and b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_all.png differ diff --git a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_compose_masked.png b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_compose_masked.png new file mode 100644 index 00000000000..53e5a236c3e Binary files /dev/null and b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_compose_masked.png differ diff --git a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_compose_unmasked.png b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_compose_unmasked.png new file mode 100644 index 00000000000..efc2304c4f2 Binary files /dev/null and b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_compose_unmasked.png differ diff --git a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_view_masked.png b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_view_masked.png new file mode 100644 index 00000000000..1530d58fd9a Binary files /dev/null and b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_view_masked.png differ diff --git a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_view_unmasked.png b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_view_unmasked.png new file mode 100644 index 00000000000..57b4bfa8f42 Binary files /dev/null and b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_ellipsized_view_unmasked.png differ diff --git a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_text.png b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_text.png index 952fd6ba634..6ffbe1e07f5 100644 Binary files a/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_text.png and b/sentry-android-core/src/test/resources/snapshots/ScreenshotEventProcessorTest/screenshot_mask_text.png differ diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt index 1d779d389bf..cd9e3dd208a 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt @@ -13,38 +13,39 @@ import androidx.compose.ui.node.LayoutNode import androidx.compose.ui.text.TextLayoutResult import kotlin.math.roundToInt -internal class ComposeTextLayout( - internal val layout: TextLayoutResult, - private val hasFillModifier: Boolean, -) : TextLayout { +internal class ComposeTextLayout(internal val layout: TextLayoutResult) : TextLayout { override val lineCount: Int get() = layout.lineCount override val dominantTextColor: Int? get() = null - override fun getPrimaryHorizontal(line: Int, offset: Int): Float { - val horizontalPos = layout.getHorizontalPosition(offset, usePrimaryDirection = true) - // when there's no `fill` modifier on a Text composable, compose still thinks that there's - // one and wrongly calculates horizontal position relative to node's start, not text's start - // for some reason. This is only the case for single-line text (multiline works fien). - // So we subtract line's left to get the correct position - return if (!hasFillModifier && lineCount == 1) { - horizontalPos - layout.getLineLeft(line) - } else { - horizontalPos + /** + * The paragraph may be laid out with a wider width (constraint maxWidth) than the actual node + * (layout result size). When that happens, getLineLeft/getLineRight return positions in the + * paragraph coordinate system, which don't match the node's bounds. In that case, text alignment + * has no visible effect, so we fall back to using line width starting from x=0. + */ + private val paragraphWidthExceedsNode: Boolean + get() = layout.multiParagraph.width > layout.size.width + + override fun getLineLeft(line: Int): Float { + if (paragraphWidthExceedsNode) { + return 0f } + return layout.getLineLeft(line) } - override fun getEllipsisCount(line: Int): Int = if (layout.isLineEllipsized(line)) 1 else 0 - - override fun getLineVisibleEnd(line: Int): Int = layout.getLineEnd(line, visibleEnd = true) + override fun getLineRight(line: Int): Float { + if (paragraphWidthExceedsNode) { + return layout.multiParagraph.getLineWidth(line) + } + return layout.getLineRight(line) + } override fun getLineTop(line: Int): Int = layout.getLineTop(line).roundToInt() override fun getLineBottom(line: Int): Int = layout.getLineBottom(line).roundToInt() - - override fun getLineStart(line: Int): Int = layout.getLineStart(line) } // TODO: probably most of the below we can do via bytecode instrumentation and speed up at runtime @@ -92,8 +93,6 @@ internal fun Painter.isMaskable(): Boolean { !className.contains("Brush") } -internal data class TextAttributes(val color: Color?, val hasFillModifier: Boolean) - /** * This method is necessary to mask text in Compose. * @@ -101,37 +100,24 @@ internal data class TextAttributes(val color: Color?, val hasFillModifier: Boole * string in their name, e.g. TextStringSimpleElement or TextAnnotatedStringElement. We then get the * color from the modifier, to be able to mask it with the correct color. * - * We also look up for classes that have a [Fill] modifier, usually they all have a `Fill` string in - * their name, e.g. FillElement. This is necessary to workaround a Compose bug where single-line - * text composable without a `fill` modifier still thinks that there's one and wrongly calculates - * horizontal position. - * * We also add special proguard rules to keep the `Text` class names and their `color` member. */ -internal fun LayoutNode.findTextAttributes(): TextAttributes { +internal fun LayoutNode.findTextColor(): Color? { val modifierInfos = getModifierInfo() - var color: Color? = null - var hasFillModifier = false for (index in modifierInfos.indices) { val modifier = modifierInfos[index].modifier val modifierClassName = modifier::class.java.name if (modifierClassName.contains("Text")) { - color = - try { - (modifier::class - .java - .getDeclaredField("color") - .apply { isAccessible = true } - .get(modifier) as? ColorProducer) - ?.invoke() - } catch (e: Throwable) { - null - } - } else if (modifierClassName.contains("Fill")) { - hasFillModifier = true + return try { + (modifier::class.java.getDeclaredField("color").apply { isAccessible = true }.get(modifier) + as? ColorProducer) + ?.invoke() + } catch (e: Throwable) { + null + } } } - return TextAttributes(color, hasFillModifier) + return null } /** diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/TextLayout.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/TextLayout.kt index 9b974efceaa..e62584bda06 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/TextLayout.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/TextLayout.kt @@ -13,15 +13,11 @@ internal interface TextLayout { */ val dominantTextColor: Int? - fun getPrimaryHorizontal(line: Int, offset: Int): Float + fun getLineLeft(line: Int): Float - fun getEllipsisCount(line: Int): Int - - fun getLineVisibleEnd(line: Int): Int + fun getLineRight(line: Int): Float fun getLineTop(line: Int): Int fun getLineBottom(line: Int): Int - - fun getLineStart(line: Int): Int } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt index 80ca1beee4e..d0583cdaa6a 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt @@ -128,21 +128,14 @@ internal fun TextLayout?.getVisibleRects( val rects = mutableListOf() for (i in 0 until lineCount) { - val lineStart = getPrimaryHorizontal(i, getLineStart(i)).toInt() - val ellipsisCount = getEllipsisCount(i) - val lineVisibleEnd = getLineVisibleEnd(i) - var lineEnd = - getPrimaryHorizontal(i, lineVisibleEnd - ellipsisCount + if (ellipsisCount > 0) 1 else 0) - .toInt() - if (lineEnd == 0 && lineVisibleEnd > 0) { - // looks like the case for when emojis are present in text - lineEnd = getPrimaryHorizontal(i, lineVisibleEnd - 1).toInt() + 1 - } + val lineLeft = getLineLeft(i).toInt() + val lineRight = getLineRight(i).toInt() val lineTop = getLineTop(i) val lineBottom = getLineBottom(i) val rect = Rect() - rect.left = globalRect.left + paddingLeft + lineStart - rect.right = rect.left + (lineEnd - lineStart) + + rect.left = globalRect.left + paddingLeft + lineLeft + rect.right = globalRect.left + paddingLeft + lineRight rect.top = globalRect.top + paddingTop + lineTop rect.bottom = rect.top + (lineBottom - lineTop) @@ -197,18 +190,30 @@ internal class AndroidTextLayout(private val layout: Layout) : TextLayout { return dominantColor?.toOpaque() } - override fun getPrimaryHorizontal(line: Int, offset: Int): Float = - layout.getPrimaryHorizontal(offset) - - override fun getEllipsisCount(line: Int): Int = layout.getEllipsisCount(line) + /** + * If text gets ellipsized, we return the left and right bounds of the ellipsized text instead of + * the width, as it's set to some obscure VERY_WIDE value. E.g. see + * https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/widget/TextView.java;l=468?q=VERY_WIDE + */ + override fun getLineLeft(line: Int): Float { + return if (layout.ellipsizedWidth > 0 && layout.ellipsizedWidth < layout.width) { + 0f + } else { + layout.getLineLeft(line) + } + } - override fun getLineVisibleEnd(line: Int): Int = layout.getLineVisibleEnd(line) + override fun getLineRight(line: Int): Float { + return if (layout.ellipsizedWidth > 0 && layout.ellipsizedWidth < layout.width) { + layout.ellipsizedWidth.toFloat() + } else { + layout.getLineRight(line) + } + } override fun getLineTop(line: Int): Int = layout.getLineTop(line) override fun getLineBottom(line: Int): Int = layout.getLineBottom(line) - - override fun getLineStart(line: Int): Int = layout.getLineStart(line) } internal fun View?.addOnDrawListenerSafe(listener: ViewTreeObserver.OnDrawListener) { diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt index f421ff9ad07..ec01d28d4fb 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt @@ -24,7 +24,7 @@ import io.sentry.android.replay.SentryReplayModifiers import io.sentry.android.replay.util.ComposeTextLayout import io.sentry.android.replay.util.boundsInWindow import io.sentry.android.replay.util.findPainter -import io.sentry.android.replay.util.findTextAttributes +import io.sentry.android.replay.util.findTextColor import io.sentry.android.replay.util.isMaskable import io.sentry.android.replay.util.toOpaque import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.GenericViewHierarchyNode @@ -189,11 +189,10 @@ internal object ComposeViewHierarchyNode { ?.action ?.invoke(textLayoutResults) - val (color, hasFillModifier) = node.findTextAttributes() val textLayoutResult = textLayoutResults.firstOrNull() var textColor = textLayoutResult?.layoutInput?.style?.color if (textColor?.isUnspecified == true) { - textColor = color + textColor = node.findTextColor() } val isLaidOut = textLayoutResult?.layoutInput?.style?.fontSize != TextUnit.Unspecified // TODO: support editable text (currently there's a way to get @Composable's padding only @@ -202,7 +201,7 @@ internal object ComposeViewHierarchyNode { TextViewHierarchyNode( layout = if (textLayoutResult != null && !isEditable && isLaidOut) { - ComposeTextLayout(textLayoutResult, hasFillModifier) + ComposeTextLayout(textLayoutResult) } else { null },