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

insertSubview:at: seems to be off by one #308

Closed
janek opened this issue Feb 18, 2020 · 2 comments · Fixed by #309
Closed

insertSubview:at: seems to be off by one #308

janek opened this issue Feb 18, 2020 · 2 comments · Fixed by #309

Comments

@janek
Copy link
Contributor

janek commented Feb 18, 2020

Problem

insertSubviews seems to be off by one, running the same code on iOS and macOS gives a different view hierarchy. Code to reproduce this in our DemoApp is attached at the bottom of the post, shortened pseudode below.

Pseudocode:

add 3 square views 
insert a blue horizontal view at index #1

Results:

iOS macOS
Screenshot 2020-02-18 at 14 23 16 Screenshot 2020-02-18 at 14 41 18
Correct, blue view seems to be on the position with index 1 Incorrect, blue view seems to be on position with index 2

Note: this does not affect inserting at index 0 - that one behaves correctly

Snippets for reproduction and debugging

DemoAppControllerForInsertSubviewsTesting.txt
testScrollIndicatorsHierarchyPosition.txt

@ephemer
Copy link
Member

ephemer commented Feb 18, 2020

@janek thanks for reporting this. Apparently this doesn't affect our use cases directly but it's a pretty fundamental functionality that we should get right. It should also be easy to make a unit test for this that runs on both iOS and Android for this.

@janek
Copy link
Contributor Author

janek commented Feb 18, 2020

Thanks!
Yes, I already experimented with a test for both platforms. The snippet I posted doesn't exactly work, but is a useful starting point. It needs to have the indicator stuff commented out and have some more variety in cases it tests.

I didn't want to get too sidetracked, as I was doing something else, but I'm very happy to push this quite soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants