streetcomplete / StreetComplete

Easy to use OpenStreetMap editor for Android
https://streetcomplete.app
GNU General Public License v3.0
3.91k stars 357 forks source link

60.0-alpha1+ answers are unclickable as they occupy same space as navbar #6030

Closed mnalis closed 6 hours ago

mnalis commented 3 days ago

In SC 60.0-alpha1 quest answer buttons (esp. for single-line quests like yes/no) are displayed at the same place as the (semi-transparent) android navigation bar, making them unclickable. That is a regression; it worked fine in SC 59.3 and below.

How to Reproduce

example videos of working (59.3) version:

https://github.com/user-attachments/assets/e4e81abf-4bee-4f1b-a1cb-fba89b23f16e

example of failing version (60.0-alpha1):

https://github.com/user-attachments/assets/fa4fee9c-14e9-4135-b23f-22b8a1b1d4fe

(unfortunately recording does not show that I attempt to click several times on the buttons at e.g. 0:05-0:10) Attempting to move the screen so answers are clickable resulted in failure - the new versions of the app remains unusable on that phone.

I seem to recall perhaps we might've had similar issues before? But I am unable to find it... (there exists https://github.com/streetcomplete/StreetComplete/issues/5861 but that one was just obscured by default and could be moved to clickable state -- which this one is not). I could be misremembering or conflating the two, though :man_shrugging:

Expected Behavior answers should be displayed at different place then navbar, so they remain clickable, just as they were before 60.0.

Versions affected SC 60.0-alpha2 (and 60.0-alpha1), Android 9 (MIUI 12.0.1) on Xiaomi Redmi Note 6 Pro

westnordost commented 3 days ago

Thanks for the issue report!

westnordost commented 2 days ago

I cannot reproduce this on my device. Can you reproduce this on any recent device?

mnalis commented 2 days ago

I cannot reproduce this on my device. Can you reproduce this on any recent device?

How recent are we talking about? :man_shrugging: I can also reproduce it in my previous phone Huawei P30 Pro (Android 10 / EMUI 12), so at least it is not single-device problem:

small_Screenshot_20241129_025956_de westnordost streetcomplete

Only newer phone that I have is Samsung Galaxy S23+ (Android 14 / OneUI 6.1) and it doesn't have the problem.

westnordost commented 2 days ago

Would be good if you tested this on more phones, maybe we'll see a pattern. Apparently you have some sort of museum in your drawer ;-)

It is curious, though. All those quest forms were the only thing that hasn't changed in v60. But for v59, you cannot reproduce it, as far as I understand.

westnordost commented 2 days ago

Can reproduce on Android 10 and below emulators

westnordost commented 2 days ago

So, if I remove MainScreen.kt line 223 .safeDrawingPadding(), the bottom sheet forms all get back their window inset padding. But of course, the controls on the main screen don't have it anymore.

This function gets the window insets (navigation bar, status bar, camera cutouts, IME...) and applies this as a padding to the widget, then consumes the insets, so that to child widgets, no additional padding would be applied when they use .safeDrawingPadding(). The bottom sheets however are not children to the controls, they are not even siblings. So, the bottom sheets should also get the window insets - for some reason, they don't. Well, at least on Android 10 and below they don't.

