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

iOS ScrollView should not scroll out of place on scrolling to element #371

Merged
merged 2 commits into from Oct 11, 2016

Conversation

Projects
None yet
6 participants
@adrianknight89
Contributor

adrianknight89 commented Sep 23, 2016

Description of Change

iOS ScrollView was behaving differently than Android when using ScrollToPosition enums. If you center or end scroll the first element, iOS moves the scrollview out of place by setting the x position to a negative value. Similarly, if you start or center scroll the last element, the x position is set to a value greater than what should be allowed. This change ensures iOS and Android behave the same.

Bugs Fixed

Behavioral Changes

iOS is now more intelligent such that ScrollView elements are positioned without going out of bounds. Developers can still set to any position they want by using ScrollToAsync(double x, double y, bool animated).

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Sep 23, 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 23, 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;

Show outdated Hide outdated Xamarin.Forms.Platform.iOS/Renderers/ScrollViewRenderer.cs
else if (positionOnScroll.Y > ContentSize.Height - Bounds.Size.Height)
{
positionOnScroll.Y = ContentSize.Height - Bounds.Size.Height;
}

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 23, 2016

Member

could you use Math.Min() and Math.Max() for this ?

positionOnScroll.X = Math.Max(positionOnScroll.X, 0); that'd help the readability

@StephaneDelcroix

StephaneDelcroix Sep 23, 2016

Member

could you use Math.Min() and Math.Max() for this ?

positionOnScroll.X = Math.Max(positionOnScroll.X, 0); that'd help the readability

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 23, 2016

Member

well, scratch that, we have a solution for this already:

positionOnScroll.X = positionOnScroll.X.Clamp(0, ContentSize.Width - Bounds.Size.Width);
positionOnScroll.Y = positionOnScroll.Y.Clamp(0, ContentSize.Height - Bounds.Size.Height);

(Clamp is an internal extension method)

@StephaneDelcroix

StephaneDelcroix Sep 23, 2016

Member

well, scratch that, we have a solution for this already:

positionOnScroll.X = positionOnScroll.X.Clamp(0, ContentSize.Width - Bounds.Size.Width);
positionOnScroll.Y = positionOnScroll.Y.Clamp(0, ContentSize.Height - Bounds.Size.Height);

(Clamp is an internal extension method)

This comment has been minimized.

@adrianknight89

adrianknight89 Sep 23, 2016

Contributor

Done

@adrianknight89

@StephaneDelcroix StephaneDelcroix merged commit e9606cd into xamarin:master Oct 11, 2016

@jamesl77

This comment has been minimized.

Show comment
Hide comment
@jamesl77

jamesl77 Oct 18, 2016

Any reason why this and this were not included in the latest NuGet 2.3.3.163-pre3?

ScrollView is a pretty basic control and it's buggy but it looks like someone fixed this, so why not include the latest NuGet?

jamesl77 commented Oct 18, 2016

Any reason why this and this were not included in the latest NuGet 2.3.3.163-pre3?

ScrollView is a pretty basic control and it's buggy but it looks like someone fixed this, so why not include the latest NuGet?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Oct 18, 2016

Member

@jamesl77 so 2.3.3 was locked in a stable branch , we only cherry pick things if really really necessary and are related directly with that release and code introduce in that 2.3.3.
This was merged into master and will go in the next version 2.3.4-pre1.

Thanks

Member

rmarinho commented Oct 18, 2016

@jamesl77 so 2.3.3 was locked in a stable branch , we only cherry pick things if really really necessary and are related directly with that release and code introduce in that 2.3.3.
This was merged into master and will go in the next version 2.3.4-pre1.

Thanks

@jamesl77

This comment has been minimized.

Show comment
Hide comment
@jamesl77

jamesl77 Oct 18, 2016

So you mean this fix will not be included in the 2.3.3 stable release(whenever this comes out), but in the first 2.3.4 pre-version?
This means another 2 months to wait for such a small but important fix...

jamesl77 commented Oct 18, 2016

So you mean this fix will not be included in the 2.3.3 stable release(whenever this comes out), but in the first 2.3.4 pre-version?
This means another 2 months to wait for such a small but important fix...

@jamesl77

This comment has been minimized.

Show comment
Hide comment
@jamesl77

jamesl77 Oct 18, 2016

Meanwhile everyone who bumps into this issue will have to bang his head against the wall why on earth ScrollView, such a basic control doesn't work properly...

With this pace, it will take years before Xamarin Forms will get everything right on the basic level

jamesl77 commented Oct 18, 2016

Meanwhile everyone who bumps into this issue will have to bang his head against the wall why on earth ScrollView, such a basic control doesn't work properly...

With this pace, it will take years before Xamarin Forms will get everything right on the basic level

@samhouts samhouts added this to the 2.3.4 milestone Jul 3, 2018

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