RCTRefreshControl: Updates progressOffset behaviour to prevent it from being applied by default #35281

objectivecosta · 2022-11-09T12:20:57Z

Summary

UIRefreshControl has a tight integration with iOS in terms of UINavigationBar/UIScrollView. In this regard, whenever there's a UIScrollView with a UINavigationBar on top, the OS automatically adjusts the contentInset of the UIScrollView to reflect that (something that is exposed to React Native). In a similar manner, UIScrollView passes this along to the attached UIRefreshControl.

By setting the frame manually, the RCTRefreshControl component was preventing this behavior. Although having the option is desired, it should not be done by default. In the past it was possible to adjust for this by manually setting the correct value, calculating the statusBar's height/safeAreaInsets.top and appending 44pt (the UINavigationBar height). However, due to changes related to the Dynamic Island (see here), the safe area and the status bar size no longer align, making this calculation more tricky.

In summary: this changes allows progressViewOffset to exist (in order to maintain feature parity with Android) but provides the opportunity for the OSs default behavior to kick in when applicable.

Applying by default	Not applying by default (this change)

Changelog

[iOS] [Fixed] - Fix application of _progressViewOffset in RCTRefreshControl to not occur by default (when value is unset)

Test Plan

The GIFs attached display the behavior as expected/unexpected. I'm unaware of any tests written for RCTRefreshControl that could be improved to cover this change. Notes appreciated.

…m being applied by default

analysis-bot · 2022-11-09T12:45:26Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,070,062	-29,214
android	hermes	armeabi-v7a	6,442,430	-26,503
android	hermes	x86	7,485,469	-31,663
android	hermes	x86_64	7,344,931	-30,860
android	jsc	arm64-v8a	8,934,550	-29,373
android	jsc	armeabi-v7a	7,668,661	-26,649
android	jsc	x86	8,994,599	-31,816
android	jsc	x86_64	9,473,532	-31,022

Base commit: b8893c7
Branch: main

…re readers

facebook-github-bot · 2022-11-09T13:02:20Z

Hi @objectivecosta!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

analysis-bot · 2022-11-09T13:21:53Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: b8893c7
Branch: main

pull-bot · 2022-11-09T13:32:16Z

PR build artifact for 4438584 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-09T13:36:23Z

PR build artifact for 4438584 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-11-09T14:06:10Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot · 2022-11-09T14:06:17Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

objectivecosta · 2022-11-14T18:06:13Z

Can't seem to request a review via the GitHub UI as usual, pinging @cipolleschi instead ;)

objectivecosta · 2022-11-14T18:07:15Z

One thing that I reflected upon is that maybe the condition can be changed for != 0.f instead of > 0.f, to allow for negative offsets. Is that a possible scenario?

RCTRefreshControl: Updates condition for applying offset to be `!= 0.f`, not `> 0.f`. This will allow negative offsets, if needed.

objectivecosta · 2022-11-15T10:04:42Z

Updated the condition to be != 0.f, to allow for negative offsets. PR is ready for review, but I don't know how to request one here :/

cipolleschi

Hi @objectivecosta, thank you so much for doing this, that's great.

I added a comment to improve the readability of the code, but it is a personal preference (coming from Swift and guard statements) so feel free to disregard it if you don't feel like it.

I'll also look into why you couldn't request a review, but it's fine to ping me (hoping I don't miss the notification! 😅)

cipolleschi · 2022-11-15T10:34:09Z

React/Views/RefreshControl/RCTRefreshControl.m

+  // Setting the UIRefreshControl's frame breaks integration with ContentInset from the superview
+  // if it is a UIScrollView. This integration happens when setting the UIScrollView's .refreshControl
+  // property. For this reason, setting the frame manually should be avoided, if not needed.
+  if (_progressViewOffset != 0.f) {


What about an early return? Something like:

if (_progressViewOffset == 0.f) { return }

It limits the indentation of the code and the positive check make it immediately clear that if the offset is 0 we don't want to do anything.

What do you think?

Makes total sense @cipolleschi! Changed it ;)

facebook-github-bot · 2022-11-15T10:36:35Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pull-bot · 2022-11-15T11:09:26Z

PR build artifact for c035025 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-15T11:11:25Z

PR build artifact for c035025 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

Updates _applyProgressViewOffset with an early return for readability

facebook-github-bot · 2022-11-15T13:26:09Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pull-bot · 2022-11-15T13:51:49Z

PR build artifact for bf24c9f is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-15T13:54:42Z

PR build artifact for bf24c9f is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

objectivecosta · 2022-11-17T13:47:25Z

ping @cipolleschi: I think this is ready for merging!

cipolleschi · 2022-11-17T16:13:43Z

It has landed! 0062b10

objectivecosta · 2022-11-19T10:58:05Z

Great stuff! Thanks @cipolleschi :D

