feat: add user interactions autocapture #186

PouriaAmini · 2024-07-01T19:27:03Z

Summary

This PR adds support for the auto-capture of user interactions with UI elements. The design is heavily based on the accessibility metadata available within the app for each UI element, which is mostly provided by the UI framework. Auto-capture of user interactions works for apps built with both UIKit and SwiftUI. To enable user interaction auto-capture, set the userInteractions parameter of the DefaultTrackingOptions initializer to true:

let amplitude = Amplitude(configuration: Configuration(
  apiKey: "API_KEY",
  defaultTracking: DefaultTrackingOptions(userInteractions = true)
))

New Automatically Captured Event

[Amplitude] User Interaction: Emitted when a user interacts with a UI element.

Note: To capture the event, the UI element must include some accessibility metadata (accessibilityLabel, accessibilityValue, and/or accessibilityTraits). Otherwise, the interaction event will be generic and will not contain information about the UI element with which the user interacted.

New Automatically Captured Properties

[Amplitude] Element Label: The accessibility label of the UI element the user interacted with. If empty, the property will be excluded.
[Amplitude] Element Value: The accessibility value of the UI element the user interacted with. If empty, the property will be excluded.
[Amplitude] Element Type: The accessibility traits of the UI element the user interacted with. If empty, the property will be excluded.
[Amplitude] Interaction: The type of the user interaction on the UI element. These interactions include:
- Tap
- Dead Tap: When a user clicks on an inactive button
- Long Press
- Dead Long Press
- Rage Tap
- Focus Gained (for text fields)
- Focus Lost (for text fields)
- Focus Lost After Text Modification (for text fields)
- Value Changed To -% (for sliders)

PR Note: Tests targeting these implementations and Objective-C support will be included in a separate PR to reduce the size of this PR.

Checklist

Does your PR title have the correct title format?
Does your PR have a breaking change?: no

PouriaAmini · 2024-07-01T19:28:12Z

Sources/Amplitude/Plugins/iOS/IOSLifecycleMonitor.swift

@@ -7,131 +7,137 @@



Most of the changes to this file are to correct the indentation.

Please check in any style related changes separately.

PouriaAmini · 2024-07-01T19:30:04Z

Sources/Amplitude/Plugins/iOS/IOSLifecycleMonitor.swift

+    private var application: UIApplication? {
+        // TODO: Check if lifecycle plugin works for app extension
+        // App extensions can't use UIApplication.shared, so
+        // funnel it through something to check; Could be nil.
+        UIApplication.value(forKeyPath: "sharedApplication") as? UIApplication
+    }


Wrapped the logic to get the UIApplication.shared in a computed property since UIApplication.shared is not available during the app startup.

? That should not be the case.

I've tried accessing the application instance later when the app starts and with the previous code, the instance is always nil.

crleona

1/ Especially on your first few commits to a new language / project, please try to keep PRs small - this is too much code to effectively review in one go.
2/ I am concerned with the overall capture approach - while it is nice that it works for both Swift and SwiftUI, most apps do not put a lot of thought into a11y, and we are ignoring much of the context we can get from a closer integration with UIKit. Things like respecting gesture recognizer failure hierarchies are important to understanding which events actually fired.

crleona · 2024-07-01T19:56:40Z

Sources/Amplitude/DefaultTrackingOptions.swift

