Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use scrollView.contentOffset when stopping ongoing scroll deceleration #527

Merged
merged 2 commits into from
Feb 4, 2022

Conversation

dudek-j
Copy link
Contributor

@dudek-j dudek-j commented Jan 18, 2022

Applies fixes as described in #526

@scenee
Copy link
Owner

scenee commented Jan 22, 2022

Thank you for your PR. Unfortunately, I found a problem where the content offset of a tracking scroll view sometimes slips after I swipe a panel up from the half state to full state. I will upload a screen record later.

@dudek-j
Copy link
Contributor Author

dudek-j commented Jan 22, 2022

Please post your findings, I will try to further look into this issue.

@scenee
Copy link
Owner

scenee commented Jan 25, 2022

Here is a screen record when I reproduced this issue.

content-slips.mp4

I guess we might need to modify a condition where initialScrollOffset is set to a content offset to fix #526 🤔

@dudek-j
Copy link
Contributor Author

dudek-j commented Jan 25, 2022

I started investigating how initalScrollOffset is set. I am having a bit of trouble figuring out what the purpose of the contentOffsetForPinning(of: scrollView) on line 804 is

if grabberAreaFrame.contains(location) {
initialScrollOffset = scrollView.contentOffset
} else {
initialScrollOffset = contentOffsetForPinning(of: scrollView)
let offsetDiff = scrollView.contentOffset - contentOffsetForPinning(of: scrollView)
switch layoutAdapter.position {

I noticed that replacing it with initialScrollOffset = scrollView.contentOffset fixes #526 without causing the problems you showed above. I updated the PR accordingly.

@dudek-j
Copy link
Contributor Author

dudek-j commented Jan 25, 2022

In general it seems as if either func floatingPanel(FloatingPanelController, contentOffsetForPinning: UIScrollView) -> CGPoint is broken, or I am not able to figure out the intended effect.

@scenee
Copy link
Owner

scenee commented Jan 29, 2022

I confirmed your PR it's working well! And then thanks to your comments, I knew the root cause is Core.contentOffsetForPinning(of scrollView: UIScrollView) implementation. (Thank you so much 👍 )

So I pushed the commit of an alternative solution to update the behavior of Core.contentOffsetForPinning(of scrollView: UIScrollView).

This solution is a bit side effect but it works func floatingPanel(_ fpc: FloatingPanelController, contentOffsetForPinning trackingScrollView: UIScrollView) of FloatingPanelControllerDelegate is working well as before. As a result, a library consumer is able to fix the side effect using the delegate method.

@dudek-j
Copy link
Contributor Author

dudek-j commented Jan 31, 2022

Glad I could help!

Unfortunately I am getting some really strange behaviour after your changes to the implementation of Core.contentOffsetForPinning(of scrollView: UIScrollView) is changed. The scrollView of the panel is locked and no longer allows the user to get to the top. As shown in the example below, I am dragging on the tableView.

Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-01-31.at.12.30.18.mp4

@scenee
Copy link
Owner

scenee commented Jan 31, 2022

Oh my gosh... Does the example code implement func floatingPanel(_ fpc: FloatingPanelController, contentOffsetForPinning trackingScrollView: UIScrollView) delegate method? If not, 5181ccd should be logically the same as de34fda 🤔

@dudek-j
Copy link
Contributor Author

dudek-j commented Jan 31, 2022

Create a new StoryBoard project and paste this into the ViewController file, the example is as basic as possible.

I think the issue with: 5181ccd is that Core.contentOffsetForPinning(of scrollView: UIScrollView) is used in other places than just line 804.

Meanwhile the change in de34fda is localised too that single place.

///
//  ViewController.swift
//  FloatingPanelLab
//
//  Created by Jakub Dudek on 2022-01-18.
//

import UIKit
import FloatingPanel

class ViewController: UIViewController {

    private lazy var floatingPanelController: FloatingPanelController = {
        let fpc = FloatingPanelController()
        fpc.set(contentViewController: tableViewController)
        fpc.track(scrollView: tableViewController.tableView)
        return fpc
    }()
    
    private lazy var tableViewController: TableViewController = {
        let vc = TableViewController()
        return vc
    }()
    
    override func viewDidLoad() {
        super.viewDidLoad()
        view.backgroundColor = .yellow
    
        view.addSubview(floatingPanelController.view)
        floatingPanelController.view.frame = view.bounds
        addChild(floatingPanelController)
        floatingPanelController.didMove(toParent: self)
        floatingPanelController.show()
    }
    
}

class TableViewController: UIViewController, UITableViewDelegate, UITableViewDataSource {
    
    let tableView = UITableView()
    
    override func viewDidLoad() {
        super.viewDidLoad()
        
        view.backgroundColor = .red
        tableView.translatesAutoresizingMaskIntoConstraints = false
        tableView.dataSource = self
        tableView.delegate = self
        
        view.addSubview(tableView)
         
        NSLayoutConstraint.activate([ 
            tableView.topAnchor.constraint(equalTo: view.topAnchor),
            tableView.leadingAnchor.constraint(equalTo: view.leadingAnchor),
            tableView.trailingAnchor.constraint(equalTo: view.trailingAnchor),
            tableView.bottomAnchor.constraint(equalTo: view.bottomAnchor)
        ])
        
        tableView.register(UITableViewCell.self, forCellReuseIdentifier: "Cell")
    }
    
    func numberOfSections(in tableView: UITableView) -> Int {
        return 1
    }    
    
    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return 20
    }
    
    func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCell(withIdentifier: "Cell", for: indexPath)
        cell.textLabel?.text = "\(indexPath.row)"
        return cell
    }
    
} 

