From c9422e81eaa9372c77d2c3c04da8fe2998856a97 Mon Sep 17 00:00:00 2001 From: mike a Date: Mon, 11 Mar 2024 18:58:58 -0700 Subject: [PATCH] Bug 1879370 - Add a ClippingBehavior supporting multiple toolbars --- .buildconfig.yml | 1 + app/build.gradle | 1 + .../fenix/browser/BaseBrowserFragment.kt | 91 ++++-- .../mozilla/fenix/browser/DownloadUtils.kt | 4 +- .../toolbar/RedesignToolbarFeature.kt | 1 + .../navbar/BottomToolbarContainerView.kt | 6 +- .../navbar/EngineViewClippingBehavior.kt | 109 +++++++ .../toolbar/navbar/NavigationBar.kt | 3 +- .../fenix/downloads/DynamicDownloadDialog.kt | 6 +- .../org/mozilla/fenix/home/HomeFragment.kt | 1 + .../java/org/mozilla/fenix/utils/Settings.kt | 34 ++ app/src/main/res/values/dimens.xml | 1 + .../fenix/browser/BaseBrowserFragmentTest.kt | 67 +++- .../toolbar/EngineViewClippingBehaviorTest.kt | 292 ++++++++++++++++++ 14 files changed, 572 insertions(+), 45 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/EngineViewClippingBehavior.kt create mode 100644 app/src/test/java/org/mozilla/fenix/components/toolbar/EngineViewClippingBehaviorTest.kt diff --git a/.buildconfig.yml b/.buildconfig.yml index 10c28a1ff..11c872884 100644 --- a/.buildconfig.yml +++ b/.buildconfig.yml @@ -83,6 +83,7 @@ projects: - support-rusthttp - support-rustlog - support-test + - support-test-fakes - support-test-libstate - support-utils - support-webextensions diff --git a/app/build.gradle b/app/build.gradle index e1b560840..ce2174c4d 100644 --- a/app/build.gradle +++ b/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 diff --git a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt index 6de923a5a..59a6163d2 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/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) { diff --git a/app/src/main/java/org/mozilla/fenix/browser/DownloadUtils.kt b/app/src/main/java/org/mozilla/fenix/browser/DownloadUtils.kt index ed0930f76..034f2b811 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/DownloadUtils.kt +++ b/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() diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/RedesignToolbarFeature.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/RedesignToolbarFeature.kt index 2529b038b..a65bda796 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/RedesignToolbarFeature.kt +++ b/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 /** diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/BottomToolbarContainerView.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/BottomToolbarContainerView.kt index 12462c770..1c638ea11 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/BottomToolbarContainerView.kt +++ b/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) diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/EngineViewClippingBehavior.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/EngineViewClippingBehavior.kt new file mode 100644 index 000000000..2e2d5cdeb --- /dev/null +++ b/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(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 + } +} diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/NavigationBar.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/NavigationBar.kt index 1cedb93bf..90c89470b 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/NavigationBar.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/NavigationBar.kt @@ -20,6 +20,7 @@ import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.getValue import androidx.compose.ui.Modifier import androidx.compose.ui.platform.LocalContext +import androidx.compose.ui.res.dimensionResource import androidx.compose.ui.res.painterResource import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.Preview @@ -177,7 +178,7 @@ private fun NavBar( Row( modifier = Modifier .background(FirefoxTheme.colors.layer1) - .height(48.dp) + .height(dimensionResource(id = R.dimen.browser_navbar_height)) .fillMaxWidth() .padding(horizontal = 16.dp), horizontalArrangement = Arrangement.SpaceBetween, diff --git a/app/src/main/java/org/mozilla/fenix/downloads/DynamicDownloadDialog.kt b/app/src/main/java/org/mozilla/fenix/downloads/DynamicDownloadDialog.kt index 6d7e00a2c..fee4711c2 100644 --- a/app/src/main/java/org/mozilla/fenix/downloads/DynamicDownloadDialog.kt +++ b/app/src/main/java/org/mozilla/fenix/downloads/DynamicDownloadDialog.kt @@ -30,7 +30,7 @@ class DynamicDownloadDialog( private val tryAgain: (String) -> Unit, private val onCannotOpenFile: (DownloadState) -> Unit, private val binding: DownloadDialogLayoutBinding, - private val toolbarHeight: Int, + private val bottomToolbarHeight: Int, private val onDismiss: () -> Unit, ) { @@ -49,7 +49,7 @@ class DynamicDownloadDialog( DynamicDownloadDialogBehavior( context, null, - toolbarHeight.toFloat(), + bottomToolbarHeight.toFloat(), ) } } @@ -58,7 +58,7 @@ class DynamicDownloadDialog( if (settings.shouldUseBottomToolbar) { val params: ViewGroup.MarginLayoutParams = binding.root.layoutParams as ViewGroup.MarginLayoutParams - params.bottomMargin = toolbarHeight + params.bottomMargin = bottomToolbarHeight } if (didFail) { diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index 983e57943..1d1dc0d61 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -475,6 +475,7 @@ class HomeFragment : Fragment() { _bottomToolbarContainerView = BottomToolbarContainerView( context = requireContext(), parent = binding.homeLayout, + hideOnScroll = false, composableContent = { FirefoxTheme { Column { diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 5fe16784f..cbffc1b74 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -1983,6 +1983,40 @@ class Settings(private val appContext: Context) : PreferencesHolder { featureFlag = FeatureFlags.completeToolbarRedesignEnabled, ) + /** + * Returns the height of the bottom toolbar. + * + * The bottom toolbar can consist of a navigation bar, + * a combination of a navigation and address bar, or be absent. + */ + fun getBottomToolbarHeight(): Int { + val isNavBarEnabled = enableIncompleteToolbarRedesign + val isToolbarAtBottom = shouldUseBottomToolbar + val toolbarHeight = appContext.resources.getDimensionPixelSize(R.dimen.browser_toolbar_height) + val navbarHeight = appContext.resources.getDimensionPixelSize(R.dimen.browser_navbar_height) + + return when { + isNavBarEnabled && isToolbarAtBottom -> toolbarHeight + navbarHeight + isNavBarEnabled -> navbarHeight + isToolbarAtBottom -> toolbarHeight + else -> 0 + } + } + + /** + * Returns the height of the top toolbar. + */ + fun getTopToolbarHeight(): Int { + val isToolbarAtTop = !shouldUseBottomToolbar + val toolbarHeight = appContext.resources.getDimensionPixelSize(R.dimen.browser_toolbar_height) + + return if (isToolbarAtTop) { + toolbarHeight + } else { + 0 + } + } + /** * Indicates if the user is shown incomplete new redesigned Toolbar UI components and behaviors. * diff --git a/app/src/main/res/values/dimens.xml b/app/src/main/res/values/dimens.xml index 8e990dae9..32003466d 100644 --- a/app/src/main/res/values/dimens.xml +++ b/app/src/main/res/values/dimens.xml @@ -70,6 +70,7 @@ 56dp + 48dp 0dp diff --git a/app/src/test/java/org/mozilla/fenix/browser/BaseBrowserFragmentTest.kt b/app/src/test/java/org/mozilla/fenix/browser/BaseBrowserFragmentTest.kt index ac0a109f9..56708c828 100644 --- a/app/src/test/java/org/mozilla/fenix/browser/BaseBrowserFragmentTest.kt +++ b/app/src/test/java/org/mozilla/fenix/browser/BaseBrowserFragmentTest.kt @@ -21,9 +21,9 @@ import mozilla.components.concept.engine.EngineView import mozilla.components.concept.engine.permission.SitePermissions import mozilla.components.feature.contextmenu.ContextMenuCandidate import mozilla.components.ui.widgets.VerticalSwipeRefreshLayout -import mozilla.components.ui.widgets.behavior.EngineViewClippingBehavior import org.junit.Before import org.junit.Test +import org.mozilla.fenix.components.toolbar.navbar.EngineViewClippingBehavior import org.mozilla.fenix.ext.components import org.mozilla.fenix.utils.Settings @@ -56,7 +56,10 @@ class BaseBrowserFragmentTest { every { settings.shouldUseBottomToolbar } returns false every { settings.shouldUseFixedTopToolbar } returns true - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 0, + ) verify { engineView.setDynamicToolbarMaxHeight(0) } } @@ -66,7 +69,10 @@ class BaseBrowserFragmentTest { every { settings.shouldUseBottomToolbar } returns true every { settings.shouldUseFixedTopToolbar } returns true - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 13, + ) verify { engineView.setDynamicToolbarMaxHeight(0) } } @@ -76,7 +82,10 @@ class BaseBrowserFragmentTest { every { settings.shouldUseFixedTopToolbar } returns false every { settings.isDynamicToolbarEnabled } returns true - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 0, + ) verify { engineView.setDynamicToolbarMaxHeight(13) } } @@ -86,7 +95,10 @@ class BaseBrowserFragmentTest { every { settings.shouldUseFixedTopToolbar } returns false every { settings.isDynamicToolbarEnabled } returns false - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 0, + ) verify { engineView.setDynamicToolbarMaxHeight(0) } } @@ -95,12 +107,16 @@ class BaseBrowserFragmentTest { fun `initializeEngineView should set EngineViewClippingBehavior when dynamic toolbar is enabled`() { every { settings.shouldUseFixedTopToolbar } returns false every { settings.isDynamicToolbarEnabled } returns true + every { settings.enableIncompleteToolbarRedesign } returns true val params: CoordinatorLayout.LayoutParams = mockk(relaxed = true) every { params.behavior } returns mockk(relaxed = true) every { swipeRefreshLayout.layoutParams } returns params val behavior = slot() - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 0, + ) // EngineViewClippingBehavior constructor parameters are not properties, we cannot check them. // Ensure just that the right behavior is set. @@ -112,7 +128,10 @@ class BaseBrowserFragmentTest { every { settings.isDynamicToolbarEnabled } returns false every { settings.shouldUseBottomToolbar } returns true - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 0, + bottomToolbarHeight = 13, + ) verify { (swipeRefreshLayout.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = 13 } } @@ -122,7 +141,10 @@ class BaseBrowserFragmentTest { every { settings.shouldUseBottomToolbar } returns false every { settings.shouldUseFixedTopToolbar } returns true - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 0, + ) verify { (swipeRefreshLayout.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = 13 } } @@ -132,7 +154,10 @@ class BaseBrowserFragmentTest { every { settings.shouldUseBottomToolbar } returns true every { settings.shouldUseFixedTopToolbar } returns true - fragment.initializeEngineView(13) + fragment.initializeEngineView( + topToolbarHeight = 0, + bottomToolbarHeight = 13, + ) verify { (swipeRefreshLayout.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = 13 } } @@ -199,6 +224,30 @@ class BaseBrowserFragmentTest { assertFalse(result) } } + + @Test + fun `WHEN initializeEngineView is called THEN setDynamicToolbarMaxHeight sets max height to the engine view as a sum of two toolbars heights`() { + every { settings.shouldUseFixedTopToolbar } returns false + every { settings.isDynamicToolbarEnabled } returns true + + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 0, + ) + verify { engineView.setDynamicToolbarMaxHeight(13) } + + fragment.initializeEngineView( + topToolbarHeight = 0, + bottomToolbarHeight = 13, + ) + verify { engineView.setDynamicToolbarMaxHeight(13) } + + fragment.initializeEngineView( + topToolbarHeight = 13, + bottomToolbarHeight = 13, + ) + verify { engineView.setDynamicToolbarMaxHeight(26) } + } } private class TestBaseBrowserFragment : BaseBrowserFragment() { diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/EngineViewClippingBehaviorTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/EngineViewClippingBehaviorTest.kt new file mode 100644 index 000000000..688a3c1d9 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/EngineViewClippingBehaviorTest.kt @@ -0,0 +1,292 @@ +/* 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 + +import android.view.View +import android.widget.EditText +import android.widget.ImageView +import android.widget.TextView +import androidx.test.ext.junit.runners.AndroidJUnit4 +import mozilla.components.browser.toolbar.BrowserToolbar +import mozilla.components.concept.engine.EngineView +import mozilla.components.support.test.fakes.engine.FakeEngineView +import mozilla.components.support.test.mock +import mozilla.components.support.test.robolectric.testContext +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.doReturn +import org.mockito.Mockito.spy +import org.mockito.Mockito.verify +import org.mozilla.fenix.components.toolbar.navbar.EngineViewClippingBehavior +import org.mozilla.fenix.components.toolbar.navbar.ToolbarContainerView + +@RunWith(AndroidJUnit4::class) +class EngineViewClippingBehaviorTest { + + // Bottom toolbar position tests + @Test + fun `GIVEN the toolbar is at the bottom WHEN toolbar is being shifted THEN EngineView adjusts bottom clipping && EngineViewParent position doesn't change`() { + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: BrowserToolbar = mock() + doReturn(Y_DOWN_TRANSITION).`when`(toolbar).translationY + + assertEquals(0f, engineParentView.translationY) + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = 0, + ).apply { + this.engineView = engineView + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + } + + // 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 bottomClipping = -Y_DOWN_TRANSITION.toInt() + verify(engineView).setVerticalClipping(bottomClipping) + + assertEquals(0f, engineParentView.translationY) + } + + @Test + fun `GIVEN the toolbar is at the bottom && the navbar is enabled WHEN toolbar is being shifted THEN EngineView adjusts bottom clipping && EngineViewParent position doesn't change`() { + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: ToolbarContainerView = mock() + doReturn(Y_DOWN_TRANSITION).`when`(toolbar).translationY + + assertEquals(0f, engineParentView.translationY) + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = 0, + ).apply { + this.engineView = engineView + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + } + + // 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 bottomClipping = -Y_DOWN_TRANSITION.toInt() + verify(engineView).setVerticalClipping(bottomClipping) + + assertEquals(0f, engineParentView.translationY) + } + + // Top toolbar position tests + @Test + fun `GIVEN the toolbar is at the top WHEN toolbar is being shifted THEN EngineView adjusts bottom clipping && EngineViewParent shifts as well`() { + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: BrowserToolbar = mock() + doReturn(Y_UP_TRANSITION).`when`(toolbar).translationY + + assertEquals(0f, engineParentView.translationY) + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = TOOLBAR_HEIGHT.toInt(), + ).apply { + this.engineView = engineView + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + } + + verify(engineView).setVerticalClipping(Y_UP_TRANSITION.toInt()) + + // 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. + val bottomClipping = Y_UP_TRANSITION + TOOLBAR_HEIGHT + assertEquals(bottomClipping, engineParentView.translationY) + } + + // Combined toolbar position tests + @Test + fun `WHEN both of the toolbars are being shifted GIVEN the toolbar is at the top && the navbar is enabled THEN EngineView adjusts bottom clipping`() { + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: BrowserToolbar = mock() + val toolbarContainerView: ToolbarContainerView = mock() + doReturn(Y_UP_TRANSITION).`when`(toolbar).translationY + doReturn(Y_DOWN_TRANSITION).`when`(toolbarContainerView).translationY + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = TOOLBAR_HEIGHT.toInt(), + ).apply { + this.engineView = engineView + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + onDependentViewChanged(mock(), mock(), toolbarContainerView) + } + + val doubleClipping = Y_UP_TRANSITION - Y_DOWN_TRANSITION + verify(engineView).setVerticalClipping(doubleClipping.toInt()) + } + + @Test + fun `WHEN both of the toolbars are being shifted GIVEN the toolbar is at the top && the navbar is enabled THEN EngineViewParent shifts as well`() { + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: BrowserToolbar = mock() + val toolbarContainerView: ToolbarContainerView = mock() + doReturn(Y_UP_TRANSITION).`when`(toolbar).translationY + doReturn(Y_DOWN_TRANSITION).`when`(toolbarContainerView).translationY + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = TOOLBAR_HEIGHT.toInt(), + ).apply { + this.engineView = engineView + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + onDependentViewChanged(mock(), mock(), toolbarContainerView) + } + + // The top of the parent should be positioned right below the toolbar, + // so when we are given the new Y position of the top of the toolbar, + // which is always negative as the element is being "scrolled" out of + // the screen, the bottom of the toolbar is just a toolbar height away + // from it. + val parentTranslation = Y_UP_TRANSITION + TOOLBAR_HEIGHT + assertEquals(parentTranslation, engineParentView.translationY) + } + + // Edge cases + @Test + fun `GIVEN top toolbar is much bigger than bottom WHEN bottom stopped shifting && top is shifting THEN bottom clipping && engineParentView shifting is still accurate`() { + val largeYUpTransition = -500f + val largeTopToolbarHeight = 500 + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: BrowserToolbar = mock() + doReturn(largeYUpTransition).`when`(toolbar).translationY + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = largeTopToolbarHeight, + ).apply { + this.engineView = engineView + this.recentBottomToolbarTranslation = Y_DOWN_TRANSITION + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + } + + val doubleClipping = largeYUpTransition - Y_DOWN_TRANSITION + verify(engineView).setVerticalClipping(doubleClipping.toInt()) + + val parentTranslation = largeYUpTransition + largeTopToolbarHeight + assertEquals(parentTranslation, engineParentView.translationY) + } + + @Test + fun `GIVEN bottom toolbar is much bigger than top WHEN top stopped shifting && bottom is shifting THEN bottom clipping && engineParentView shifting is still accurate`() { + val largeYBottomTransition = 500f + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbarContainerView: ToolbarContainerView = mock() + doReturn(largeYBottomTransition).`when`(toolbarContainerView).translationY + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = TOOLBAR_HEIGHT.toInt(), + ).apply { + this.engineView = engineView + this.recentTopToolbarTranslation = Y_UP_TRANSITION + }.run { + onDependentViewChanged(mock(), mock(), toolbarContainerView) + } + + val doubleClipping = Y_UP_TRANSITION - largeYBottomTransition + verify(engineView).setVerticalClipping(doubleClipping.toInt()) + + val parentTranslation = Y_UP_TRANSITION + TOOLBAR_HEIGHT + assertEquals(parentTranslation, engineParentView.translationY) + } + + @Test + fun `GIVEN a bottom toolbar WHEN translation returns NaN THEN no exception thrown`() { + val engineView: EngineView = spy(FakeEngineView(testContext)) + val engineParentView: View = spy(View(testContext)) + val toolbar: View = mock() + doReturn(100).`when`(toolbar).height + doReturn(Float.NaN).`when`(toolbar).translationY + + EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = engineParentView, + topToolbarHeight = 0, + ).apply { + this.engineView = engineView + }.run { + onDependentViewChanged(mock(), mock(), toolbar) + } + + assertEquals(0f, engineView.asView().translationY) + } + + // General tests + @Test + fun `WHEN layoutDependsOn receives a class that isn't a ScrollableToolbar THEN it ignores it`() { + val behavior = EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = mock(), + topToolbarHeight = 0, + ) + + assertFalse(behavior.layoutDependsOn(mock(), mock(), TextView(testContext))) + assertFalse(behavior.layoutDependsOn(mock(), mock(), EditText(testContext))) + assertFalse(behavior.layoutDependsOn(mock(), mock(), ImageView(testContext))) + } + + @Test + fun `WHEN layoutDependsOn receives a class that is a ScrollableToolbar THEN it recognizes it as a dependency`() { + val behavior = EngineViewClippingBehavior( + context = mock(), + attrs = null, + engineViewParent = mock(), + topToolbarHeight = 0, + ) + + assertTrue(behavior.layoutDependsOn(mock(), mock(), BrowserToolbar(testContext))) + assertTrue(behavior.layoutDependsOn(mock(), mock(), ToolbarContainerView(testContext))) + } +} + +private const val TOOLBAR_HEIGHT = 100f +private const val Y_UP_TRANSITION = -42f +private const val Y_DOWN_TRANSITION = -42f