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

[Android] ScrollView should send correct ScrollX and ScrollY #394

Merged
merged 10 commits into from Dec 6, 2016

Conversation

Projects
None yet
5 participants
@adrianknight89
Contributor

adrianknight89 commented Sep 29, 2016

Description of Change

Android ScrollView was incorrectly sending ScrollX and ScrollY values when scroll orientation was Both. This is because a horizontal scrollview is a child of a vertical scrollview and they are raising scroll events independently of each other. The user saw alternating 0 values for both positions.

This should send the final ScrollX and ScrollY to Core with every position update.

EDIT:

While testing horizontal + vertical scrolling, I discovered a bug. ScrollToAsync() method did not work when orientation was set to Both. Maybe I should have created a bug report for this and submitted it in a new PR. However, the fix works nicely with the fix for this bug. See the switch blocks for proper scroll calls. I also added a unit test to watch out for possible incorrect animation positioning. When you do back to back scroll animations, there shouldn't be an intermediate position. The second animation should begin where the first left off.

Bugs Fixed

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Sep 29, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Sep 29, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@adrianknight89 adrianknight89 changed the title from Android scrollview to [Android] ScrollView should send correct ScrollX and ScrollY Sep 29, 2016

@jamesl77

This comment has been minimized.

Show comment
Hide comment
@jamesl77

jamesl77 Oct 18, 2016

I'm getting 0 values for ScrollX when scrolling, just like in the description of this PR I think.
Any update on merging this fix and releasing it in the NuGet?

jamesl77 commented Oct 18, 2016

I'm getting 0 values for ScrollX when scrolling, just like in the description of this PR I think.
Any update on merging this fix and releasing it in the NuGet?

@samhouts samhouts self-assigned this Oct 21, 2016

@samhouts

Tested, works well. I would like to see the test automated. This one seems to work well: https://gist.github.com/samhouts/6457f779b5d7e3658620fcb4d6c3d2fa

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 22, 2016

Contributor

@samhouts I made improvements so we monitor changes during flight instead of only looking at final values.

Do you accept UI tests not associated with bugs? The idea is to write tests based on expectations in order to hopefully prevent future regression. If so, where would they be placed?

Contributor

adrianknight89 commented Oct 22, 2016

@samhouts I made improvements so we monitor changes during flight instead of only looking at final values.

Do you accept UI tests not associated with bugs? The idea is to write tests based on expectations in order to hopefully prevent future regression. If so, where would they be placed?

@rmarinho rmarinho merged commit 9dff4c1 into xamarin:master Dec 6, 2016

2 checks passed

OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3620, ignored: 10
Details
@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 7, 2016

Member

Hey @adrianknight89 this test is failing on CI on all iOS version

Failing for Ipad Air iOS 9

Failing for iPhone 6 plus on iOS 8

Failing for Iphone 6 on iOS 10

Also if i run in my sim i don't get the 100 100 values we are expecting when clicking.

simulator screen shot 7 dec 2016 12 37 01

I m reverting this PR for now..

Member

rmarinho commented Dec 7, 2016

Hey @adrianknight89 this test is failing on CI on all iOS version

Failing for Ipad Air iOS 9

Failing for iPhone 6 plus on iOS 8

Failing for Iphone 6 on iOS 10

Also if i run in my sim i don't get the 100 100 values we are expecting when clicking.

simulator screen shot 7 dec 2016 12 37 01

I m reverting this PR for now..

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 7, 2016

Member

Better i will not revert, i will just run the test in Android.

Member

rmarinho commented Dec 7, 2016

Better i will not revert, i will just run the test in Android.

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

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