@dudek-j
Copy link
Contributor Author

dudek-j commented Jan 31, 2022

The issue is with Core.allowScrollPanGesture(for scrollView: UIScrollView).

    private func allowScrollPanGesture(for scrollView: UIScrollView) -> Bool {
        guard state == layoutAdapter.mostExpandedState else { return false }
        var offsetY: CGFloat = 0
        switch layoutAdapter.position {
        case .top, .left:
            offsetY = value(of: scrollView.fp_contentOffsetMax - scrollView.contentOffset)
        case .bottom, .right:
            offsetY = value(of: scrollView.contentOffset - contentOffsetForPinning(of: scrollView))
        }
        return offsetY <= -30.0 || offsetY > 0
    }

offsetY will always resolve to 0 with the current Core.contentOffsetForPinning(of scrollView: UIScrollView) implementation.

@scenee
Copy link
Owner

scenee commented Feb 4, 2022

Thanks to your detail comments, I've realized I misunderstood that de34fda affected func floatingPanel(_ fpc: FloatingPanelController, contentOffsetForPinning trackingScrollView: UIScrollView)method. I tested it on a wrong condition. The commit still works for the delegate method! I'm really sorry. I will removed 5181ccd and merge this PR to the main branch 👍

@scenee scenee force-pushed the scrollView-offset-reset branch from 5181ccd to de34fda Compare February 4, 2022 13:18
@dudek-j
Copy link
Contributor Author

dudek-j commented Feb 4, 2022

No reason to apologise, glad to hear that things are working as intended. I really enjoy using your library, very happy that I could contribute a little.

@scenee
Copy link
Owner

scenee commented Feb 4, 2022

Thank you for saying that. I'm so glad.

@scenee scenee merged commit 6e7c311 into scenee:master Feb 4, 2022
scenee added a commit that referenced this pull request Jan 21, 2023
This commit sets the initial scroll offset to the pinning offset.
The previous implementation, which set it to the current content offset,
leads various scroll tracking bugs. This reproduction is one of issues.

Using 'Scroll tracking(UITableView)' in Samples app.
1. Bounce the scroll content at the top most anchor.
2. Pull down the panel in bouncing at a minus content offset. (the
   scroll content stops at the minus offset.)
3. Pull up it

The previous implementation was implemented for #526/#527. But now the
issue hasn't been reproduced in v2.5.6.
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