From 06769a572f7003fb662109b595952d6ff5c63a74 Mon Sep 17 00:00:00 2001 From: Jeremy Woods Date: Tue, 8 Jun 2021 16:35:28 -0700 Subject: [PATCH] Fix fragment transitions in new state manager In aosp/1679887 we attempted to fix doing shared element transitions on ViewGroups with the transitionGroup="false" attribute set. That change caused a regression in enter and exit transitions, and could also cause errors if the entering fragment attempted to transition more views than the exiting fragment. This change makes it so that fragments no longer looks at all of the views in the hierarchy of sharedelement views. We depend on transition names to determine if a view is a shared element, and if a shared element is a viewgroup with transitionGroup="true", the entering/exiting transition should ignore that viewgroup and transition its unnamed children. RelNote: "Fixed regression in shared element transtions caused by aosp/1679887. Setting transitionGroup=\"true\" will now work correctly and shared elements will no longer throw `IndexOutOfBoundsException`s." Test: FragmentSharedElementTransitionTest Bug: 188679569 Bug: 188969304 Change-Id: I164845639cae1f64cf54e4e89b2a24d7eb649beb --- .../FragmentSharedElementTransitionTest.kt | 99 ++++++++++++++++++- .../src/androidTest/res/layout/scene6.xml | 37 +++++++ .../src/androidTest/res/layout/scene7.xml | 37 +++++++ .../app/DefaultSpecialEffectsController.java | 33 +++---- 4 files changed, 183 insertions(+), 23 deletions(-) create mode 100644 fragment/fragment/src/androidTest/res/layout/scene6.xml create mode 100644 fragment/fragment/src/androidTest/res/layout/scene7.xml diff --git a/fragment/fragment/src/androidTest/java/androidx/fragment/app/FragmentSharedElementTransitionTest.kt b/fragment/fragment/src/androidTest/java/androidx/fragment/app/FragmentSharedElementTransitionTest.kt index b3491c79f672b..c0f6bfa7a468a 100644 --- a/fragment/fragment/src/androidTest/java/androidx/fragment/app/FragmentSharedElementTransitionTest.kt +++ b/fragment/fragment/src/androidTest/java/androidx/fragment/app/FragmentSharedElementTransitionTest.kt @@ -64,7 +64,7 @@ class FragmentSharedElementTransitionTest { } @Test - fun testNestedSharedElementViewNonMatching() { + fun testNestedSharedElementViewsMoreOutViews() { with(ActivityScenario.launch(FragmentTestActivity::class.java)) { val fragment = TransitionFragment(R.layout.scene5) withActivity { @@ -80,6 +80,11 @@ class FragmentSharedElementTransitionTest { val redSquare = withActivity { findViewById(R.id.redSquare) } val startBlueBounds = containerBlueSquare.boundsOnScreen + fragment.enterTransition.verifyAndClearTransition { + enteringViews += listOf(greenSquare, redSquare) + } + verifyNoOtherTransitions(fragment) + val fragment2 = TransitionFragment(R.layout.scene4) withActivity { @@ -93,16 +98,102 @@ class FragmentSharedElementTransitionTest { val blueSquare = withActivity { findViewById(R.id.blueSquare) } - fragment.enterTransition.verifyAndClearTransition { - enteringViews += listOf(containerBlueSquare, greenSquare, redSquare) + fragment.exitTransition.verifyAndClearTransition { + exitingViews += listOf(greenSquare, redSquare) + epicenter = startBlueBounds } fragment2.sharedElementEnter.verifyAndClearTransition { enteringViews += listOf(blueSquare) - exitingViews += listOf(containerBlueSquare, greenSquare) + exitingViews += listOf(containerBlueSquare) epicenter = startBlueBounds } verifyNoOtherTransitions(fragment) verifyNoOtherTransitions(fragment2) } } + + @Test + fun testNestedSharedElementViewsMoreInViews() { + with(ActivityScenario.launch(FragmentTestActivity::class.java)) { + val fragment = TransitionFragment(R.layout.scene4) + withActivity { + supportFragmentManager + .beginTransaction() + .setReorderingAllowed(true) + .replace(R.id.content, fragment) + .commit() + } + + val blueSquare = withActivity { findViewById(R.id.blueSquare) } + val startBlueBounds = blueSquare.boundsOnScreen + + fragment.enterTransition.verifyAndClearTransition { + enteringViews += listOf(blueSquare) + } + verifyNoOtherTransitions(fragment) + + val fragment2 = TransitionFragment(R.layout.scene6) + + withActivity { + supportFragmentManager + .beginTransaction() + .setReorderingAllowed(true) + .addSharedElement(blueSquare, "blueSquare") + .replace(R.id.content, fragment2) + .commit() + } + + val containerBlueSquare = withActivity { findViewById(R.id.containerBlueSquare) } + + fragment2.sharedElementEnter.verifyAndClearTransition { + exitingViews += listOf(blueSquare) + enteringViews += listOf(containerBlueSquare) + epicenter = startBlueBounds + } + verifyNoOtherTransitions(fragment) + verifyNoOtherTransitions(fragment2) + } + } + + @Test + fun testNestedTransitionGroupTrue() { + with(ActivityScenario.launch(FragmentTestActivity::class.java)) { + val fragment = TransitionFragment(R.layout.scene7) + withActivity { + supportFragmentManager + .beginTransaction() + .setReorderingAllowed(true) + .replace(R.id.content, fragment) + .commit() + } + + val containerBlueSquare = withActivity { findViewById(R.id.containerBlueSquare) } + + fragment.enterTransition.verifyAndClearTransition { + enteringViews += listOf(containerBlueSquare) + } + verifyNoOtherTransitions(fragment) + + val fragment2 = TransitionFragment(R.layout.scene4) + + withActivity { + supportFragmentManager + .beginTransaction() + .setReorderingAllowed(true) + .replace(R.id.content, fragment2) + .commit() + } + + val blueSquare = withActivity { findViewById(R.id.blueSquare) } + + fragment.exitTransition.verifyAndClearTransition { + exitingViews += listOf(containerBlueSquare) + } + fragment2.enterTransition.verifyAndClearTransition { + enteringViews += listOf(blueSquare) + } + verifyNoOtherTransitions(fragment) + verifyNoOtherTransitions(fragment2) + } + } } diff --git a/fragment/fragment/src/androidTest/res/layout/scene6.xml b/fragment/fragment/src/androidTest/res/layout/scene6.xml new file mode 100644 index 0000000000000..552ee6f6d7991 --- /dev/null +++ b/fragment/fragment/src/androidTest/res/layout/scene6.xml @@ -0,0 +1,37 @@ + + + + + + + + \ No newline at end of file diff --git a/fragment/fragment/src/androidTest/res/layout/scene7.xml b/fragment/fragment/src/androidTest/res/layout/scene7.xml new file mode 100644 index 0000000000000..76ad6ee951ebb --- /dev/null +++ b/fragment/fragment/src/androidTest/res/layout/scene7.xml @@ -0,0 +1,37 @@ + + + + + + + + \ No newline at end of file diff --git a/fragment/fragment/src/main/java/androidx/fragment/app/DefaultSpecialEffectsController.java b/fragment/fragment/src/main/java/androidx/fragment/app/DefaultSpecialEffectsController.java index 5b2884d5b82c3..20cf2c6c0ff82 100644 --- a/fragment/fragment/src/main/java/androidx/fragment/app/DefaultSpecialEffectsController.java +++ b/fragment/fragment/src/main/java/androidx/fragment/app/DefaultSpecialEffectsController.java @@ -33,6 +33,7 @@ import androidx.core.util.Preconditions; import androidx.core.view.OneShotPreDrawListener; import androidx.core.view.ViewCompat; +import androidx.core.view.ViewGroupCompat; import java.util.ArrayList; import java.util.Collection; @@ -459,11 +460,7 @@ public void run() { } }); - // Capture all views from the firstOut Fragment under the shared element views - for (View sharedElementView : firstOutViews.values()) { - captureTransitioningViews(sharedElementFirstOutViews, - sharedElementView); - } + sharedElementFirstOutViews.addAll(firstOutViews.values()); // Compute the epicenter of the firstOut transition if (!exitingNames.isEmpty()) { @@ -473,11 +470,7 @@ public void run() { firstOutEpicenterView); } - // Capture all views from the lastIn Fragment under the shared element views - for (View sharedElementView : lastInViews.values()) { - captureTransitioningViews(sharedElementLastInViews, - sharedElementView); - } + sharedElementLastInViews.addAll(lastInViews.values()); // Compute the epicenter of the lastIn transition if (!enteringNames.isEmpty()) { @@ -700,16 +693,18 @@ void retainMatchingViews(@NonNull ArrayMap sharedElementViews, */ void captureTransitioningViews(ArrayList transitioningViews, View view) { if (view instanceof ViewGroup) { - if (!transitioningViews.contains(view) - && ViewCompat.getTransitionName(view) != null) { - transitioningViews.add(view); - } ViewGroup viewGroup = (ViewGroup) view; - int count = viewGroup.getChildCount(); - for (int i = 0; i < count; i++) { - View child = viewGroup.getChildAt(i); - if (child.getVisibility() == View.VISIBLE) { - captureTransitioningViews(transitioningViews, child); + if (ViewGroupCompat.isTransitionGroup(viewGroup)) { + if (!transitioningViews.contains(view)) { + transitioningViews.add(viewGroup); + } + } else { + int count = viewGroup.getChildCount(); + for (int i = 0; i < count; i++) { + View child = viewGroup.getChildAt(i); + if (child.getVisibility() == View.VISIBLE) { + captureTransitioningViews(transitioningViews, child); + } } } } else {