Actually, it is even enough to just query and not even consume the window insets via val insets = WindowInsets.safeDrawing in the MainScreen to let the bottom sheets not be notified about any insets anymore. So, this is really weird. Because, another compose widget that is part of the MainScreen and a sibling to the controls, the ̀ EditHistorySidebar` also accesses the insets and consumes them, yet, they don't trigger the bug at all.

westnordost commented 1 day ago

So, I think this must be a bug in Android in API level 29 and below.

StackOverflow is full of issues with applying window insets correctly. But Google changed the API for (dealing with) window insets, navigation and status bars about every second Android version or so, so, naturally, this at least created a lot of confusion but I bet some bugs, too. Recently, the androidx library finally introduced a unifying interface for this, and you see by the implementation that I wasn't exaggerating about every second Android version or so:

https://android.googlesource.com/platform/frameworks/support/+/HEAD/activity/activity/src/main/java/androidx/activity/EdgeToEdge.kt#77

Compose of course (thank god) also has a straightforward API, too.

Anyway.

The issue seems to be this:

Prior to R, dispatchApplyWindowInsets had an issue: The modified insets changed by onApplyWindowInsets were passed to the entire view hierarchy in prefix order, including siblings as well as siblings of parents further down the hierarchy. This violates the basic concepts of the view hierarchy, and thus, the hierarchical dispatching mechanism was hard to use for apps. In order to make window inset dispatching work properly, we dispatch window insets in the view hierarchy in a proper hierarchical manner if this flag is set to false.

(Source: documentation comment on static boolean sBrokenInsetsDispatch; in Android API 34 source code for View.java)

So, a widget in the Compose code (the controls on top of the map) consume the window insets. From the point of view of the Android view system, Compose is just another view. That view is a sibling to the bottom sheet fragment(s). So, with this "broken insets dispatch", the dispatching of window insets stops after the compose view. Somehow, we need to set this sBrokenInsetsDispatch to false, but it is not part of the public API!

westnordost commented 1 day ago

While on Android API 34, it is simply that (so no problem, sBrokenInsetsDispatch = false)

sBrokenInsetsDispatch = targetSdkVersion < Build.VERSION_CODES.R;

, curiously, the code looks like this for API 29:

sBrokenInsetsDispatch = ViewRootImpl.sNewInsetsMode != NEW_INSETS_MODE_FULL
                    || targetSdkVersion < Build.VERSION_CODES.Q;

What the hell is even that? This "newInsetsMode" vanished from the codebase after API 29. Browsing the source code further...

/**
 * If set to 2, the view system will switch from using rectangles retrieved from window to
 * dispatch to the view hierarchy to using {@link InsetsController}, that derives the insets
 * directly from the full configuration, enabling richer information about the insets state, as
 * well as new APIs to control it frame-by-frame, and synchronize animations with it.
 * <p>
 * Only set this to 2 once the new insets system is productionized and the old APIs are
 * fully migrated over.
 * <p>
 * If set to 1, this will switch to a mode where we only use the new approach for IME, but not
 * for the status/navigation bar.
 */
private static final String USE_NEW_INSETS_PROPERTY = "persist.wm.new_insets";

sNewInsetsMode =
            SystemProperties.getInt(USE_NEW_INSETS_PROPERTY, 0);

Hu, so I need to set that system property? Can I even do that as an app? I don't think so...

westnordost commented 1 day ago

So, I can't really think of a solution. It's just broken behavior by Android, and no chance to adjust it. It would only be solved by migrating all the stuff that is currently shown in the bottom sheet to compose... that would take a while....

mnalis commented 1 day ago

So, this is really weird. Because, another compose widget that is part of the MainScreen and a sibling to the controls, the ̀ EditHistorySidebar also accesses the insets and consumes them, yet, they don't trigger the bug at all.

I'm completely out of my depth here, but if EditHistorySidebar works correctly, could we not attempt to employ the same manual access/consuming of insets (e.g. .consumeWindowInsets(insets)) in MainScreen too? Perhaps it would sidestep the bug there too?

mnalis commented 1 day ago

Worst case (if we really can't find a better solution), we could disable full-screen mode (i.e. skip calling enableEdgeToEdge in MainActivity.kt) on Android 10 and below? This seems to work around the issue (at a loss of some nicer-looking partial view of the map at the bottom of the screen, of course):

small_Screenshot_20241130_145455_de westnordost streetcomplete debug

It would not be ideal, but certainly better then unclickable quests I guess...

westnordost commented 6 hours ago

I've found another workaround.

mnalis commented 55 minutes ago

I've found another workaround.

Thanks! I can confirm https://github.com/streetcomplete/StreetComplete/commit/524c029d59b5d035fa25b9ad2c4eb2f514afd3e9 fixing my Android 9 and 10 devices, while not breaking otherwise working devices with Android 6 (with separate navbar area) & Android 14.