REPLAY-1330 Support `UIDatePicker` elements in SR (date & time selection) #1204

ncreated · 2023-03-10T14:11:36Z

What and why?

📦 This PR enhances the recording of UIDatePicker elements in session replay - in all modes (date, time, date & time) and all styles (compact, inline, wheels). Tested on iOS 13.7, 14.5, 15.5. Snapshots generated for 16.2:

Date selection:

With no masking & light mode:	With masking & dark mode:

Time selection:

With no masking & light mode:	With masking & dark mode:

How?

At the highest level, UIDatePickerRecorder uses one of 3 private recorders depending on its datePickerStyle:

internal struct UIDatePickerRecorder: NodeRecorder {
    private let wheelsStyleRecorder = WheelsStyleDatePickerRecorder()
    private let compactStyleRecorder = CompactStyleDatePickerRecorder()
    private let inlineStyleRecorder = InlineStyleDatePickerRecorder()

    // ...
}

The WheelsStyleDatePickerRecorder uses UIPickerViewRecorder (which is how UIKit handles the .wheels mode). I was able to reuse 90% of work done for picker views - but the strategy of choosing "selected labels" turned out to be flaky. I enhanced it by much better predicate that depends solely on transform3D and labels visibility:

if attributes.isTranslucent || !CATransform3DIsIdentity(view.transform3D) {
   // If this view has any 3D effect applied, ignore it and do not enter its subtree:
   return IgnoredElement(subtreeStrategy: .ignore)
}

Other than this, I had to improve text obfuscation logic. While we want to obfuscate labels in UIPickerViewRecorder, according to product definition we don't want this if that recorder is ran for WheelsStyleDatePickerRecorder subtree. What I propose in this PR is having 3 separate text obfuscators in the context propagated during view-tree traversal. Parent recorders can now adjust selectionTextObfuscator accordingly before passing it to child recorders 👌:

internal struct ViewTreeRecordingContext {
    /// ...

    /// Text obfuscator applied to all non-sensitive texts. No-op if privacy mode is disabled.
    var textObfuscator: TextObfuscating
    /// Text obfuscator applied to user selection texts (such as labels in picker control).
    var selectionTextObfuscator: TextObfuscating
    /// Text obfuscator applied to all sensitive texts (such as passwords or e-mail address).
    let sensitiveTextObfuscator: TextObfuscating
}

Last, I did few changes to SRSnapshotTests project so it can be ran on iOS 13.4 and above. Snapshot testing is still limited to iOS 16.2, but it is possible to generate images manually for older ones.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests
Run integration tests
Run smoke tests

maciejburda

Amazing work really!

It's easy to follow the solution and learn about the edge cases.

Left some minor comments 🙌

maciejburda · 2023-03-13T12:19:08Z

DatadogSessionReplay/SRSnapshotTests/SRSnapshotTests.xcodeproj/project.pbxproj

@@ -439,6 +443,7 @@
 				INFOPLIST_KEY_UIMainStoryboardFile = Main;
 				INFOPLIST_KEY_UISupportedInterfaceOrientations = UIInterfaceOrientationPortrait;
 				INFOPLIST_KEY_UISupportedInterfaceOrientations_iPad = "UIInterfaceOrientationLandscapeLeft UIInterfaceOrientationLandscapeRight UIInterfaceOrientationPortrait UIInterfaceOrientationPortraitUpsideDown";
+				IPHONEOS_DEPLOYMENT_TARGET = 13.4;


Great! Is there a hard constraint on having 13.0 here?

I thought there is some, but turned out there is not 🙂. Changed to 13.0 👍

maciejburda · 2023-03-13T12:19:47Z

DatadogSessionReplay/SRSnapshotTests/SRSnapshotTests/Utils/SnapshotTestCase.swift

@@ -63,6 +63,14 @@ internal class SnapshotTestCase: XCTestCase {
        return createSideBySideImage(appImage, wireframesImage)
    }

