Skip to content
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

feat: support contentInset on Android #49145

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kirillzyusko
Copy link
Contributor

@kirillzyusko kirillzyusko commented Feb 3, 2025

Summary:

At the moment contentInset property is available only on iOS. In this PR I'm adding the support for this property via applying additional padding directly to ScrollView native component.

Note: in this PR I handle only a case that is needed for react-native-keyboard-contoller and it may not work in other cases really well (I know that I overwrite other paddings potentially set by users, etc.), but... the purpose of this PR is to provide a rough idea (use padding to simulate contentInset behavior + setClipToPadding) and get a feedback, whether you'd like to have such changes or not 🙌

Changelog:

[ANDROID] [ADDED] - support contentInset on Android

Test Plan:

iOS Android
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-02-03.at.19.18.35.mp4
telegram-cloud-document-2-5262625811495674255.mp4

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 3, 2025
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi @kirillzyusko, thanks for the PR.

Have you checked whether this works properly in the New Arch?

Comment on lines +1147 to +1157
post(new Runnable() {
@Override
public void run() {
int maxScrollY = getMaxScrollY();
// If the current scroll offset is now beyond the new maximum,
// adjust it to the new maximum.
if (getScrollY() > maxScrollY) {
scrollTo(getScrollX(), maxScrollY);
}
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

would this show a flicker or an animation or does it happen immediately? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it happens immediately. Basically it fixes a very strange problem. Without this code problems looks like this:

telegram-cloud-document-2-5264877611309358434.mp4

Even though I can clearly see that setter gets called:

image

There is still a frozen area of the contentInset. But it disappear as soon as I start scrolling. So I thought to add scrollTo programmatically. Maybe there is a better fix for this problem - let me know if you find one 🙌

@kirillzyusko
Copy link
Contributor Author

kirillzyusko commented Feb 4, 2025

Have you checked whether this works properly in the New Arch?

I tested only new architecture 🙂 However I have a question about ReactAndroid.api file - is it autogenerated?

In fact it's very draft changes, because I assume at the moment padding may conflict with contentInset, contentInset is not returned in onScroll event, update HorizontalScrollView implementation etc. But again, I'll be glad to fix all of those issues if you say that you'd like to have such changes in the codebase (I don't know a reason why this prop wasn't added on Android - my assumption is that because there is no 1:1 API, so the prop was simply ignored, so I'm curios whether you like to have such changes or not) 😊

@cipolleschi
Copy link
Contributor

I tested only new architecture 🙂

Great, thanks for confirming!

However I have a question about ReactAndroid.api file - is it autogenerated?

No, we have a script to generate it, but it only runs in the internal infra, sadly. So we have to run it ourselves after we import PRs

In fact it's very draft changes, because I assume at the moment padding may conflict with contentInset, contentInset is not returned in onScroll event, update HorizontalScrollView implementation etc. But again, I'll be glad to fix all of those issues if you say that you'd like to have such changes in the codebase (I don't know a reason why this prop wasn't added on Android - my assumption is that because there is no 1:1 API, so the prop was simply ignored, so I'm curios whether you like to have such changes or not) 😊

I don't have a lot of context on the why. I also don't know about all those implications as android is not really my area of expertise. But I'll make sure to pass this around to other Android engineers that might have context/knowledge on this.

@facebook-github-bot
Copy link
Contributor

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

@kirillzyusko
Copy link
Contributor Author

But I'll make sure to pass this around to other Android engineers that might have context/knowledge on this.

Awesome! Thank you!

As I said before - I'm ready to implement missing pieces, just would like to know what exactly I should implement 🙌 😊 Thank you for your assistance ❤️

@cipolleschi
Copy link
Contributor

@javache Mentioned that, for sure we need to have the HorizontalScrollView implementation (but you knew about that!)

There might be some other gotchas, and he is definitely more expert on Android than me.

@kirillzyusko kirillzyusko force-pushed the feat/support-content-inset-on-Android branch from b1c1f8e to 0ee7413 Compare March 7, 2025 14:04
@kirillzyusko
Copy link
Contributor Author

@javache @cipolleschi I encountered one problem and I really don't understand the root cause 🤔

Basically if I apply setPadding + setClipToPadding(false) then ScrollView will put both paddings into the bottom, i. e.:

- item 0
- item 1
- ...
- item N
- (paddingTop)
- (paddingBottom)

I've tried to workaround that problem by:

View content = getContentView();
content.setTranslationY(top);

setPadding(left, top, right, bottom);
scrollBy(0, top);
// simulate iOS behavior
setClipToPadding(false);

And it kind of works, but since we shift the content of ScrollView via translate property in some areas of ScrollView scrolling becomes impossible (we don't detect a tap, because there is no content, since we applied translate).

My other idea was to add marginTop to inner ReactViewGroup (that acts as a container), so that the new padding model will be like this:

- marginTop
- item 0
- item 1
- ...
- item N
- (paddingBottom) <-- `paddingTop` will be compensated by `marginTop`

However that approach doesn't work at all 🙃 I'm not allowed to change paddings of React views because they are laid out by YOGA and all my manual changes get ignored?
Do you have any ideas how to overcome this problem? Or at least a direction that I should try to discover?

We can also have a conversation in discord, if you think that it'll be easier/faster for you 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants