Add the ability to force the cursor to stay visible in snapshots for all text fields #17
Conversation
…all text fields Fixes issue cashapp#15
|
|
||
| // Set the speed and time offset of the layer such that the cursor will remain visible | ||
| fieldEditorLayer.speed = 0 | ||
| fieldEditorLayer.timeOffset = fieldEditorLayer.beginTime |
There was a problem hiding this comment.
Let's make sure to restore the original values here to avoid potentially breaking any other tests associated with the view. Maybe build up a set of (layer: CALayer, speed: Float, timeOffset: CFTimeInterval) here and then go through and restore the values after the snapshot?
I'm not sure the best way to pause/resume this layer animation is. We might want to always set timeOffset to 0 here, and then set beginTime to the original beginTime + timeOffset when we restore it? Might need to do a bit of research to see exactly how that works.
There was a problem hiding this comment.
Added a mechanism to resume the animation after the snapshot completes
| @@ -38,6 +39,7 @@ extension FBSnapshotTestCase { | |||
| public func SnapshotVerifyAccessibility( | |||
| _ view: UIView, | |||
| identifier: String = "", | |||
| forceCursorVisible: Bool = true, | |||
There was a problem hiding this comment.
I'm tempted to say this is a patch change since it addresses flaky tests, but I think defaulting this to true makes it a breaking change, since tests can start failing if consumers already had a different fix in place - for example, pausing the animation in a similar way except setting the offset to a point where the cursor is hidden.
To be clear, making a breaking change is totally fine, just wanted to call it out here.
There was a problem hiding this comment.
Agreed, probably a breaking change.
Fix snapshot tests
…ot test completes
| var pausedTextFieldLayers = [CALayer]() | ||
| if forceCursorVisible { | ||
| // Show the cursor of all text fields. | ||
| view.recursiveForEach(viewType: UITextField.self) { textField in |
There was a problem hiding this comment.
Realizing this isn't quite the behavior we want actually. Going to iterate on this a bit more.
There was a problem hiding this comment.
Hmm... I'm not sure exactly what this will do right now. I think setting it to the converted CACurrentMediaTime() is going to pause the animation in its current position. Instead we probably want something like:
func pauseAnimation() -> (_ speed: Float, _ timeOffset: CFTimeInterval) {
let originalValues = (speed, timeOffset)
speed = 0
timeOffset = 0
return originalValues
}
func resumeAnimation(originalSpeed: Float, originalTimeOffset: CFTimeInterval) {
timeOffset = originalTimeOffset
speed = originalSpeed
}
Fixes issue #15