fix(iOS): push and pop transitions change after full screen back swipe #2234

maksg · 2024-07-05T14:55:59Z

Description

Doing the swipe also caused programmatic push to use manual transition and have no shadow.
Fixes #2227.

Changes

Set _isFullWidthSwiping to NO when swipe gesture ends in handleSwipe: so programmatic push and pop can use OS logic by default.

Screenshots / GIFs

Before

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-05.at.16.43.46.mp4

After

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-05.at.16.44.34.mp4

Test code and steps to reproduce

Run Test2227 in TestsExample app.
Push screen with Go to Details.
Swipe back with full screen swipe gesture.
Push screen with Go to Details again.
Before this fix there would be no shadow during transition after first full screen swipe back.

Checklist

Included code example that can be used to test this change
Updated TS types
Updated documentation:
Ensured that CI passes

kkafar

Left a tine remark, other than that - great job!

kkafar · 2024-07-08T08:26:56Z

ios/RNSScreenStack.mm

@@ -752,7 +752,7 @@ - (BOOL)gestureRecognizerShouldBegin:(UIGestureRecognizer *)gestureRecognizer
  [self cancelTouchesInParent];
  return YES;
 #else
-  // RNSPanGestureRecognizer will receive events iff topScreen.fullScreenSwipeEnabled == YES;
+  // RNSPanGestureRecognizer will receive events if topScreen.fullScreenSwipeEnabled == YES;


The iff is intentional - it is widely used, especially in mathematics & stands for if and only if (wtedy i tylko wtedy) 😄

Suggested change

// RNSPanGestureRecognizer will receive events if topScreen.fullScreenSwipeEnabled == YES;

// RNSPanGestureRecognizer will receive events iff topScreen.fullScreenSwipeEnabled == YES;

Ah, haven't heard about that one before. That's good to know, thanks for enlightening me! 😃 I will revert that change

kkafar · 2024-07-08T08:36:36Z

ios/RNSScreenStack.mm

@@ -872,6 +872,7 @@ - (void)handleSwipe:(UIPanGestureRecognizer *)gestureRecognizer
        [_interactionController cancelInteractiveTransition];
      }
      _interactionController = nil;
+      _isFullWidthSwiping = NO;


Okay, so the main issue (change of transition style after popping a screen with swipe for the very first time) was caused by not properly reset state. Great finding!

kkafar · 2024-07-08T08:40:20Z