+    func wait(seconds: TimeInterval) {


maybe could be a part of TestUtilities ?

If only we will need it anywhere else, we can move it. I would add things to TestUtilities only when we see opportunity - otherwise it would become bloated.

maciejburda · 2023-03-13T12:22:05Z

.../Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UIDatePickerRecorder.swift

+            nodes = wheelsStyleRecorder.recordNodes(of: datePicker, with: attributes, in: context)
+        }
+
+        let isDisplayedInPopover: Bool = {


Probably a style preference on my side, but it's a perfect candidate for nested function. IMO slightly more swiftly. Obviously not a blocker :)

maciejburda · 2023-03-13T12:24:03Z

.../Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UIDatePickerRecorder.swift

+                borderColor: isDisplayedInPopover ? SystemColors.secondarySystemFill : nil,
+                borderWidth: isDisplayedInPopover ? 1 : 0,
+                backgroundColor: isDisplayedInPopover ? SystemColors.secondarySystemGroupedBackground : SystemColors.systemBackground,
+                cornerRadius: 10,


Maybe worth getting that from the layer? 🤔

We can't - if date picker is shown in a popover, the background comes from parent's _UIVisualEffectBackdropView child, not the pickerView.layer itself. Doing arbitrary parent tree traversal would be flaky and only work on certain OSes (where popover is displayed). To solve this complexity, we use 10 as approximation.

maciejburda · 2023-03-13T12:24:53Z

...s/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UIImageViewRecorder.swift

-internal struct UIImageViewRecorder: NodeRecorder {
+internal class UIImageViewRecorder: NodeRecorder {
+    /// An option for overriding default semantics from parent recorder.
+    var semanticsOverride: (UIImageView, ViewAttributes) -> NodeSemantics? = { _, _ in nil }


maciejburda · 2023-03-13T12:26:53Z

...lay/Sources/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/ViewTreeSnapshotBuilder.swift

+        let className = "\(type(of: imageView))"
+        // This gets effective on iOS 15.0+ which is the earliest version that displays some elements in popover views.
+        // Here we explicitly ignore the "shadow" effect applied to popover.
+        let isSystemShadow = className == "_UICutoutShadowView"
+        return isSystemShadow ? IgnoredElement(subtreeStrategy: .ignore) : nil


It almost sounds like it should be default value of the semanticsOverride in the init.

We probably want to expand the coverage of non-essential images here as well.

Sounds fair 👍, moved this to default value on UIImageViewRecorder.

…S 13.0

…ll image recorders

ncreated added 2 commits March 10, 2023 14:47

REPLAY-1330 Add snapshot tests for UIDatePicker

e37bd0b

REPLAY-1330 Record UIDatePickers

4f96587

ncreated self-assigned this Mar 10, 2023

ncreated marked this pull request as ready for review March 10, 2023 14:59

ncreated requested a review from a team as a code owner March 10, 2023 14:59

maciejburda approved these changes Mar 13, 2023

View reviewed changes

ncreated added 2 commits March 21, 2023 13:34

REPLAY-1330 CR feedback - update SnapshotTest deployment target to iO…

682c340

…S 13.0

REPLAY-1330 CR feedback - ignore _UICutoutShadowView uniformly in a…

236555d

…ll image recorders

ncreated merged commit 4911a71 into develop Mar 21, 2023

ncreated deleted the ncreated/REPLAY-1330-support-date-pickers branch March 21, 2023 14:17

ncreated mentioned this pull request Mar 31, 2023

Dogfood recent changes #1232

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REPLAY-1330 Support `UIDatePicker` elements in SR (date & time selection) #1204

REPLAY-1330 Support `UIDatePicker` elements in SR (date & time selection) #1204

ncreated commented Mar 10, 2023 •

edited

Loading

maciejburda left a comment

maciejburda Mar 13, 2023

ncreated Mar 21, 2023

maciejburda Mar 13, 2023

ncreated Mar 21, 2023

maciejburda Mar 13, 2023

maciejburda Mar 13, 2023

ncreated Mar 21, 2023

maciejburda Mar 13, 2023

maciejburda Mar 13, 2023

ncreated Mar 21, 2023

REPLAY-1330 Support UIDatePicker elements in SR (date & time selection) #1204

REPLAY-1330 Support UIDatePicker elements in SR (date & time selection) #1204

Conversation

ncreated commented Mar 10, 2023 • edited Loading

What and why?

How?

Review checklist

Custom CI job configuration (optional)

maciejburda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

REPLAY-1330 Support `UIDatePicker` elements in SR (date & time selection) #1204

REPLAY-1330 Support `UIDatePicker` elements in SR (date & time selection) #1204

ncreated commented Mar 10, 2023 •

edited

Loading