-
Notifications
You must be signed in to change notification settings - Fork 84
fix(replay): correct platform view mask geometry and capture-failure fallback #547
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
Changes from all commits
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,5 @@ | ||
| --- | ||
| 'posthog_flutter': patch | ||
| --- | ||
|
|
||
| Fix session replay masking on screens with a platform view (map, WebView, camera preview). A revealed view (`maskAllPlatformViews = false`) no longer turns black when the native capture fails β the SDK keeps the Flutter pixels and logs the failure instead. The mask rect is now clipped to the ancestor clip chain, so it no longer spills past the view onto the widgets below. The platform view rects are also collected in the same frame as the widget mask rects, so a mask can no longer land a frame late over moved pixels. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,9 +300,12 @@ class ScreenshotCapturer { | |
| return; | ||
| } | ||
| try { | ||
| final rect = clippedPaintBounds(ro, ancestor); | ||
| // A view clipped away by an ancestor covers nothing, so it gets no rect. | ||
| if (rect.isEmpty) return; | ||
| final transform = ro.getTransformTo(ancestor); | ||
| final data = ElementData( | ||
| rect: ro.paintBounds, | ||
| rect: rect, | ||
| type: 'platformView', | ||
| transform: transform, | ||
| ); | ||
|
|
@@ -338,16 +341,20 @@ class ScreenshotCapturer { | |
| ) async { | ||
| final transform = viewRect.transform; | ||
| if (transform == null) return; | ||
| final transformedRect = MatrixUtils.transformRect(transform, viewRect.rect); | ||
| // The user chose to reveal this view. A failed native capture must leave | ||
| // the Flutter pixels in place, never paint the mask they turned off. | ||
| if (bytes == null) { | ||
| _imageMaskPainter.drawMaskedImage(canvas, [viewRect], pixelRatio); | ||
| printIfDebug( | ||
| 'Native capture returned no bytes for a revealed platform view; keeping the Flutter pixels.'); | ||
| return; | ||
| } | ||
| final nativeImage = await _decodeRawPixels(bytes, nativeW, nativeH); | ||
| if (nativeImage == null) { | ||
| _imageMaskPainter.drawMaskedImage(canvas, [viewRect], pixelRatio); | ||
| printIfDebug( | ||
| 'Failed to decode the native capture for a revealed platform view; keeping the Flutter pixels.'); | ||
| return; | ||
| } | ||
| final transformedRect = MatrixUtils.transformRect(transform, viewRect.rect); | ||
| canvas.drawImageRect( | ||
| nativeImage, | ||
| Rect.fromLTWH( | ||
|
|
@@ -595,6 +602,16 @@ class ScreenshotCapturer { | |
| return; | ||
| } | ||
|
|
||
| // Collect the platform view rects in the same frame as the widget mask | ||
| // rects, before any await. Collecting them after toImage() lets the UI | ||
| // move first, so a mask lands a frame late over the wrong pixels. | ||
| final defaultPolicy = replayConfig.maskAllPlatformViews | ||
| ? PostHogPlatformViewPrivacy.mask | ||
| : PostHogPlatformViewPrivacy.capture; | ||
| final pvRects = _collectPlatformViewRects(defaultPolicy); | ||
| final hasCapturedViews = pvRects.captured.isNotEmpty; | ||
| hasCapturedPlatformViews = hasCapturedViews; | ||
|
|
||
|
Comment on lines
+605
to
+614
Contributor
Author
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. Early rect collection makes native capture requests staleWhy we think it's a valid issue
Issue descriptionThe code collects captured platform view rects before Suggested fixSeparate mask geometry from native capture geometry. Keep the frame-aligned rects for masking and final placement. Refresh each native view's screen bounds immediately before Prompt to fix with AI (copy-paste)
Contributor
Author
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 is a real tradeoff introduced by the frame-alignment fix, and I'm escalating it rather than fixing it unattended. Confirmed on the current head: the revealed platform-view rects are collected before The reason this isn't a safe autonomous fix:
What a human needs to decide: whether to take on the dual-geometry refresh (retain RenderObjects / re-walk to re-query bounds at capture time, match results back to placement rects, drop on mismatch), and to validate it on-device against a scrolling map/WebView β the exact E2E this autonomous run can't perform. |
||
| image = await renderObject.toImage(pixelRatio: pixelRatio); | ||
|
|
||
| final currentImage = image; | ||
|
|
@@ -651,13 +668,6 @@ class ScreenshotCapturer { | |
| final preMaskHash = _computeImageHash(imageBytes); | ||
| imageBytes = null; | ||
|
|
||
| final defaultPolicy = replayConfig.maskAllPlatformViews | ||
| ? PostHogPlatformViewPrivacy.mask | ||
| : PostHogPlatformViewPrivacy.capture; | ||
| final pvRects = _collectPlatformViewRects(defaultPolicy); | ||
| final hasCapturedViews = pvRects.captured.isNotEmpty; | ||
| hasCapturedPlatformViews = hasCapturedViews; | ||
|
|
||
| if (!hasCapturedViews && preMaskHash == statusView.imageBytesHash) { | ||
| printIfDebug( | ||
| 'Snapshot is the same as the last one, nothing changed, do nothing.', | ||
|
|
@@ -820,6 +830,33 @@ class ScreenshotCapturer { | |
| } | ||
| } | ||
|
|
||
| /// Intersects [ro]'s paint bounds with every clip its ancestors apply, up to | ||
| /// but not including [ancestor], and returns the visible rect in [ro]'s local | ||
| /// coordinates. A platform view reports its full, unclipped paint bounds, so a | ||
| /// map inside a scroll view or a `ClipRect` would otherwise place a mask past | ||
| /// the visible edge and over the widgets below. Returns [Rect.zero] when the | ||
| /// view is fully clipped away. | ||
| @visibleForTesting | ||
| Rect clippedPaintBounds(RenderBox ro, RenderObject? ancestor) { | ||
| var clipped = ro.paintBounds; | ||
| RenderObject child = ro; | ||
| RenderObject? node = ro.parent; | ||
| while (node != null && !identical(node, ancestor)) { | ||
| final clip = node.describeApproximatePaintClip(child); | ||
| if (clip != null) { | ||
| // The clip is in node's coordinates; map it into ro's frame. | ||
| final toRo = Matrix4.tryInvert(ro.getTransformTo(node)); | ||
| if (toRo != null) { | ||
| clipped = clipped.intersect(MatrixUtils.transformRect(toRo, clip)); | ||
| if (clipped.isEmpty) return Rect.zero; | ||
| } | ||
| } | ||
| child = node; | ||
| node = node.parent; | ||
| } | ||
| return clipped; | ||
| } | ||
|
|
||
| @visibleForTesting | ||
| PostHogPlatformViewPrivacy resolvePrivacyPolicyForElement( | ||
| Element element, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import 'package:flutter/rendering.dart'; | ||
| import 'package:flutter/widgets.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:posthog_flutter/src/replay/screenshot/screenshot_capturer.dart'; | ||
|
|
||
| void main() { | ||
| group('clippedPaintBounds β ancestor clip intersection', () { | ||
| testWidgets('an unclipped view keeps its full paint bounds', | ||
| (tester) async { | ||
| await tester.pumpWidget( | ||
| const Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: Center( | ||
| child: SizedBox( | ||
| key: Key('ancestor'), | ||
| width: 300, | ||
| height: 300, | ||
| child: Center( | ||
| child: SizedBox(key: Key('view'), width: 200, height: 200), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| final view = | ||
| tester.renderObject<RenderBox>(find.byKey(const Key('view'))); | ||
| final ancestor = | ||
| tester.renderObject<RenderBox>(find.byKey(const Key('ancestor'))); | ||
|
|
||
| expect(clippedPaintBounds(view, ancestor), | ||
| const Rect.fromLTWH(0, 0, 200, 200)); | ||
| }); | ||
|
|
||
| testWidgets('a ClipRect ancestor trims the view to the visible region', | ||
| (tester) async { | ||
| await tester.pumpWidget( | ||
| const Directionality( | ||
| textDirection: TextDirection.ltr, | ||
| child: Center( | ||
| child: SizedBox( | ||
| key: Key('ancestor'), | ||
| width: 100, | ||
| height: 100, | ||
| child: ClipRect( | ||
| child: OverflowBox( | ||
| alignment: Alignment.topLeft, | ||
| maxWidth: 300, | ||
| maxHeight: 300, | ||
| child: SizedBox(key: Key('view'), width: 300, height: 300), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| final view = | ||
| tester.renderObject<RenderBox>(find.byKey(const Key('view'))); | ||
| final ancestor = | ||
| tester.renderObject<RenderBox>(find.byKey(const Key('ancestor'))); | ||
|
|
||
| // The view paints 300x300 but the ClipRect only shows the top-left 100x100. | ||
| expect(clippedPaintBounds(view, ancestor), | ||
| const Rect.fromLTWH(0, 0, 100, 100)); | ||
| }); | ||
| }); | ||
| } |
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.
Clipped capture rect prevents native platform view capture
Why we think it's a valid issue
_addIfNewatposthog_flutter/lib/src/replay/screenshot/screenshot_capturer.dart:303-311storesclippedPaintBounds(ro, ancestor)inElementData.rect;_viewSpecatscreenshot_capturer.dart:322-332builds thex/y/width/heightpayload from that samerect; line 712-717 sends it tocaptureNativeScreenshots. So the clipped rect, not the full view rect, is what the native side receives.captureOneNativelooks up the target withfindWKWebView(in: window, containedBy: cropRect)(posthog_flutter/darwin/posthog_flutter/Sources/posthog_flutter/PosthogFlutterPlugin.swift:704), and the predicate isrect.insetBy(dx: -1, dy: -1).contains(frameInWindow)(PosthogFlutterPlugin.swift:786). One point of slack only. A crop rect trimmed by an ancestor clip is smaller than the WKWebView frame, so no web view matches and the function falls through toonResult(nil)atPosthogFlutterPlugin.swift:723.compositeSurfaceViewsOntoskips any SurfaceView that extends past the destination bitmap:destX + svLogW > destBitmap.width + tolerance || destY + svLogH > destBitmap.height + tolerancewithtolerance = 8(posthog_flutter/android/src/main/kotlin/com/posthog/flutter/PosthogFlutterPlugin.kt:1430-1434).destBitmapis sized from the requestedwidth/height(PosthogFlutterPlugin.kt:1258-1263), so a clip that removes more than 8 logical pixels makes the platform-view SurfaceView fail the test and get dropped from the composite._compositeRevealedView(screenshot_capturer.dart:346-356) return early on a null or undecodable capture and keep the Flutter pixels. The Flutter pixels do not contain the native view content on these composition modes, which is the reason the native capture exists.useAndroidViewSurface) inside aClipRect, or partly scrolled inside a viewport, withmaskAllPlatformViews = falseor a per-viewcapturepolicy. Result: iOS getsnilbytes, Android gets a composite without the SurfaceView, and the replay frame shows a blank region where the view is. Before this change the unclipped rect satisfied both containment checks and the content was captured (it then spilled past the clip, which is the bug this PR set out to fix). The two changes in this PR interact: the clipped request rect causes the capture failure, and the new fallback then silently keeps blank Flutter pixels instead of the old black mask, so the breakage is quiet.should_fix.maskAllPlatformViewsdefaults totrue(posthog_flutter/lib/src/posthog_config.dart:728), so the affected configuration is opt-in, and the outcome is lost replay fidelity, not a privacy leak, data loss, or a crash. It is still a real regression in the exact scenario the PR targets and should be resolved before the reveal path is advertised as fixed.Issue description
The code uses the clipped rect for the native capture request. The native implementations require the request rect to contain the full platform view. A clipped WebView or SurfaceView fails this check. The replay then keeps the Flutter pixels, which do not contain the native view on affected composition modes. Revealed clipped views therefore remain blank or stale.
Suggested fix
Keep the full platform view bounds and the visible clipped bounds. Use the full bounds to identify the native view. Capture only the visible intersection, or capture the identified view and crop it safely. Composite the result into the clipped destination rect. Add capture tests for a revealed WebView and SurfaceView inside a ClipRect.
Prompt to fix with AI (copy-paste)
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.
Confirmed: this is a real regression this PR introduces, and I'm escalating it for a human rather than fixing it unattended.
The capture path sends the clipped rect to native.
_addIfNewstoresclippedPaintBoundsinElementData.rect, and revealed (captured) views build their native request from that same rect (_viewSpecβcaptureNativeScreenshots). But both native sides deliberately require the request rect to contain the full platform view:findWKWebView(in:containedBy:)matches only whencropRect.insetBy(-1,-1)contains the WKWebView frame (1pt slack) β a clip-trimmed rect is smaller than the frame, so nothing matches and it returns nil. This containment rule is intentional: it stops a neighboring masked web view from being snapshotted and leaked.So a revealed platform view inside a
ClipRect(or partly scrolled in a viewport) withmaskAllPlatformViews = falsefails the native capture, and the new capture-failure fallback then keeps the blank Flutter pixels β a quiet loss of the exact content the reveal path exists to show.What a human needs to decide and verify on-device:
ElementDataβ the full (unclipped) paint bounds for the native request/identification, and the clipped bounds for masking and for the composite destination._compositeRevealedViewright so the revealed content lands correctly and still doesn't spill past the clip (the original bug this PR fixed).Why I'm not doing it here: this environment has no Dart/Flutter toolchain (both
flutteranddartare missing), so I can't runflutter analyzeor the tests, and the fix's correctness is fundamentally native + visual β WKWebView snapshot geometry, Android SurfaceView compositing, and the on-canvas crop β which can only be validated by running on a real iOS and Android device. It needs the capture tests the reviewer asked for (revealed WebView and SurfaceView inside aClipRect), exercised in CI/on-device.