…m being applied by default (facebook#35281) Summary: UIRefreshControl has a tight integration with iOS in terms of UINavigationBar/UIScrollView. In this regard, whenever there's a UIScrollView with a UINavigationBar on top, the OS automatically adjusts the contentInset of the UIScrollView to reflect that (something that is exposed to React Native). In a similar manner, UIScrollView passes this along to the attached UIRefreshControl. By setting the frame manually, the RCTRefreshControl component was preventing this behavior. Although having the option is desired, it should not be done by default. In the past it was possible to adjust for this by manually setting the correct value, calculating the statusBar's height/safeAreaInsets.top and appending 44pt (the UINavigationBar height). However, due to changes related to the Dynamic Island (see [here](https://useyourloaf.com/blog/iphone-14-screen-sizes)), the safe area and the status bar size no longer align, making this calculation more tricky. In summary: this changes allows `progressViewOffset` to exist (in order to maintain feature parity with Android) but provides the opportunity for the OSs default behavior to kick in when applicable. | Applying by default | Not applying by default (this change) | :-------------------------:|:-------------------------: ![](https://user-images.githubusercontent.com/753361/200828632-0c9aa605-770c-47be-a825-1e061478c2b4.gif) | ![](https://user-images.githubusercontent.com/753361/200828664-dded9a47-bdbc-4b88-8ab7-92a76cea47e8.gif) ## Changelog  [iOS] [Fixed] - Fix application of _progressViewOffset in RCTRefreshControl to not occur by default (when value is unset) Pull Request resolved: facebook#35281 Test Plan: The GIFs attached display the behavior as expected/unexpected. I'm unaware of any tests written for RCTRefreshControl that could be improved to cover this change. Notes appreciated. Reviewed By: sammy-SC Differential Revision: D41302080 Pulled By: cipolleschi fbshipit-source-id: a2a8e6ef1dcc2e73220c2a182b4516f3bbd94f60

RCTRefreshControl: Updates progressOffset behaviour to prevent it fro…

b80cfb3

…m being applied by default

react-native-bot added Bug Platform: iOS iOS applications. labels Nov 9, 2022

objectivecosta mentioned this pull request Nov 9, 2022

[iOS] Over-zealous applying of progressViewOffset prevents UIScrollView/UIRefreshControl OS behavior #35283

Closed

RCTRefreshControl: Added explainer comment to clarify purpose to futu…

4438584

…re readers

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 9, 2022

RCTRefreshControl: Updates condition for applying offset

c035025

RCTRefreshControl: Updates condition for applying offset to be `!= 0.f`, not `> 0.f`. This will allow negative offsets, if needed.

cipolleschi approved these changes Nov 15, 2022

View reviewed changes

RCTRefreshControl: Updates _applyProgressViewOffset

bf24c9f

Updates _applyProgressViewOffset with an early return for readability

facebook-github-bot closed this in 0062b10 Nov 17, 2022

objectivecosta mentioned this pull request Nov 22, 2022

useHeaderHeight(); returning incorrect value on phones with Dynamic Island react-navigation/react-navigation#10989

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCTRefreshControl: Updates progressOffset behaviour to prevent it from being applied by default #35281

RCTRefreshControl: Updates progressOffset behaviour to prevent it from being applied by default #35281

objectivecosta commented Nov 9, 2022 •

edited

Loading

analysis-bot commented Nov 9, 2022 •

edited

Loading

facebook-github-bot commented Nov 9, 2022

analysis-bot commented Nov 9, 2022 •

edited

Loading

pull-bot commented Nov 9, 2022

pull-bot commented Nov 9, 2022

facebook-github-bot commented Nov 9, 2022

facebook-github-bot commented Nov 9, 2022

objectivecosta commented Nov 14, 2022

objectivecosta commented Nov 14, 2022

objectivecosta commented Nov 15, 2022

cipolleschi left a comment

cipolleschi Nov 15, 2022

objectivecosta Nov 15, 2022

facebook-github-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

facebook-github-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

objectivecosta commented Nov 17, 2022

cipolleschi commented Nov 17, 2022

objectivecosta commented Nov 19, 2022

RCTRefreshControl: Updates progressOffset behaviour to prevent it from being applied by default #35281

RCTRefreshControl: Updates progressOffset behaviour to prevent it from being applied by default #35281

Conversation

objectivecosta commented Nov 9, 2022 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Nov 9, 2022 • edited Loading

facebook-github-bot commented Nov 9, 2022

Action Required

Process

analysis-bot commented Nov 9, 2022 • edited Loading

pull-bot commented Nov 9, 2022

pull-bot commented Nov 9, 2022

facebook-github-bot commented Nov 9, 2022

facebook-github-bot commented Nov 9, 2022

objectivecosta commented Nov 14, 2022

objectivecosta commented Nov 14, 2022

objectivecosta commented Nov 15, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi Nov 15, 2022

Choose a reason for hiding this comment

objectivecosta Nov 15, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

facebook-github-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

objectivecosta commented Nov 17, 2022

cipolleschi commented Nov 17, 2022

objectivecosta commented Nov 19, 2022

objectivecosta commented Nov 9, 2022 •

edited

Loading

analysis-bot commented Nov 9, 2022 •

edited

Loading

analysis-bot commented Nov 9, 2022 •

edited

Loading