mozilla-mobile / fenix

⚠️ Fenix (Firefox for Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
6.47k stars 1.27k forks source link

Improve 3-dot menu items visibility on small screens #13987

Closed BlitW0 closed 1 year ago

BlitW0 commented 4 years ago

When opening the 3-dot menu on a website, it does not look vertically scrollable. With the recent addition of Downloads option, the Add-ons sub-menu remains hidden at the top and is not intuitively accessible because the menu does not look scrollable. I would suggest making it more obvious that the menu is scrollable or decreasing the number of items in the 3-dot menu.

Android Device: Moto G5 Plus; Android 8.1 Fenix Version: Nightly 200820 06:08

┆Issue is synchronized with this Jira Task

janmechtel commented 3 years ago

I'd like to add that on my small unihertz jelly 2 screen with the URL bar at the bottom (which is the default i think), the most vital buttons at the bottom of the menu (forward, back, ..., refresh) are out of sight.

Meaning users need to scroll down all the time for these most used button.

I'd be great if the number of items is reduced again, or maybe the bottom is initially in view and users can scroll up?

janmechtel commented 3 years ago

I was digging a bit and DefaultToolbarMenu is passing on endOfMenuAlwaysVisible = shouldUseBottomToolbar led me to WebExtensionBrowserMenuBuilder and then BrowserMenu

There I found this comment:

 * @param endOfMenuAlwaysVisible when is set to true makes sure the bottom of the menu is always visible otherwise,
 *  the top of the menu is always visible. 

Which seems to be not working in my case. I haven't figured out why yet.

I'm wondering whether i should make this a separate issue because it's not about improving but rather looks like this part is not working as intended.

janmechtel commented 3 years ago

I found these two parts but I can't guess which part of the if clause is the relevant one. One the one hand the if explicitly mentions "AnchoredToBottom" on the other hand. the else also makes use of endOfMenuAlwaysVisible and is the only place that calls the method "setEndOfMenuAlwaysVisibleCompact".

But if the else branch, is the relevant one, why does it use StickyItemPlacement.TOP?

Shouldn't StickItemPlacement be dependent on endOfMenuAlwaysVisible

    @VisibleForTesting
    internal fun configureExpandableMenu(
        view: ViewGroup,
        endOfMenuAlwaysVisible: Boolean
    ): ViewGroup {
        // If the menu is placed at the bottom it should start as collapsed.
        if (menuPositioningData.inferredMenuPlacement is BrowserMenuPlacement.AnchoredToBottom.Dropdown ||
            menuPositioningData.inferredMenuPlacement is BrowserMenuPlacement.AnchoredToBottom.ManualAnchoring) {

            val collapsingMenuIndexLimit = adapter.visibleItems.indexOfFirst { it.isCollapsingMenuLimit }
            val stickyFooterPosition = adapter.visibleItems.indexOfLast { it.isSticky }
            if (collapsingMenuIndexLimit > 0) {
                return ExpandableLayout.wrapContentInExpandableView(
                    view,
                    collapsingMenuIndexLimit,
                    stickyFooterPosition
                ) { dismiss() }
            }
        } else {
            // The menu is by default set as a bottom one. Reconfigure it as a top one.
            menuList?.layoutManager = StickyItemsLinearLayoutManager.get<BrowserMenuAdapter>(
                view.context, StickyItemPlacement.TOP
            )

            // By default the menu is laid out from and scrolled to top - showing the top most items.
            // For the top menu it may be desired to initially show the bottom most items.
            menuList?.let { list ->
                list.setEndOfMenuAlwaysVisibleCompact(
                    endOfMenuAlwaysVisible, list.layoutManager as LinearLayoutManager
                )
            }
        }

        return view
    }

    private fun RecyclerView.setEndOfMenuAlwaysVisibleCompact(
        endOfMenuAlwaysVisible: Boolean,
        layoutManager: LinearLayoutManager
    ) {
        // In devices with Android 6 and below stackFromEnd is not working properly,
        // as a result, we have to provided a backwards support.
        // See: https://github.com/mozilla-mobile/android-components/issues/3211
        if (endOfMenuAlwaysVisible && Build.VERSION.SDK_INT <= Build.VERSION_CODES.M) {
            scrollOnceToTheBottom(this)
        } else {
            layoutManager.stackFromEnd = endOfMenuAlwaysVisible
        }
    }
csadilek commented 1 year ago

Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1809237

Change performed by the Move to Bugzilla add-on.