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
ggfan · Jun 9, 2021 · 06769a5 · 06769a5
1 parent 4370e11
commit 06769a5
Show file tree

Hide file tree

Showing 4 changed files with 183 additions and 23 deletions.
diff --git a/...ragment/src/androidTest/java/androidx/fragment/app/FragmentSharedElementTransitionTest.kt b/...ragment/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
@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="utf-8"?><!--
+  Copyright 2021 The Android Open Source Project
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+  -->
+
+<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    android:id="@+id/sharedElementContainer"
+    android:layout_width="match_parent"
+    android:layout_height="match_parent">
+    <FrameLayout android:id="@+id/containerBlueSquare"
+        android:transitionName="blueSquare"
+        android:layout_width="match_parent"
+        android:layout_height="match_parent"
+        android:background="#008">
+        <View android:id="@+id/greenSquare"
+            android:transitionName="greenSquare"
+            android:layout_width="100dp"
+            android:layout_height="100dp"
+            android:background="#080"/>
+        <View android:id="@+id/redSquare"
+            android:layout_width="100dp"
+            android:layout_height="100dp"
+            android:background="#800"
+            android:layout_gravity="end"/>
+    </FrameLayout>
+</FrameLayout>
diff --git a/fragment/fragment/src/androidTest/res/layout/scene7.xml b/fragment/fragment/src/androidTest/res/layout/scene7.xml
@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="utf-8"?><!--
+  Copyright 2021 The Android Open Source Project
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+  -->
+
+<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    android:id="@+id/sharedElementContainer"
+    android:layout_width="match_parent"
+    android:layout_height="match_parent">
+    <FrameLayout android:id="@+id/containerBlueSquare"
+        android:transitionGroup="true"
+        android:layout_width="match_parent"
+        android:layout_height="match_parent"
+        android:background="#008">
+        <View android:id="@+id/greenSquare"
+            android:transitionName="greenSquare"
+            android:layout_width="100dp"
+            android:layout_height="100dp"
+            android:background="#080"/>
+        <View android:id="@+id/redSquare"
+            android:layout_width="100dp"
+            android:layout_height="100dp"
+            android:background="#800"
+            android:layout_gravity="end"/>
+    </FrameLayout>
+</FrameLayout>
diff --git 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<String, View> sharedElementViews,
      */
     void captureTransitioningViews(ArrayList<View> 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 {