BottomSheetDialogFragment: Incorrect View hierarchy (id clashing) [180021387]

Fixed

Bug

Status Update

No update yet.

Description

ko...@gmail.com

created issue #1

Feb 12, 2021 01:33PM

Component used: Fragment Version used: 1.3.0 Devices/Android versions reproduced on: Any

Occurs when we use BottomSheetDialogFragment after updating androidx.fragment:fragment from 1.2.5 to 1.3.0.

Steps to reproduce:

Inherit BottomSheetDialogFragment.
In onCreateView return view inflated from a layout.
In the layout define FrameLayout/FragmentContainer with id container
Add a child fragment into a container with id container Expected: A child fragment is added into our container specified in the layout. Actual result: A child fragment added into the container used for CoordinatorLayout.

Expected view hierarchy (1.2.5):

DecorView
   - LinearLayout
      - FrameLayout
         - FitWindowsFrameLayout(id:action_bar_root)
            - ContentFrameLayout(id:content)
               - FrameLayout(id:container)
                  - CoordinatorLayout(id:coordinator)
                     - View(id:touch_outside)
                     - FrameLayout(id:design_bottom_sheet)
                        - LinearLayout(id:my_bottom_sheet_root)
                           - TextView(id:my_bottom_sheet_text)
                           - FragmentContainerView(id:container)
                              - FrameLayout(id:my_fragment_root)
                                 - TextView(id:my_fragment_text)

Actual view hierarchy (1.3.0):

DecorView
   - LinearLayout
      - FrameLayout
         - FitWindowsFrameLayout(id:action_bar_root)
            - ContentFrameLayout(id:content)
               - FrameLayout(id:container)
                  - CoordinatorLayout(id:coordinator)
                     - View(id:touch_outside)
                     - FrameLayout(id:design_bottom_sheet)
                        - LinearLayout(id:my_bottom_sheet_root)
                           - TextView(id:my_bottom_sheet_text)
                           - FragmentContainerView(id:container)
                  - FrameLayout(id:my_fragment_root)
                     - TextView(id:my_fragment_text)

It seems that it happens because in 1.3.0 a code for finding the right container to use for fragment transaction was changed. A new implementation for DialogFragment is looking for the container starting from the top of hierarchy (DecorView):

@Nullable
public <T extends View> T findViewById(@IdRes int id) {
    return mWindow.findViewById(id);
}

At the same time it's different for Fragment:

@NonNull
FragmentContainer createFragmentContainer() {
    return new FragmentContainer() {
        @Nullable
        public View onFindViewById(int id) {
            if (Fragment.this.mView == null) {
                throw new IllegalStateException("Fragment " + Fragment.this + " does not have a view");
            } else {
                return Fragment.this.mView.findViewById(id);
            }
        }

        public boolean onHasView() {
            return Fragment.this.mView != null;
        }
    };
}

The only workaround we found is to use a different id for our container, anything but container. But this doesn't seem appropriate as we could have a ton of BottomSheets in the project and this kind of id clashing is resolved very well in ordinary fragments.

WrongContainer.zip

141 KB

Download

Comments

il...@google.com <il...@google.com> #2Feb 12, 2021 10:29PM

Assigned to jb...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

ap...@google.com <ap...@google.com> #3Apr 15, 2021 04:22PM

Project: platform/frameworks/support
Branch: androidx-main

commit b72c198ba05378b4fe1e739c8992f850312742dd
Author: Jeremy Woods <jbwoods@google.com>
Date: Wed Apr 14 16:56:41 2021

Look at DialogFragment inflated layout before the dialog's

When using something like a BottomSheetDialog, because it reuses ids for
its child view, the dialog's view will be consider as the container
instead of the actual inflated container. To fix this, we should just
look at the inflated layout first.

RelNote: "You can now add fragment to `BottomSheetDialogFragment`s and
they will have a properly nested view hierarchy."
Test: DialogFragmentTest
Bug: 180021387

Change-Id: Ie62791677cb8771f22b19bdce5985426c669fba0

M fragment/fragment/src/androidTest/java/androidx/fragment/app/DialogFragmentTest.kt
M fragment/fragment/src/main/java/androidx/fragment/app/DialogFragment.java

https://android-review.googlesource.com/1676082

jb...@google.com <jb...@google.com> #4Apr 15, 2021 04:22PM

Marked as fixed.

This has been fixed internally and will be available in the Fragment 1.3.3 release.

Issue 180021387

Description

Issue summary

Comments

il...@google.com <il...@google.com> #2Feb 12, 2021 10:29PM

ap...@google.com <ap...@google.com> #3Apr 15, 2021 04:22PM

jb...@google.com <jb...@google.com> #4Apr 15, 2021 04:22PM

Add comment

Issue metadata