material-components / material-components-android

Modular and customizable Material Design UI components for Android
Apache License 2.0
16.34k stars 3.06k forks source link

[SearchView] When using android:enableOnBackInvokedCallback="true" , the SearchView is collapsed together with the keyboard, instead of just the keyboard #4185

Open AndroidDeveloperLB opened 5 months ago

AndroidDeveloperLB commented 5 months ago

Description: Full description of issue here I have a toolbar with a search menu item in it. I also use setOnActionExpandListener on it to handle some stuff, related to predictive back gesture . As such, I enable the predictive back gesture in the manifest:

  android:enableOnBackInvokedCallback="true" 

All works fine everywhere else, except here. When I put a focus on the SearchView and I enter something, and then press back key, it closes both the keyboard AND the Search View, making it impossible to go back to it and change it, and of course to see the results in case the search was performed. When the above is set to false, it works fine.

Expected behavior: Screenshots and/or description of expected behavior Should close only the keyboard first, and only if I press back key again, it should close the SearchView.

Source code: The code snippet which is causing this issue

    override fun onCreateOptionsMenu(menu: Menu): Boolean {
        // Inflate the menu; this adds items to the action bar if it is present.
        menuInflater.inflate(R.menu.menu_main, menu)
        menu.findItem(R.id.menuItem_search)
            .setOnActionExpandListener(object : OnActionExpandListener {
                override fun onMenuItemActionExpand(item: MenuItem): Boolean {
                    Log.d("AppLog", "onMenuItemActionExpand")
                    return true
                }

                override fun onMenuItemActionCollapse(item: MenuItem): Boolean {
                    Log.d("AppLog", "onMenuItemActionCollapse")
                    return true
                }
            })
        return true
    }
<menu xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    xmlns:tools="http://schemas.android.com/tools"
    tools:context="com.lb.myapplication.MainActivity">

    <item
        android:id="@+id/menuItem_search"
        android:icon="@android:drawable/ic_search_category_default"
        android:title="search"
        app:actionViewClass="androidx.appcompat.widget.SearchView"
        app:showAsAction="always|collapseActionView" />

    <item
        android:id="@+id/action_settings" android:orderInCategory="100"
        android:title="@string/action_settings" app:showAsAction="never" />
</menu>

Minimal sample app repro: Please consider attaching a minimal sample app that reproduces the issue. This will help narrow down the conditions required for reproducing the issue, and it will speed up the bug fix process. You may attach a zip file of the sample app or link to a GitHub repo that contains the sample app.

My Application.zip

Video:

studio64_2024-05-21_14-21-38.zip

Android API version: Android API version here 34

Material Library version: Material Android Library version you are using here (e.g., 1.1.0-alpha07)

implementation 'com.google.android.material:material:1.13.0-alpha02'

Device: Device on which the bug was encountered here Pixel 6

I can see a similar behavior on the sample of the repository, but here it's not in the toolbar at all, and it's also inconsistent. Watch:

scrcpy_2024-05-21_14-31-57.zip

I've also reported about this here but nobody has fixed it yet, and even my sample video and sample project were deleted by the forum: https://issuetracker.google.com/issues/326681254

AndroidDeveloperLB commented 5 months ago

Please update the sample with normal SearchView which is a part of a normal toolbar, and not just the fancy one. This can help to find such issues, and also handle them no matter what the app was chosen to use

dsn5ft commented 4 months ago

@AndroidDeveloperLB the SearchView embedded within a Toolbar is a somewhat outdated pattern, and it's not part of the Material spec anymore. Additionally, androidx.appcompat.widget.SearchView is not part of the Material library so we have no control over it.

Is there a reason you can't use the newer Material SearchBar + SearchView? Or if you don't want to use SearchBar, you could use a regular Toolbar with search icon and on click show the SearchView. That should be a better user experience because it gives more space for the user to enter search text, and also you can show search history / suggestions in the SearchView.

AndroidDeveloperLB commented 4 months ago

@dsn5ft

  1. So how can I have a search in a toolbar? I now have a toolbar with multiple action items. Where would I put something else? How would I migrate to something else without removing any functionality that users have?

  2. Why do you think the newer one is better, and you mark it as "completed" , if I've shown you that it occurs even here on this sample of this repository?

  3. What is the better user experience? It's still a search box. Users put text inside and that's it. What extra functionality is there here? There is no more space at all. It's still a single line to put text.

Please explain.

dsn5ft commented 4 months ago
  1. So how can I have a search in a toolbar? I now have a toolbar with multiple action items. Where would I put something else? How would I migrate to something else without removing any functionality that users have?

