Skip to content

recording: improvements to screenshot masking - #147

Merged
marandaneto merged 3 commits into
mainfrom
chore/screenshot-masking-improvements
Jun 26, 2024
Merged

marandaneto merged 3 commits into
mainfrom
chore/screenshot-masking-improvements

Conversation

@marandaneto

@marandaneto marandaneto commented Jun 26, 2024 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

  • only mask if there's any content (text, image)
  • added missing components that are placeholders for wireframes but not for screenshots

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

Comment on lines +181 to +204
if let webView = view as? WKWebView {
// since we cannot the webview content, if masking texts or images are enabled
// we mask the whole webview as well
if isAnyInputSensitive(webView) {
maskableWidgets.append(view.toAbsoluteRect(parent))
return
}
}

if let button = view as? UIButton {
if isButtonSensitive(button) {
maskableWidgets.append(view.toAbsoluteRect(parent))
return
}
}

if let theSwitch = view as? UISwitch {
if isSwitchSensitive(theSwitch) {
maskableWidgets.append(view.toAbsoluteRect(parent))
return
}
}

if let picker = view as? UIPickerView {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few missing items that are placeholders for wireframes but not for screenshots and can be sensitive

Comment on lines +299 to +314
private func hasText(_ text: String?) -> Bool {
if let text = text, !text.isEmpty {
return true
} else {
// if there's no text, there's nothing to mask
return false
}
}

private func isImageViewSensitive(_ view: UIImageView) -> Bool {
var isAsset = false
if let image = view.image {
isAsset = isAssetsImage(image)
} else {
// if there's no image, there's nothing to mask
return false

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if masking is enabled, only mask if there's actually any content (text|image)

@marandaneto
marandaneto marked this pull request as ready for review June 26, 2024 08:29
@marandaneto
marandaneto requested a review from a team June 26, 2024 08:29

@daibhin daibhin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random thought: I don't recall seeing any exit early conditions. I know rrweb will not check child nodes if a parent has been masked which could help some of the extra processing time here

@marandaneto

Copy link
Copy Markdown
Member Author

Random thought: I don't recall seeing any exit early conditions. I know rrweb will not check child nodes if a parent has been masked which could help some of the extra processing time here

findMaskableWidgets (screenshot masking) returns if any condition is met and doesn't check the children's list, so I mask the whole X-Y no matter what.
toWireframe indeed does not do it, but the reason is that a component can be composed of different types, eg a Button contains a Label but I have to cast to both types to read the property of each of them, so it's just easier to let it run thru the whole thing, but it's a place to improve if the wireframe has performance issues as well for sure, good point.

@marandaneto
marandaneto merged commit 9cf626d into main Jun 26, 2024
@marandaneto
marandaneto deleted the chore/screenshot-masking-improvements branch June 26, 2024 08:45
eakurnikov pushed a commit to trustwallet/posthog-ios that referenced this pull request Feb 6, 2025
@turnipdabeets turnipdabeets mentioned this pull request Nov 13, 2026
6 of 18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants