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

fix(core/ios): reset additional insets if they're zero #10134

Conversation

mukaschultze
Copy link
Contributor

@mukaschultze mukaschultze commented Dec 16, 2022

PR Checklist

What is the current behavior?

Sometimes additionalSafeAreaInsets are correctly set in a layout pass but should be reset back to zero if the size if a sibling view changes. This doesn't happen and causes the safezone to be incorrectly calculated for the view that had the additionalSafeAreaInsets set.

What is the new behavior?

additionalSafeAreaInsets is set back to null if the newly calculated additional insets are zero.

@nx-cloud
Copy link

nx-cloud bot commented Dec 16, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 67ea121. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link
Member

@rigor789 rigor789 left a comment

Choose a reason for hiding this comment

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

Looks good to me - nice catch 👍

@CatchABus
Copy link
Contributor

CatchABus commented Dec 17, 2022

@mukaschultze @rigor789 Unfortunately, this is not fixing #10092
It's not that we don't know what's causing this, it's just that fix is not yet released.

After trying to look what causes the problem, @cjohn001 and I concluded the following patches are currently causing trouble:
#10044
#9773

See https://discord.com/channels/603595811204366337/606457628729671691/1048331850315202654

@edusperoni Removing each one of these separately was fixing the problem so I suspect a conflict between the two of them.
I tend to think we need to re-examine #9773 specifically and see if it's still needed there.

@mukaschultze About the case of additionalSafeAreaInsets, I'm not sure if we ever get cases that return zero for top and bottom for view controllers that "already" got positive additional insets (never checked tbh), but I definitely like precautions like this one.

@cjohn001
Copy link

does not solve #10092 (comment)

@mukaschultze
Copy link
Contributor Author

@mukaschultze About the case of additionalSafeAreaInsets, I'm not sure if we ever get cases that return zero for top and bottom for view controllers that "already" got positive additional insets (never checked tbh), but I definitely like precautions like this one.

@CatchABus it's possible, although rare, to get zero for controllers that had the insets already set. For example, if we have something like:

<GridLayout rows="auto, *" #1>
	<GridLayout row="1" *ngIf="someConditionThatChangesDuringRouteChange" #2></GridLayout>
	<GridLayout row="2" #3>
		<page-router-outlet><page-router-outlet>
	</GridLayout>
</StackLayout>

When the someConditionThatChangesDuringRouteChange changes it will lead to the Page (page-router-outlet) having an inset even though the inset was set in the GridLayout#2 when it was created.

This is why I thought this PR might fix #10092 too because it's a very similar issue to the one I'm facing. I've tested removing the layoutIfNeeded and recalculating the additional insets during the viewSafeAreaInsetsDidChange but none of that worked for the example above.

@CatchABus
Copy link
Contributor

CatchABus commented Dec 18, 2022

@mukaschultze About the case of additionalSafeAreaInsets, I'm not sure if we ever get cases that return zero for top and bottom for view controllers that "already" got positive additional insets (never checked tbh), but I definitely like precautions like this one.

@CatchABus it's possible, although rare, to get zero for controllers that had the insets already set. For example, if we have something like:

<GridLayout rows="auto, *" #1>
	<GridLayout row="1" *ngIf="someConditionThatChangesDuringRouteChange" #2></GridLayout>
	<GridLayout row="2" #3>
		<page-router-outlet><page-router-outlet>
	</GridLayout>
</StackLayout>

When the someConditionThatChangesDuringRouteChange changes it will lead to the Page (page-router-outlet) having an inset even though the inset was set in the GridLayout#2 when it was created.

This is why I thought this PR might fix #10092 too because it's a very similar issue to the one I'm facing. I've tested removing the layoutIfNeeded and recalculating the additional insets during the viewSafeAreaInsetsDidChange but none of that worked for the example above.

Thank you very much for clarification! Hopefully, this may solve issues like #10067

@NathanWalker NathanWalker merged commit 8b7d5ab into NativeScript:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants