Bug 1879370 - Add a ClippingBehavior supporting multiple toolbars

fenix/.buildconfig.yml

@@ -83,6 +83,7 @@ projects:
     - support-rusthttp
     - support-rustlog
     - support-test
+    - support-test-fakes
     - support-test-libstate
     - support-utils
     - support-webextensions

fenix/app/build.gradle

@@ -621,6 +621,7 @@ dependencies {
     implementation project(':lib-push-firebase')
     implementation project(':lib-state')
     implementation project(':lib-dataprotect')
+    testImplementation project(':support-test-fakes')
 
     debugImplementation ComponentsDependencies.leakcanary
     debugImplementation ComponentsDependencies.androidx_compose_ui_tooling

fenix/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt

@@ -63,6 +63,7 @@ import mozilla.components.browser.state.state.TabSessionState
 import mozilla.components.browser.state.state.content.DownloadState
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.browser.thumbnails.BrowserThumbnails
+import mozilla.components.browser.toolbar.BrowserToolbar
 import mozilla.components.concept.base.crash.Breadcrumb
 import mozilla.components.concept.engine.permission.SitePermissions
 import mozilla.components.concept.engine.prompt.ShareData
@@ -115,7 +116,6 @@ import mozilla.components.support.ktx.android.view.hideKeyboard
 import mozilla.components.support.ktx.kotlin.getOrigin
 import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifAnyChanged
 import mozilla.components.support.locale.ActivityContextWrapper
-import mozilla.components.ui.widgets.behavior.EngineViewClippingBehavior
 import mozilla.components.ui.widgets.withCenterAlignedButtons
 import org.mozilla.fenix.BuildConfig
 import org.mozilla.fenix.FeatureFlags
@@ -146,7 +146,9 @@ import org.mozilla.fenix.components.toolbar.interactor.BrowserToolbarInteractor
 import org.mozilla.fenix.components.toolbar.interactor.DefaultBrowserToolbarInteractor
 import org.mozilla.fenix.components.toolbar.navbar.BottomToolbarContainerView
 import org.mozilla.fenix.components.toolbar.navbar.BrowserNavBar
+import org.mozilla.fenix.components.toolbar.navbar.EngineViewClippingBehavior
 import org.mozilla.fenix.components.toolbar.navbar.NavbarIntegration
+import org.mozilla.fenix.components.toolbar.navbar.ToolbarContainerView
 import org.mozilla.fenix.compose.Divider
 import org.mozilla.fenix.crashes.CrashContentIntegration
 import org.mozilla.fenix.customtabs.ExternalAppBrowserActivity
@@ -183,7 +185,8 @@ import org.mozilla.fenix.utils.allowUndo
 import org.mozilla.fenix.wifi.SitePermissionsWifiIntegration
 import java.lang.ref.WeakReference
 import kotlin.coroutines.cancellation.CancellationException
-import mozilla.components.ui.widgets.behavior.ToolbarPosition as MozacToolbarPosition
+import mozilla.components.ui.widgets.behavior.EngineViewClippingBehavior as OldEngineViewClippingBehavior
+import mozilla.components.ui.widgets.behavior.ToolbarPosition as OldToolbarPosition
 
 /**
  * Base fragment extended by [BrowserFragment].
@@ -365,8 +368,6 @@ abstract class BaseBrowserFragment :
         val store = context.components.core.store
         val activity = requireActivity() as HomeActivity
 
-        val toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height)
-
         browserAnimator = BrowserAnimator(
             fragment = WeakReference(this),
             engineView = WeakReference(binding.engineView),
@@ -478,9 +479,12 @@ abstract class BaseBrowserFragment :
                 )
             }
 
+            val shouldHideOnScroll =
+                !context.settings().shouldUseFixedTopToolbar && context.settings().isDynamicToolbarEnabled
             _bottomToolbarContainerView = BottomToolbarContainerView(
                 context = context,
                 parent = binding.browserLayout,
+                hideOnScroll = shouldHideOnScroll,
                 composableContent = {
                     FirefoxTheme {
                         Column {
@@ -717,6 +721,8 @@ abstract class BaseBrowserFragment :
             },
         )
 
+        val bottomToolbarHeight = context.settings().getBottomToolbarHeight()
+
         downloadFeature.onDownloadStopped = { downloadState, _, downloadJobStatus ->
             handleOnDownloadFinished(downloadState, downloadJobStatus, downloadFeature::tryAgain)
         }
@@ -725,7 +731,7 @@ abstract class BaseBrowserFragment :
             getCurrentTab()?.id,
             store,
             context,
-            toolbarHeight,
+            bottomToolbarHeight,
         )
 
         shareDownloadsFeature.set(
@@ -1058,7 +1064,11 @@ abstract class BaseBrowserFragment :
             owner = this,
             view = view,
         )
-        initializeEngineView(toolbarHeight)
+
+        initializeEngineView(
+            topToolbarHeight = context.settings().getTopToolbarHeight(),
+            bottomToolbarHeight = bottomToolbarHeight,
+        )
     }
 
     protected fun showUndoSnackbar(message: String) {
@@ -1194,7 +1204,7 @@ abstract class BaseBrowserFragment :
         sessionId: String?,
         store: BrowserStore,
         context: Context,
-        toolbarHeight: Int,
+        bottomToolbarHeight: Int,
     ) {
         val savedDownloadState =
             sharedViewModel.downloadDialogState[sessionId]
@@ -1227,7 +1237,7 @@ abstract class BaseBrowserFragment :
                 showCannotOpenFileError(binding.dynamicSnackbarContainer, context, it)
             },
             binding = binding.viewDynamicDownloadDialog,
-            toolbarHeight = toolbarHeight,
+            bottomToolbarHeight = bottomToolbarHeight,
             onDismiss = onDismiss,
         ).show()
 
@@ -1241,37 +1251,58 @@ abstract class BaseBrowserFragment :
             !inFullScreen
     }
 
+    /**
+     * Sets up the necessary layout configurations for the engine view. If the toolbar is dynamic, this method sets a
+     * [CoordinatorLayout.Behavior] that will adjust the top/bottom paddings when the tab content is being scrolled.
+     * If the toolbar is not dynamic, it simply sets the top and bottom margins to ensure that content is always
+     * displayed above or below the respective toolbars.
+     *
+     * @param topToolbarHeight The height of the top toolbar, which could be zero if the toolbar is positioned at the
+     * bottom, or it could be equal to the height of [BrowserToolbar].
+     * @param bottomToolbarHeight The height of the bottom toolbar, which could be equal to the height of
+     * [BrowserToolbar] or [ToolbarContainerView], or zero if the toolbar is positioned at the top without a navigation
+     * bar.
+     */
     @VisibleForTesting
-    internal fun initializeEngineView(toolbarHeight: Int) {
+    internal fun initializeEngineView(
+        topToolbarHeight: Int,
+        bottomToolbarHeight: Int,
+    ) {
         val context = requireContext()
 
         if (!context.settings().shouldUseFixedTopToolbar && context.settings().isDynamicToolbarEnabled) {
-            getEngineView().setDynamicToolbarMaxHeight(toolbarHeight)
-
-            val toolbarPosition = when (context.settings().toolbarPosition) {
-                ToolbarPosition.BOTTOM -> MozacToolbarPosition.BOTTOM
-                ToolbarPosition.TOP -> MozacToolbarPosition.TOP
-            }
-            (getSwipeRefreshLayout().layoutParams as CoordinatorLayout.LayoutParams).behavior =
-                EngineViewClippingBehavior(
+            getEngineView().setDynamicToolbarMaxHeight(topToolbarHeight + bottomToolbarHeight)
+
+            if (IncompleteRedesignToolbarFeature(context.settings()).isEnabled) {
+                (getSwipeRefreshLayout().layoutParams as CoordinatorLayout.LayoutParams).behavior =
+                    EngineViewClippingBehavior(
+                        context = context,
+                        attrs = null,
+                        engineViewParent = getSwipeRefreshLayout(),
+                        topToolbarHeight = topToolbarHeight,
+                    )
+            } else {
+                val toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height)
+                val toolbarPosition = when (context.settings().toolbarPosition) {
+                    ToolbarPosition.BOTTOM -> OldToolbarPosition.BOTTOM
+                    ToolbarPosition.TOP -> OldToolbarPosition.TOP
+                }
+                OldEngineViewClippingBehavior(
                     context,
                     null,
                     getSwipeRefreshLayout(),
                     toolbarHeight,
                     toolbarPosition,
                 )
+            }
         } else {
             // Ensure webpage's bottom elements are aligned to the very bottom of the engineView.
             getEngineView().setDynamicToolbarMaxHeight(0)
 
-            // Effectively place the engineView on top/below of the toolbar if that is not dynamic.
-            val swipeRefreshParams =
-                getSwipeRefreshLayout().layoutParams as CoordinatorLayout.LayoutParams
-            if (context.settings().toolbarPosition == ToolbarPosition.TOP) {
-                swipeRefreshParams.topMargin = toolbarHeight
-            } else {
-                swipeRefreshParams.bottomMargin = toolbarHeight
-            }
+            // Effectively place the engineView on top/below of the toolbars if that is not dynamic.
+            val swipeRefreshParams = getSwipeRefreshLayout().layoutParams as CoordinatorLayout.LayoutParams
+            swipeRefreshParams.topMargin = topToolbarHeight
+            swipeRefreshParams.bottomMargin = bottomToolbarHeight
         }
     }
 
@@ -1349,9 +1380,9 @@ abstract class BaseBrowserFragment :
                 fullScreenChanged(false)
                 browserToolbarView.expand()
 
-                val toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height)
                 val context = requireContext()
-                resumeDownloadDialogState(selectedTab.id, context.components.core.store, context, toolbarHeight)
+                val bottomToolbarHeight = context.settings().getBottomToolbarHeight()
+                resumeDownloadDialogState(selectedTab.id, context.components.core.store, context, bottomToolbarHeight)
                 it.announceForAccessibility(selectedTab.toDisplayTitle())
             }
         } else {
@@ -1674,8 +1705,10 @@ abstract class BaseBrowserFragment :
             }
             if (webAppToolbarShouldBeVisible) {
                 browserToolbarView.view.isVisible = true
-                val toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height)
-                initializeEngineView(toolbarHeight)
+                initializeEngineView(
+                    topToolbarHeight = requireContext().settings().getTopToolbarHeight(),
+                    bottomToolbarHeight = requireContext().settings().getBottomToolbarHeight(),
+                )
                 browserToolbarView.expand()
             }
             if (customTabSessionId == null && requireContext().settings().isTabletAndTabStripEnabled) {

fenix/app/src/main/java/org/mozilla/fenix/browser/DownloadUtils.kt

@@ -7,8 +7,8 @@ package org.mozilla.fenix.browser
 import mozilla.components.browser.state.state.content.DownloadState
 import mozilla.components.browser.state.state.content.DownloadState.Status
 import mozilla.components.feature.downloads.AbstractFetchDownloadService
-import org.mozilla.fenix.R
 import org.mozilla.fenix.downloads.DynamicDownloadDialog
+import org.mozilla.fenix.ext.settings
 
 internal fun BaseBrowserFragment.handleOnDownloadFinished(
     downloadState: DownloadState,
@@ -43,7 +43,7 @@ internal fun BaseBrowserFragment.handleOnDownloadFinished(
                 tryAgain = tryAgain,
                 onCannotOpenFile = onCannotOpenFile,
                 binding = binding.viewDynamicDownloadDialog,
-                toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height),
+                bottomToolbarHeight = safeContext.settings().getBottomToolbarHeight(),
             ) { sharedViewModel.downloadDialogState.remove(downloadState.sessionId) }
 
             dynamicDownloadDialog.show()

fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/RedesignToolbarFeature.kt

@@ -4,6 +4,7 @@
 
 package org.mozilla.fenix.components.toolbar
 
+import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.utils.Settings
 
 /**

fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/BottomToolbarContainerView.kt

@@ -21,11 +21,13 @@ import mozilla.components.ui.widgets.behavior.ViewPosition
  *
  * @param context The Context the view is running in.
  * @param parent The ViewGroup into which the NavigationBar composable will be added.
+ * @param hideOnScroll If the container should react to the [EngineView] content being scrolled.
  * @param composableContent
  */
 class BottomToolbarContainerView(
     context: Context,
     parent: ViewGroup,
+    hideOnScroll: Boolean = false,
     composableContent: @Composable () -> Unit,
 ) {
 
@@ -46,7 +48,9 @@ class BottomToolbarContainerView(
             CoordinatorLayout.LayoutParams.WRAP_CONTENT,
         ).apply {
             gravity = Gravity.BOTTOM
-            behavior = EngineViewScrollingBehavior(parent.context, null, ViewPosition.BOTTOM)
+            if (hideOnScroll) {
+                behavior = EngineViewScrollingBehavior(parent.context, null, ViewPosition.BOTTOM)
+            }
         }
 
         parent.addView(toolbarContainerView)

fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/EngineViewClippingBehavior.kt

@@ -0,0 +1,109 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.fenix.components.toolbar.navbar
+
+import android.content.Context
+import android.util.AttributeSet
+import android.view.View
+import androidx.annotation.VisibleForTesting
+import androidx.coordinatorlayout.widget.CoordinatorLayout
+import mozilla.components.browser.toolbar.BrowserToolbar
+import mozilla.components.concept.engine.EngineView
+import mozilla.components.concept.toolbar.ScrollableToolbar
+import mozilla.components.support.ktx.android.view.findViewInHierarchy
+
+/**
+ * A modification of [mozilla.components.ui.widgets.behavior.EngineViewClippingBehavior] that supports two toolbars.
+ *
+ * This behavior adjusts the top margin of the [EngineView] parent to ensure that tab content is displayed
+ * right below the top toolbar when it is translating upwards. Additionally, it modifies the
+ * [EngineView.setVerticalClipping] when the bottom toolbar is translating downwards, ensuring that page content, like
+ * banners or webpage toolbars, is displayed right above the app toolbar.
+ *
+ * This class could be a candidate to replace the original and be integrated into A-C:
+ * https://bugzilla.mozilla.org/show_bug.cgi?id=1884835
+ *
+ * @param context [Context] for various Android interactions.
+ * @param attrs XML attributes configuring this behavior.
+ * @param engineViewParent The parent [View] of the [EngineView].
+ * @param topToolbarHeight The height of [ScrollableToolbar] when placed above the [EngineView].
+ */
+
+class EngineViewClippingBehavior(
+    context: Context?,
+    attrs: AttributeSet?,
+    private val engineViewParent: View,
+    private val topToolbarHeight: Int,
+) : CoordinatorLayout.Behavior<View>(context, attrs) {
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal var engineView = engineViewParent.findViewInHierarchy { it is EngineView } as EngineView?
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal var recentBottomToolbarTranslation = 0f
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal var recentTopToolbarTranslation = 0f
+
+    private val hasTopToolbar = topToolbarHeight > 0
+
+    override fun layoutDependsOn(parent: CoordinatorLayout, child: View, dependency: View): Boolean {
+        if (dependency is ScrollableToolbar) {
+            return true
+        }
+
+        return super.layoutDependsOn(parent, child, dependency)
+    }
+
+    // This method will be sequentially called with BrowserToolbar and ToolbarContainerView as dependencies when the
+    // navbar feature is on. Each call adjusts the translations of both elements and saves the most recent ones for
+    // future calls, ensuring that translations remain in sync. This is crucial, especially in cases where the toolbars
+    // have different sizes: as the top toolbar moves up, the bottom content clipping should be adjusted at twice the
+    // speed to compensate for the increased parent view height. However, once the top toolbar is completely hidden, the
+    // bottom content clipping should then move at the normal speed.
+    override fun onDependentViewChanged(parent: CoordinatorLayout, child: View, dependency: View): Boolean {
+        // Added NaN check for translationY as a precaution based on historical issues observed in
+        // [https://bugzilla.mozilla.org/show_bug.cgi?id=1823306]. This check aims to prevent similar issues, as
+        // confirmed by the test. Further investigation might be needed to identify all possible causes of NaN values.
+        if (dependency.translationY.isNaN()) {
+            return true
+        }
+
+        when (dependency) {
+            is BrowserToolbar -> {
+                if (hasTopToolbar) {
+                    recentTopToolbarTranslation = dependency.translationY
+                } else {
+                    recentBottomToolbarTranslation = dependency.translationY
+                }
+            }
+            is ToolbarContainerView -> recentBottomToolbarTranslation = dependency.translationY
+        }
+
+        engineView?.let {
+            if (hasTopToolbar) {
+                // Here we are adjusting the vertical position of
+                // the engine view container to be directly under
+                // the toolbar. The top toolbar is shifting up, so
+                // its translation will be either negative or zero.
+                // It might be safe to use the child view here, but the original
+                // implementation was adjusting the size of the parent passed
+                // to the class directly, and I feel cautious to change that
+                // considering possible side effects.
+                engineViewParent.translationY = recentTopToolbarTranslation + topToolbarHeight
+            }
+
+            // We want to position the engine view popup content
+            // right above the bottom toolbar when the toolbar
+            // is being shifted down. The top of the bottom toolbar
+            // is either positive or zero, but for clipping
+            // the values should be negative because the baseline
+            // for clipping is bottom toolbar height.
+            val contentBottomClipping = recentTopToolbarTranslation - recentBottomToolbarTranslation
+            it.setVerticalClipping(contentBottomClipping.toInt())
+        }
+        return true
+    }
+}