Well there are two potential options:

  1. You could still have the search icon in the toolbar, but instead of using app:actionViewClass="androidx.appcompat.widget.SearchView" you could show a Material SearchView on click of the search icon. That would not remove any of your current functionality.

  2. Use a Material SearchBar instead of a toolbar and put your additional action items in the SearchBar (examples). Clicking on the SearchBar itself would initiate the search, and the user would still have the ability to click on the additional action items.

image

Why do you think the newer one is better, and you mark it as "completed" , if I've shown you that it occurs even here on this sample of this repository?

As I mentioned, androidx.appcompat.widget.SearchView is not part of the Material library / repository. So this is not the right place to file the bug. I see you already filed one for AppCompat so I'd suggest continuing to follow up on that.

  1. What is the better user experience? It's still a search box. Users put text inside and that's it. What extra functionality is there here? There is no more space at all. It's still a single line to put text.

I didn't realize that the AppCompat SearchView also hides the additional action items, so I guess the space for searching is roughly the same.

One other potential advantage of the Material SearchView is that it allows you to put action items within the expanded search area, e.g. a microphone option for voice search, or any other actions that could be useful during search (like filters):

image
AndroidDeveloperLB commented 4 months ago
  1. Screenshots show it's more space for not more features. It looks nice though. I will still be able to perform all operations as I can do now on the Toolbar?

  2. Please fix the bug here. The one of the issue tracker wasn't fixed for a long time. Why did you close it here ? The bug exists on the sample itself, as I've shown. Also, please show each of the ways to implement in the sample, after you fix this bug.

Right now this bug exists on all types of search UIs, whether by appcompatx or material.

dsn5ft commented 4 months ago
  1. Screenshots show it's more space for not more features. It looks nice though. I will still be able to perform all operations as I can do now on the Toolbar?

The Material SearchBar extends from Toolbar, so you should be able to perform the same operations.

  1. Please fix the bug here. The one of the issue tracker wasn't fixed for a long time.

This whole time I actually thought you were talking about Gesture Navigation, since you mentioned predictive back, which does not have the issue:

https://github.com/material-components/material-components-android/assets/1420597/02ef1445-b9eb-4074-bee0-952e7c386d51

I only just realized that you are using the 3-Button Navigation mode, so I now see the keyboard issue for Material SearchView when using that mode. Reopening so we can investigate that.

AndroidDeveloperLB commented 4 months ago

Why would you think about gestures when I clearly showed you a video with 3 buttons navigation and I never mentioned gestures, and I also wrote "press back key"?

Also, the same issue occurs with gestures: When I make the back-gesture, instead of closing the keyboard and that's it, it closes both the keyboard and the search box completely.

Try it for yourself. Change android:enableOnBackInvokedCallback to false and compare with when it's true. You will see the behavior is different, even though it's just a back key/gesture. It should always have the same behavior : just close the keyboard alone first.

Please fix it for all types of navigation, and for all types of toolbars and search components. It doesn't make sense.

scrcpy_2024-05-23_22-08-08.zip

dsn5ft commented 4 months ago

Why would you think about gestures when I clearly showed you a video with 3 buttons navigation and I never mentioned gestures, and I also wrote "press back key"?

You started with "I also use setOnActionExpandListener on it to handle some stuff, related to predictive back gesture" and "I enable the predictive back gesture in the manifest", so that made it sound like you were using gesture navigation.

Additionally, as you can see below even when using gesture navigation there is still a back key when the keyboard is shown:

image

Regardless, it was a simple misunderstanding. Please be respectful. We will look into this issue.

AndroidDeveloperLB commented 4 months ago

@dsn5ft That's not a back key. That's a hide-keyboard key. Gesture navigation has no back key in the navigation bar. It has back gesture on the sides, and back-gesture always functions exactly as the back key, doesn't matter where it came from.

If you use a keyboard or the emulator's back key or use adb command, you will see that doing so (meaning back key and back gesture, as I've written already) has the same bug : It closes the keyboard and the search itself.

As for misunderstanding, again, just because I've implemented something doesn't mean I'm talking about a scenario that is related to it. I specifically mentioned pressing a key, I've shown 2 videos about it, and I've shown a link to the same issue on the issue tracker.

Please fix this bug on all cases. All navigation methods should be consistent in their behavior. All UI components should be consistent in their behavior : searchbar/toolbar/editText of all kinds .

AndroidDeveloperLB commented 4 months ago

@dsn5ft Actually, even on the main screen of the sample there is a problem with searching, as the keyboard doesn't always show up when reaching the search, and the back key/gesture might close the keyboard together with the Activity (not sure in which scenarios, but it happened).

I personally don't like any of the search/toolbar UIs that are on the sample. I prefer the original one we had, that has a title that's properly aligned to the language, with action buttons on the side.

A whole row of search makes sense only if the entire screen right now is for searching and there is only one main UI of the app.