Permalink
Browse files

ScrollView: Smart `contentOffset` preserving

Summary:
Previous `contentOffset` can be invalid for a new layout and overscroll the ScrollView, so the diff fixes that.
Also documented here: #13566

Reviewed By: mmmulani

Differential Revision: D5414442

fbshipit-source-id: 7de1b4a4571108a37d1795e80f165bca5aba5fef
  • Loading branch information...
shergin authored and facebook-github-bot committed Jul 18, 2017
1 parent 301830d commit 1d22f8fb27e9432e357401ce6f6cede4d710472c
Showing with 14 additions and 2 deletions.
  1. +14 −2 React/Views/RCTScrollView.m
@@ -325,10 +325,22 @@ - (void)setFrame:(CGRect)frame
return;
}
// Preserving `contentOffset` between layout passes.
// Preserving and revalidating `contentOffset`.
CGPoint originalOffset = self.contentOffset;
[super setFrame:frame];
self.contentOffset = originalOffset;
UIEdgeInsets contentInset = self.contentInset;
CGSize contentSize = self.contentSize;
CGSize fullContentSize = CGSizeMake(
contentSize.width + contentInset.left + contentInset.right,
contentSize.height + contentInset.top + contentInset.bottom);
CGSize boundsSize = self.bounds.size;
self.contentOffset = CGPointMake(
MAX(0, MIN(originalOffset.x, fullContentSize.width - boundsSize.width)),
MAX(0, MIN(originalOffset.y, fullContentSize.height - boundsSize.height)));
}
#if !TARGET_OS_TV

7 comments on commit 1d22f8f

@kpink224

This comment has been minimized.

Show comment
Hide comment
@kpink224

kpink224 Jul 27, 2017

Contributor

🥇

Contributor

kpink224 replied Jul 27, 2017

🥇

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 1, 2017

Collaborator

@shergin This breaks setting negative offsets, I'm using this with contentInset + RefreshControl to make sure it is positioned properly under a overlapping header. This could probably be solved by supporting progressViewOffset on iOS, not sure if there are other use cases for negative offsets.

Collaborator

janicduplessis replied Aug 1, 2017

@shergin This breaks setting negative offsets, I'm using this with contentInset + RefreshControl to make sure it is positioned properly under a overlapping header. This could probably be solved by supporting progressViewOffset on iOS, not sure if there are other use cases for negative offsets.

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Aug 1, 2017

Contributor

@janicduplessis So, do you think we have to support setting negative value for contentOffset? Or we should just fix RefreshControl properly on native side? Can we do this without adding additional props?

Contributor

shergin replied Aug 1, 2017

@janicduplessis So, do you think we have to support setting negative value for contentOffset? Or we should just fix RefreshControl properly on native side? Can we do this without adding additional props?

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 1, 2017

Collaborator

That's the only use case I have for negative contentOffset, idk if there are others. RefreshControl already has a prop called progressViewOffset that is Android only so we could make it work on iOS to control where it appears.

Collaborator

janicduplessis replied Aug 1, 2017

That's the only use case I have for negative contentOffset, idk if there are others. RefreshControl already has a prop called progressViewOffset that is Android only so we could make it work on iOS to control where it appears.

@shergin

This comment has been minimized.

Show comment
Hide comment
@shergin

shergin Aug 2, 2017

Contributor

@janicduplessis PR is very welcome. 😸

Contributor

shergin replied Aug 2, 2017

@janicduplessis PR is very welcome. 😸

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 2, 2017

Collaborator

@shergin I think someone made a PR a while back but it never got merged, I could try to revive it.

Collaborator

janicduplessis replied Aug 2, 2017

@shergin I think someone made a PR a while back but it never got merged, I could try to revive it.

@wschurman

This comment has been minimized.

Show comment
Hide comment
@wschurman

wschurman Sep 5, 2017

Contributor

I believe this made it into v0.48 without a fix for the RefreshControl issue. Past PRs that @janicduplessis mentioned: #10718, #11356.

New issue: #15808

Contributor

wschurman replied Sep 5, 2017

I believe this made it into v0.48 without a fix for the RefreshControl issue. Past PRs that @janicduplessis mentioned: #10718, #11356.

New issue: #15808

Please sign in to comment.