Closed janek closed 4 years ago
Merging #307 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #307 +/- ##
======================================
Coverage 52.7% 52.7%
======================================
Files 87 87
Lines 3161 3161
======================================
Hits 1666 1666
Misses 1495 1495
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update f1bfbc4...8c8ce75. Read the comment docs.
I looked into doing this as a UI test on both iOS and Mac, and it won't be viable for this test, as it won't let us check isDecelerating
value during a touch (after touch down and before touch up). I also don't see a way of mocking touches for this as a unit test in iOSTestTarget
- certainly the way it's mocked now doesn't work there.
Generally, I see your point, I'll do some more manual testing now to see if another way of proving this comes to mind.
This is problematic to cover with tests because we can't evaluate conditions during a touch on iOS (after touch down and before touch up). I got here by debugging with prints but I'm having trouble reproducing that now and I'm not 100% sure of this fix.
I'll close this unmerged for now.
If we ever reopen this topic, here are the print logs that should illustrate the problem:
scrollViewDidScroll; is decelerating: false
scrollViewDidScroll; is decelerating: false
(…)
scrollViewDidScroll; is decelerating: false
scrollViewDidScroll; is decelerating: false
< We let go of the touch >
scrollViewDidEndDragging; is Decelerating? false, willDecelerate? true
< The scrollView is still in a velocity scroll, so it continues scrolling >
scrollViewDidScroll; is decelerating: true
scrollViewDidScroll; is decelerating: true
(…)
scrollViewDidScroll; is decelerating: true
scrollViewDidScroll; is decelerating: true
< We touch again to stop the scroll>
< We lift our touch >
scrollViewDidEndDragging; is Decelerating? true, willDecelerate? false
Those are from iOS, so ground truth. The bug would be that isDecelerating
is false
on Android in the last line - but as I said, I had some trouble recreating that.
The code snippet to put in DemoApp to be able to recreate the logging setup:
class ViewController: UIViewController, UIScrollViewDelegate {
func scrollViewWillBeginDragging(_ scrollView: UIScrollView) {
return
}
let label = UILabel()
override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
let scrollView = UIScrollView(frame: CGRect(x: 200, y: 100, width: 300, height: 150))
scrollView.contentSize = CGSize(width: 6000, height: 300)
scrollView.backgroundColor = .white
scrollView.delegate = self
view.backgroundColor = UIColor(red: 0 / 255, green: 206 / 255, blue: 201 / 255, alpha: 1)
view.addSubview(scrollView)
}
func scrollViewDidEndDragging(_ scrollView: UIScrollView, willDecelerate decelerate: Bool) {
print("scrollViewDidEndDragging; is Decelerating? \(scrollView.isDecelerating), willDecelerate? \(decelerate)")
}
func scrollViewDidScroll(_ scrollView: UIScrollView) {
print("scrollViewDidScroll; is decelerating: \(scrollView.isDecelerating)")
}
}
Type of change: Bug fix
Motivation (current vs expected behavior)
When comparing behaviour with iOS, I realized that the
isDecelerating
value forscrollView
is not being reported the same.The scenario was: one "fast" pan that starts a velocity scroll, then another touch that stops it, maybe pans around a bit, and releases.
As soon as the first pan is released and the velocity scroll starts,
isDecelerating
istrue
. Then when the second touch starts, the visible deceleration stops and we are reportingisDecelerating == false
. This seems reasonable, but it's not what iOS does. On iOS, the scrollView is still marked as decelerating, until the touch is released.This PR aligns behavior with iOS and adds a test that covers this case.
Warning
I realized that the difference in behaviour could come from the LongPressRecognizer that's there in iOS (but not yet in UIKit-crossplatform). If that's the case, this is likely not the right fix - please keep that in mind and let me know.
Please check if the PR fulfills these requirements