ios/RNSScreenStackAnimator.mm

    [UIView animateWithDuration:[self transitionDuration:transitionContext]
        animations:^{
          fromViewController.view.transform = leftTransform;
          toViewController.view.transform = CGAffineTransformIdentity;
+          shadowView.alpha = 0.1;


My only suggestion here would be to extract this target alpha value to a constant, see the top of the RNSScreenStackAnimator.mm file (maybe use constexpr instead of static const) & if there is any reasoning behind this value (e.g. "Looked best in experimentation") - leave a comment behind.

Sure thing, I'll extract it and I'll add a comment a comment with explanation

kkafar

One last thing, and we're good to go.

kkafar · 2024-07-08T11:56:52Z

ios/RNSScreenStackAnimator.mm

@@ -9,6 +9,9 @@
 static const float RNSSlideCloseTransitionDurationProportion = 0.25 / 0.35;
 static const float RNSFadeCloseTransitionDurationProportion = 0.15 / 0.35;
 static const float RNSFadeCloseDelayTransitionDurationProportion = 0.1 / 0.35;
+// same value is used in other projects using similar approach for transistions
+// and it looks the most similar to the value used by Apple
+constexpr float RNSShadowViewMaxAlpha = 0.1;


Idk why I suggested sole constexpr here, let's make it static also, so that the definition is visible only in this translation unit.

Suggested change

constexpr float RNSShadowViewMaxAlpha = 0.1;

static constexpr float RNSShadowViewMaxAlpha = 0.1;

WoLewicki

I am afraid doing it like this is a breaking change. When people use fullScreenSwipe right now, they don't have the shadow included. With those changes, it will appear which can be not expected by the users. Shouldn't we make this an optional prop of screen/navigator instead?

kkafar · 2024-07-08T12:14:58Z

I considered it rather a fix, than breaking the behaviour. Although I can see your point.

One thing for sure: the change responsible for using default animation for programming push / pop after popping a screen with a gesture is a fix & it should be merged as is.

The code responsible for adding a dimming view for swipe-transition might be hidden behind a prop, as opt-in.

@maksg @WoLewicki Let's split this PR into two separate ones. Does that sound right?

… pop

maksg · 2024-07-08T13:59:43Z

I moved shadow view implementation to #2239

jwoodmansey · 2024-07-08T16:46:56Z

I have just played around with this branch and it seems that this issue shows up again randomly. As in, every so often navigation still uses the incorrect transition, it is now very rare. Are there more cases where _isFullWidthSwiping does not get properly reset?

Sorry I know that's not particularly helpful, I was swiping around a lot in a large app and have seen it a few times.

kkafar · 2024-07-09T07:51:11Z

@jwoodmansey Can you post a video where you trigger the buggy behaviour? Maybe we can figure out the scenario in which in happens together somehow.

Despite the fact, this PR improves the situation. We should proceed @maksg.

…adow during swipe gesture (#2239) ## Description When using full screen swipe back there was no shadow under the view. Related PR #2234 with discussion about this change. Corresponding PR in `react-navigation`: * react-navigation/react-navigation#12053 ## Changes Added shadow to transition in animateSimplePushWithTransitionContext:toVC:fromVC:. ## Test code and steps to reproduce 1. Run `Test2227` in TestsExample app. 2. Push screen with Go to Details. 3. Swipe back with full screen swipe gesture. 5. Before this fix there would be no shadow during transition with full screen swipe back. ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [x] Updated documentation:  - [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes

software-mansion#2234) ## Description Doing the swipe also caused programmatic push to use manual transition and have no shadow. Fixes software-mansion#2227. ## Changes Set `_isFullWidthSwiping` to `NO` when swipe gesture ends in `handleSwipe:` so programmatic push and pop can use OS logic by default. ## Screenshots / GIFs ### Before https://github.com/software-mansion/react-native-screens/assets/11507974/0f710aec-c8f0-4a3a-82df-d3edb5f0fb19 ### After https://github.com/software-mansion/react-native-screens/assets/11507974/51119c74-b2c3-4c32-9202-6954c3c70218 ## Test code and steps to reproduce 1. Run `Test2227` in TestsExample app. 2. Push screen with `Go to Details`. 3. Swipe back with full screen swipe gesture. 4. Push screen with `Go to Details` again. 7. Before this fix there would be no shadow during transition after first full screen swipe back. ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation:  - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes

…adow during swipe gesture (software-mansion#2239) ## Description When using full screen swipe back there was no shadow under the view. Related PR software-mansion#2234 with discussion about this change. Corresponding PR in `react-navigation`: * react-navigation/react-navigation#12053 ## Changes Added shadow to transition in animateSimplePushWithTransitionContext:toVC:fromVC:. ## Test code and steps to reproduce 1. Run `Test2227` in TestsExample app. 2. Push screen with Go to Details. 3. Swipe back with full screen swipe gesture. 5. Before this fix there would be no shadow during transition with full screen swipe back. ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [x] Updated documentation:  - [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes

maksg requested review from kkafar and WoLewicki July 5, 2024 14:56

kkafar reviewed Jul 8, 2024

View reviewed changes

tboba self-requested a review July 8, 2024 10:00

maksg force-pushed the @maksg/shadow-during-custom-push-pop-fix branch from 6a729b4 to 34420b2 Compare July 8, 2024 10:26

kkafar approved these changes Jul 8, 2024

View reviewed changes

WoLewicki reviewed Jul 8, 2024

View reviewed changes

maksg force-pushed the @maksg/shadow-during-custom-push-pop-fix branch from 34420b2 to c30c7d3 Compare July 8, 2024 12:02

maksg mentioned this pull request Jul 8, 2024

feat(iOS): add shadow to custom push pop transitions #2239

Merged

8 tasks

fix(iOS): shadow does not appear on manual transition during push and…

37d59ba

… pop

maksg force-pushed the @maksg/shadow-during-custom-push-pop-fix branch from c30c7d3 to 37d59ba Compare July 8, 2024 13:57

maksg changed the title ~~fix(iOS): shadow does not appear on manual transition during push and pop~~ fix(iOS): push and pop transitions change after full screen back swipe Jul 8, 2024

kkafar approved these changes Jul 9, 2024

View reviewed changes

maksg merged commit 8a13075 into main Jul 9, 2024
5 checks passed

maksg deleted the @maksg/shadow-during-custom-push-pop-fix branch July 9, 2024 09:26

WesleyR10 mentioned this pull request Aug 21, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.33.0 WesleyR10/IgniteTeams#5

Open

shonore mentioned this pull request Aug 22, 2024

[Snyk] Upgrade react-native-screens from 3.0.0 to 3.33.0 shonore/Fickle#1609

Open

2lambda123 mentioned this pull request Aug 22, 2024

[Snyk] Upgrade react-native-screens from 3.32.0 to 3.33.0 2lambda123/abp#163

Merged

Cabreira97 mentioned this pull request Aug 23, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.33.0 Cabreira97/Connect-Tech#5

Closed

2lambda123 mentioned this pull request Aug 26, 2024

[Snyk] Upgrade react-native-screens from 3.33.0 to 3.34.0 2lambda123/abp#164

Merged

shonore mentioned this pull request Aug 27, 2024

[Snyk] Upgrade react-native-screens from 3.0.0 to 3.34.0 shonore/Fickle#1634

Open

undrthegraveyard mentioned this pull request Sep 4, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.34.0 undrthegraveyard/gigslick#5

Closed

asraful-cse mentioned this pull request Sep 6, 2024

[Snyk] Upgrade react-native-screens from 3.11.1 to 3.34.0 asraful-cse/planet_app_react_native#412

Open

This was referenced Sep 22, 2024

[Snyk] Upgrade: , , , ramda, react-native, react-native-calendars, react-native-config, react-native-screens Olaw2jr/sushi#18

Open

[Snyk] Upgrade: , , , ramda, react-native, react-native-calendars, react-native-config, react-native-screens Olaw2jr/sushi#19

Open

Ayush-Bulbule mentioned this pull request Oct 2, 2024

[Snyk] Upgrade react-native-screens from 3.22.1 to 3.34.0 Ayush-Bulbule/FoodleApp#5

Merged

tobistudio mentioned this pull request Oct 3, 2024

[Snyk] Upgrade react-native-screens from 3.27.0 to 3.34.0 tobistudio/React_Native_Audio#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iOS): push and pop transitions change after full screen back swipe #2234

fix(iOS): push and pop transitions change after full screen back swipe #2234

maksg commented Jul 5, 2024 •

edited

Loading

kkafar left a comment

kkafar Jul 8, 2024

maksg Jul 8, 2024

kkafar Jul 8, 2024

kkafar Jul 8, 2024

maksg Jul 8, 2024

kkafar left a comment

kkafar Jul 8, 2024

WoLewicki left a comment

kkafar commented Jul 8, 2024

maksg commented Jul 8, 2024

jwoodmansey commented Jul 8, 2024

kkafar commented Jul 9, 2024

	// RNSPanGestureRecognizer will receive events if topScreen.fullScreenSwipeEnabled == YES;
	// RNSPanGestureRecognizer will receive events iff topScreen.fullScreenSwipeEnabled == YES;

	constexpr float RNSShadowViewMaxAlpha = 0.1;
	static constexpr float RNSShadowViewMaxAlpha = 0.1;

fix(iOS): push and pop transitions change after full screen back swipe #2234

fix(iOS): push and pop transitions change after full screen back swipe #2234

Conversation

maksg commented Jul 5, 2024 • edited Loading

Description

Changes

Screenshots / GIFs

Before

After

Test code and steps to reproduce

Checklist

kkafar left a comment

Choose a reason for hiding this comment

kkafar Jul 8, 2024

Choose a reason for hiding this comment

maksg Jul 8, 2024

Choose a reason for hiding this comment

kkafar Jul 8, 2024

Choose a reason for hiding this comment

kkafar Jul 8, 2024

Choose a reason for hiding this comment

maksg Jul 8, 2024

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

kkafar Jul 8, 2024

Choose a reason for hiding this comment

WoLewicki left a comment

Choose a reason for hiding this comment

kkafar commented Jul 8, 2024

maksg commented Jul 8, 2024

jwoodmansey commented Jul 8, 2024

kkafar commented Jul 9, 2024

maksg commented Jul 5, 2024 •

edited

Loading