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

Fixing broken table scrolling #1103

Conversation

harleyjcooper
Copy link
Contributor

@harleyjcooper harleyjcooper commented Apr 16, 2019

When attempting to tap offscreen table elements, after [tableView scrollToRowAtIndexPath:indexPath atScrollPosition:UITableViewScrollPositionNone animated:animationEnabled] is called, the scrolling never actually occurs before the call to [tableView cellForRowAtIndexPath:indexPath], which will always return nil for offscreen table cells. A yield could be inserted in there, but for large tables, that would be an enormous amount of time.

The spinning of the runloop after that block only allows the final call to scrollToRowAtIndexPath to be executed, so the code will only succeed if the cell is in view initially, or it's in the final scrolled section in the table. This code, as it was, would never succeed if the cell being sought was above the initially scrolled area of the table.

Switching the scrollToRowAtIndexPath call to [tableView scrollRectToVisible:sectionRect animated:NO] is more instantaneous, requires no yield, and works for cells above AND below the currently-scrolled portion of the table.

Plus, the final call to [tableView scrollRectToVisible:initialPosition animated:NO] will succeed in moving back to the initial position for all cases, essentially instantaneously.

@justinseanmartin
Copy link
Contributor

Overall LGTM as long as you update the PR description to explain what you're doing and why.

@justinseanmartin justinseanmartin self-requested a review April 17, 2019 06:02
@harleyjcooper harleyjcooper changed the title [DNM] Fixing table scrolling Fixing broken table scrolling Apr 17, 2019
@dflems
Copy link
Contributor

dflems commented Apr 17, 2019

@justinseanmartin Can you cut a new version once this is merged?

@justinseanmartin
Copy link
Contributor

Yea, @dflems - were you able to test the change, did it work for you all?

@harleyjcooper harleyjcooper merged commit b7ee6ea into kif-framework:master Apr 17, 2019
@harleyjcooper harleyjcooper deleted the harley/kif-fix-table-scrolling branch April 17, 2019 20:44
@harleyjcooper
Copy link
Contributor Author

@dflems We've released KIF 3.7.6 - https://github.com/kif-framework/KIF/releases/tag/v3.7.6

@dflems
Copy link
Contributor

dflems commented Apr 18, 2019

@justinseanmartin Looking good. Previously ~40 tests were failing/flaking and now it's down to two (looks like its for other reasons than the scrolling though). I'll follow up once we diagnose those.

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.

3 participants