Skip to content

Commit

Permalink
ScrollView: Smart contentOffset preserving
Browse files Browse the repository at this point in the history 
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 @facebook-github-bot
shergin authored and facebook-github-bot committed Jul 18, 2017
1 parent 301830d commit 1d22f8f
Showing 1 changed file with 14 additions and 2 deletions.
16 changes: 14 additions & 2 deletions React/Views/RCTScrollView.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

7 comments on commit 1d22f8f

@kpink224
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

@shergin shergin commented on 1d22f8f Aug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@shergin shergin commented on 1d22f8f Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janicduplessis PR is very welcome. 😸

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wschurman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.