+    public convenience init (
+        sessions: Bool = true,
+        appLifecycles: Bool = false,
+        screenViews: Bool = false,
+        userInteractions: Bool = false
+    ) {
+        self.init(sessions: sessions, appLifecycles: appLifecycles, screenViews: screenViews)
+        self.userInteractions = userInteractions


Is there a reason you are adding a new convenience initializer vs just adding userInteractions to the default initializer?

This was to bypass an error related to the new parameter of the original initializer. I'll look into a different way to fix that.

crleona · 2024-07-01T19:58:43Z

Sources/Amplitude/Events/UserInteractionEvent.swift

+extension UIAccessibilityTraits {
+    func stringify() -> String? {
+        var strings = [String]()
+        if contains(.adjustable) { strings.append("Adjustable") }
+        if contains(.allowsDirectInteraction) { strings.append("Allows Direct Interaction") }
+        if contains(.button) { strings.append("Button") }
+        if contains(.causesPageTurn) { strings.append("Causes Page Turn") }
+        if contains(.header) { strings.append("Header") }
+        if contains(.image) { strings.append("Image") }
+        if contains(.keyboardKey) { strings.append("Keyboard Key") }
+        if contains(.link) { strings.append("Link") }
+        if contains(.notEnabled) { strings.append("Not Enabled") }
+        if contains(.playsSound) { strings.append("Plays Sound") }
+        if contains(.searchField) { strings.append("Search Field") }
+        if contains(.selected) { strings.append("Selected") }
+        if contains(.startsMediaSession) { strings.append("Starts Media Session") }
+        if contains(.staticText) { strings.append("Static Text") }
+        if contains(.summaryElement) { strings.append("Summary Element") }
+        if contains(.tabBar) { strings.append("Tab Bar") }
+        if contains(.updatesFrequently) { strings.append("Updates Frequently") }
+
+        return strings.isEmpty ? nil : strings.joined(separator: ", ")
+    }
+}


There's not a lot here that's informative in any sort of analytical sense, we should not collect this.

I agree that some of them are unnecessary, but traits like button, link, tab, and search field can provide semantics info about the element that was clicked. Is that something that we would want to capture?

I think, in many cases, this data will be used to help uniquely identify an auto captured event - something like "signup button clicked".
So, if we have more specific indicators like an accessibility label, I'm not sure this is useful to capture. If information is a lot more sparse and this would be a useful indicator in telling one event from the other, then the above approach is probably the way to go.

crleona · 2024-07-01T20:00:42Z

Sources/Amplitude/Plugins/iOS/IOSLifecycleMonitor.swift

@@ -7,131 +7,137 @@



Please check in any style related changes separately.

crleona · 2024-07-01T20:03:21Z

.../AmplitudeSwiftUIExample.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

@@ -0,0 +1,23 @@
+{


This is probably a relic from including a local Amplitude-Swift in another project - please do not check this in.

crleona · 2024-07-01T20:07:48Z

Sources/Amplitude/Plugins/iOS/iOSUserInteractionCapture/CustomUITextFieldDelegate.swift

+
+import UIKit
+
+internal final class TextFieldDelegateWrapper: NSObject, UITextFieldDelegate {


Overriding delegates is extremely error prone and is not a technique I am comfortable releasing.

One other approach I tried to capture when a text field gains/loses focus was through figuring out location of the user clicks, but it had many edge cases. Is there a better approach that overriding the delegate?

Does UITextfield fire any relevant control events?

crleona · 2024-07-01T21:49:41Z

Sources/Amplitude/Plugins/iOS/iOSUserInteractionCapture/CustomUIGestureRecognizers.swift

+
+    // MARK: - Life Cycle
+
+    init(for captureDelegateHandler: @escaping () -> UserInteractionCaptureDelegate?) {


Why are you passing around lazy accessors like this? There is almost assuredly a better way to accomplish whatever you're trying to work around.

I was trying to get a weak reference to self. A capture list within a closure seemed to be the only way to do this. Is there a better way to accomplish this? (the weak reference here was to prevent CustomUIGestureRecognizers and UserInteractionCaptureDelegate have circular reference.)

Ah, this is probably a good time to learn the delegate pattern :) - see https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/DelegatesandDataSources/DelegatesandDataSources.html, or maybe https://medium.com/@afonso.script.sol/a-step-by-step-guide-to-the-delegate-pattern-in-swift-91a28de1baf8 for a more modern reference

crleona · 2024-07-01T21:56:02Z

Sources/Amplitude/Plugins/iOS/iOSUserInteractionCapture/CustomUIGestureRecognizers.swift

+
+// MARK: -
+
+internal final class GlobalUIPressGestureRecognizer: UIGestureRecognizer {


Are you actually using anything from UIGestureRecognizer in this class?

Nope, I can remove UIGestureRecognizer in this case.

crleona · 2024-07-01T21:59:12Z

Sources/Amplitude/Plugins/iOS/iOSUserInteractionCapture/UserInteractionCaptureDelegate.swift

+    /// The accessibility targets within `keyWindow`.
+    var accessibilityTargets: [AccessibilityTarget] {
+        guard let keyWindow else { return [] }
+        return accessibilityHierarchyParser.parseAccessibilityElements(in: keyWindow)


Why do you need to return the whole accessibility hierarchy when you only ever use it to find a single view?

Good call! Looking at it again, I think we can just get the first element under the tap - my initial concern was that multiple elements might be tapped with a single tap, but that doesn't seem to be an issue anymore since I'm already getting the first one.

crleona · 2024-07-01T22:23:04Z

Sources/Amplitude/Plugins/iOS/iOSUserInteractionCapture/CustomUIGestureRecognizers.swift

+    // MARK: - Private Properties
+
+    /// The activation delta between the initial touch point and the final touch point.
+    private static let pressActivationDelta: Double = 10


This is generally not what the system regards as a "press", please see the documentation for UIControl.Event.touchUpInside.

This delta seems to be the minimum difference between the initial and final touch points since anything more might cause a scroll or swipe if the touch was on a scrollable element. Although this might not detect some presses, it will avoid false positives where the change in touch positions was for a scroll, but a press is detected.

For something like UIButton, the default touchUpInside behavior would detect a touch down, swipe anywhere in bounds, then touch up over any period of time as a single event. I think auto tracking will replace manually firing an event from the default handler (ie, addTarget(_:action:for:) for UIButton), so we should get as close to this behavior as possible.

crleona · 2024-07-01T22:24:02Z

Sources/Amplitude/Plugins/iOS/iOSUserInteractionCapture/CustomUIGestureRecognizers.swift

+            let startTime,
+            let initialTarget,
+            let initialRootView,
+            let endTouchLocation = touches.first?.location(in: initialRootView),


What happens if there are multiple overlapping a11y elements? how would the system resolve this?

The first element from the hierarchy parser is the top most element.

PouriaAmini · 2024-07-02T00:01:54Z

1/ Especially on your first few commits to a new language / project, please try to keep PRs small - this is too much code to effectively review in one go. 2/ I am concerned with the overall capture approach - while it is nice that it works for both Swift and SwiftUI, most apps do not put a lot of thought into a11y, and we are ignoring much of the context we can get from a closer integration with UIKit. Things like respecting gesture recognizer failure hierarchies are important to understanding which events actually fired.

Thanks @crleona for the review!
1/ I'll convert this into a draft and try to break this code into multiple PRs.
2/ The primary reason why I was lenient towards extracting analytics from accessibility metadata was that many of the built-in component in SwiftUI already have descriptive accessibility data - for example any button that has a label will have an accessibility label by default. The primary reason for opting for global gesture recognition than swizzling concrete subclasses of UIGestureRecognizer was that many of the SwiftUI components don't use UIGestureRecognizer.

Mercy811

Can we merge this pr to a feature branch like we discussed?

Mercy811 · 2024-07-02T00:07:21Z

Sources/Amplitude/Constants.swift

@@ -83,6 +83,7 @@ public struct Constants {
    static let AMP_APPLICATION_BACKGROUNDED_EVENT = "\(AMP_AMPLITUDE_PREFIX)Application Backgrounded"
    static let AMP_DEEP_LINK_OPENED_EVENT = "\(AMP_AMPLITUDE_PREFIX)Deep Link Opened"
    static let AMP_SCREEN_VIEWED_EVENT = "\(AMP_AMPLITUDE_PREFIX)Screen Viewed"
+    static let AMP_USER_INTERACTION_EVENT = "\(AMP_AMPLITUDE_PREFIX)User Interaction"


Should we change the name to be "User Interacted" to follow the pattern of other default tracking event names?

We can discuss with Alan to align on the naming.

PouriaAmini · 2024-07-02T00:40:14Z

Can we merge this pr to a feature branch like we discussed?

@Mercy811 - I'm thinking that since we are going to break this PR into chunks, it would minimize its impact on the SDK. So I'm not sure if having a feature branch would make the introduction of the new feature more safe. Unless we want to test the implementation with a set of customers first before merging in the feature. I'm open to further discussion.

Mercy811 · 2024-07-02T00:40:29Z

Sources/Amplitude/Utilities/AccessibilityHierarchyParser.swift

+    public var shape: Shape
+
+    /// The object representing the accessibility node.
+    public var object: NSObject


Looks like these properties are not meant to change after initialization. If so, it makes more sense to use let instead of var.

Mercy811 · 2024-07-02T00:41:08Z

Sources/Amplitude/Utilities/AccessibilityHierarchyParser.swift

+    // MARK: - Life Cycle
+
+    public init() {}


Can we delete it if it doesn't perform any additional setup

crleona · 2024-07-02T01:17:13Z

2/ The primary reason why I was lenient towards extracting analytics from accessibility metadata was that many of the built-in component in SwiftUI already have descriptive accessibility data - for example any button that has a label will have an accessibility label by default. The primary reason for opting for global gesture recognition than swizzling concrete subclasses of UIGestureRecognizer was that many of the SwiftUI components don't use UIGestureRecognizer.

It does sound like this may be as good as we can get in SwiftUI, so we should keep this approach as a fallback - perhaps enabled by a different config if we ever run into trouble with loading a11y libs. I'd like to see where we can get with a UIKit only approach as well - there should be a lot more context we can gather, and we can get a closer mapping of tracked events to events that the app is actually taking action on.

Add user interactions autocapture

e612696

PouriaAmini self-assigned this Jul 1, 2024

PouriaAmini commented Jul 1, 2024

View reviewed changes

PouriaAmini requested review from crleona, Mercy811 and yuhao900914 July 1, 2024 19:30

PouriaAmini added 5 commits July 1, 2024 13:01

Fix lint issues

80d1271

Fix force_cast and large_tuple issues

5989c9f

Remove Package.resolved

ffab6f3

Remove Package.resolved

1d5f0c6

remove Package.resolved

078b44a

crleona requested changes Jul 1, 2024

View reviewed changes

PouriaAmini marked this pull request as draft July 2, 2024 00:16

Mercy811 reviewed Jul 2, 2024

View reviewed changes

PouriaAmini closed this Jul 2, 2024

PouriaAmini removed their assignment Jul 24, 2024


		import UIKit

		internal final class TextFieldDelegateWrapper: NSObject, UITextFieldDelegate {


		// MARK: - Life Cycle

		init(for captureDelegateHandler: @escaping () -> UserInteractionCaptureDelegate?) {


		// MARK: -

		internal final class GlobalUIPressGestureRecognizer: UIGestureRecognizer {

feat: add user interactions autocapture #186

feat: add user interactions autocapture #186

Conversation

PouriaAmini commented Jul 1, 2024 • edited Loading

Summary

New Automatically Captured Event

New Automatically Captured Properties

Checklist

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

crleona 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

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

PouriaAmini Jul 2, 2024 • edited Loading

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

PouriaAmini commented Jul 2, 2024

Mercy811 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PouriaAmini commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crleona commented Jul 2, 2024

PouriaAmini commented Jul 1, 2024 •

edited

Loading

PouriaAmini Jul 2, 2024 •

edited

Loading