-
Notifications
You must be signed in to change notification settings - Fork 81
RUM-16379: Fix dialog opening from another dialog in sr #3563
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
base: develop
Are you sure you want to change the base?
Changes from all commits
108296f
1fbb3c3
309e2b6
17a292a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. | ||
| * This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| * Copyright 2016-Present Datadog, Inc. | ||
| */ | ||
|
|
||
| package com.datadog.android.sessionreplay.internal.recorder | ||
|
|
||
| import android.annotation.SuppressLint | ||
| import android.view.View | ||
| import android.view.Window | ||
|
|
||
| @SuppressLint("PrivateApi") // intentional: accessing mWindow via reflection to get the Window from a decor view | ||
| internal object WindowReflectionUtils { | ||
|
|
||
| private const val WINDOW_FIELD_NAME = "mWindow" | ||
|
|
||
| fun getWindowFromDecorView(view: View): Window? { | ||
| return try { | ||
| var currentClass: Class<*>? = view.javaClass | ||
| while (currentClass != null) { | ||
| try { | ||
| @Suppress("UnsafeThirdPartyFunctionCall") // exceptions caught by outer try-catch | ||
| return currentClass.getDeclaredField(WINDOW_FIELD_NAME) | ||
| .also { it.isAccessible = true } | ||
| .get(view) as? Window | ||
| } catch (_: NoSuchFieldException) { | ||
| currentClass = currentClass.superclass | ||
| } | ||
| } | ||
| null | ||
| } catch (_: ReflectiveOperationException) { | ||
| null | ||
| } catch (_: SecurityException) { | ||
| null | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ package com.datadog.android.sessionreplay.internal.recorder.callback | |
| import android.content.Context | ||
| import android.graphics.Point | ||
| import android.view.MotionEvent | ||
| import android.view.View | ||
| import android.view.Window | ||
| import androidx.annotation.MainThread | ||
| import com.datadog.android.api.InternalLogger | ||
|
|
@@ -21,6 +22,7 @@ import com.datadog.android.sessionreplay.internal.TouchPrivacyManager | |
| import com.datadog.android.sessionreplay.internal.async.RecordedDataQueueHandler | ||
| import com.datadog.android.sessionreplay.internal.recorder.ViewOnDrawInterceptor | ||
| import com.datadog.android.sessionreplay.internal.recorder.WindowInspector | ||
| import com.datadog.android.sessionreplay.internal.recorder.WindowReflectionUtils | ||
| import com.datadog.android.sessionreplay.internal.utils.RumContextProvider | ||
| import com.datadog.android.sessionreplay.model.MobileSegment | ||
| import java.util.LinkedList | ||
|
|
@@ -45,8 +47,10 @@ internal class RecorderWindowCallback( | |
| private val motionEventUtils: MotionEventUtils = MotionEventUtils, | ||
| private val motionUpdateThresholdInNs: Long = MOTION_UPDATE_DELAY_THRESHOLD_NS, | ||
| private val flushPositionBufferThresholdInNs: Long = FLUSH_BUFFER_THRESHOLD_NS, | ||
| private val windowInspector: WindowInspector = WindowInspector | ||
| private val windowInspector: WindowInspector = WindowInspector, | ||
| private val windowFromDecorView: (View) -> Window? = { WindowReflectionUtils.getWindowFromDecorView(it) } | ||
| ) : FixedWindowCallback(wrappedCallback) { | ||
| private val appContext: Context = appContext | ||
| private val pixelsDensity = appContext.resources.displayMetrics.density | ||
| internal val pointerInteractions: MutableList<MobileSegment.MobileRecord> = LinkedList() | ||
| private var lastOnMoveUpdateTimeInNs: Long = 0L | ||
|
|
@@ -102,6 +106,7 @@ internal class RecorderWindowCallback( | |
| textAndInputPrivacy = privacy, | ||
| imagePrivacy = imagePrivacy | ||
| ) | ||
| installCallbackOnNewWindows(rootViews) | ||
| } | ||
| super.onWindowFocusChanged(hasFocus) | ||
| } | ||
|
|
@@ -110,6 +115,51 @@ internal class RecorderWindowCallback( | |
|
|
||
| // region Internal | ||
|
|
||
| private fun installCallbackOnNewWindows(rootViews: List<View>) { | ||
| rootViews.forEach { decorView -> | ||
| // Skip zero-size windows (NavHost scaffolding) — installing on them causes spurious | ||
| // stopIntercepting() calls that drop frames on every navigation event. | ||
| if (decorView.width == 0 || decorView.height == 0) return@forEach | ||
| val window = windowFromDecorView(decorView) | ||
| if (window == null) { | ||
| internalLogger.log( | ||
| InternalLogger.Level.WARN, | ||
| InternalLogger.Target.MAINTAINER, | ||
| { | ||
| WINDOW_FROM_DECOR_VIEW_ERROR_MESSAGE_PREFIX + | ||
| decorView.javaClass.name + | ||
| WINDOW_FROM_DECOR_VIEW_ERROR_MESSAGE_SUFFIX | ||
| }, | ||
| onlyOnce = true | ||
| ) | ||
| return@forEach | ||
| } | ||
| if (window.callback !is RecorderWindowCallback) { | ||
| // Post so the dialog's own onWindowFocusChanged(true) fires first, | ||
| // ensuring the new callback starts in a steady recording state. | ||
| decorView.post { | ||
| if (window.callback !is RecorderWindowCallback) { | ||
| val toWrap = window.callback ?: NoOpWindowCallback() | ||
| window.callback = RecorderWindowCallback( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new path installs a Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this installs the same callback on an offset dialog, Useful? React with 👍 / 👎. |
||
| appContext = appContext, | ||
| recordedDataQueueHandler = recordedDataQueueHandler, | ||
| wrappedCallback = toWrap, | ||
| timeProvider = timeProvider, | ||
| rumContextProvider = rumContextProvider, | ||
| viewOnDrawInterceptor = viewOnDrawInterceptor, | ||
| internalLogger = internalLogger, | ||
| privacy = privacy, | ||
| imagePrivacy = imagePrivacy, | ||
| touchPrivacyManager = touchPrivacyManager, | ||
| windowInspector = windowInspector, | ||
| windowFromDecorView = windowFromDecorView | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @MainThread | ||
| private fun handleEvent(event: MotionEvent) { | ||
| when (event.action.and(MotionEvent.ACTION_MASK)) { | ||
|
|
@@ -215,5 +265,9 @@ internal class RecorderWindowCallback( | |
| "RecorderWindowCallback: intercepted null motion event" | ||
| internal const val FAIL_TO_PROCESS_MOTION_EVENT_ERROR_MESSAGE = | ||
| "RecorderWindowCallback: wrapped callback failed to handle the motion event" | ||
| internal const val WINDOW_FROM_DECOR_VIEW_ERROR_MESSAGE_PREFIX = | ||
| "SR: failed to get Window from " | ||
| internal const val WINDOW_FROM_DECOR_VIEW_ERROR_MESSAGE_SUFFIX = | ||
| " via reflection — Compose dialog {} destination may not be recorded" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /* | ||
| * Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. | ||
| * This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| * Copyright 2016-Present Datadog, Inc. | ||
| */ | ||
|
|
||
| package com.datadog.android.sessionreplay.internal.recorder | ||
|
|
||
| import android.view.View | ||
| import android.view.Window | ||
| import com.datadog.android.sessionreplay.forge.ForgeConfigurator | ||
| import fr.xgouchet.elmyr.junit5.ForgeConfiguration | ||
| import fr.xgouchet.elmyr.junit5.ForgeExtension | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.jupiter.api.Test | ||
| import org.junit.jupiter.api.extension.ExtendWith | ||
| import org.junit.jupiter.api.extension.Extensions | ||
| import org.mockito.junit.jupiter.MockitoExtension | ||
| import org.mockito.junit.jupiter.MockitoSettings | ||
| import org.mockito.kotlin.mock | ||
| import org.mockito.quality.Strictness | ||
|
|
||
| @Extensions( | ||
| ExtendWith(MockitoExtension::class), | ||
| ExtendWith(ForgeExtension::class) | ||
| ) | ||
| @MockitoSettings(strictness = Strictness.LENIENT) | ||
| @ForgeConfiguration(ForgeConfigurator::class) | ||
| internal class WindowReflectionUtilsTest { | ||
|
|
||
| @Test | ||
| fun `M return null W getWindowFromDecorView {view has no mWindow field}`() { | ||
| assertThat(WindowReflectionUtils.getWindowFromDecorView(mock())).isNull() | ||
| } | ||
|
|
||
| @Test | ||
| fun `M return Window W getWindowFromDecorView {view class has mWindow field}`() { | ||
| // Given — mimics DecorView's private mWindow field | ||
| val fakeWindow = mock<Window>() | ||
| val fakeDecorLike = object : View(mock()) { | ||
| @Suppress("unused") | ||
| private val mWindow: Window = fakeWindow | ||
| } | ||
|
|
||
| // When + Then | ||
| assertThat(WindowReflectionUtils.getWindowFromDecorView(fakeDecorLike)).isSameAs(fakeWindow) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a Compose dialog contains an
AndroidView/interop node,RootSemanticsNodeMapperdelegates it tomappingContext.interopViewCallback.map(), which traverses the Android View with the normal view mappers andDefaultViewBoundsResolver.getLocationOnScreen(), so those wireframes are already screen-absolute. Offsetting the entire returned list here shifts those interop wireframes by the dialog position a second time, causing embedded Android Views inside offset Compose roots/dialogs to appear in the wrong location.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move this change into it's own pr