Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Commit

Permalink
Bug 1879370 - Add a ClippingBehavior supporting multiple toolbars
Browse files Browse the repository at this point in the history
  • Loading branch information
mike a committed Mar 13, 2024
1 parent 5576119 commit d0841b2
Show file tree
Hide file tree
Showing 14 changed files with 573 additions and 45 deletions.
1 change: 1 addition & 0 deletions fenix/.buildconfig.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ projects:
- support-rusthttp
- support-rustlog
- support-test
- support-test-fakes
- support-test-libstate
- support-utils
- support-webextensions
Expand Down
1 change: 1 addition & 0 deletions fenix/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ 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.EngineView
import mozilla.components.concept.engine.permission.SitePermissions
import mozilla.components.concept.engine.prompt.ShareData
import mozilla.components.concept.storage.LoginEntry
Expand Down Expand Up @@ -115,7 +117,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
Expand Down Expand Up @@ -146,7 +147,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
Expand Down Expand Up @@ -183,7 +186,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].
Expand Down Expand Up @@ -365,8 +369,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),
Expand Down Expand Up @@ -478,9 +480,12 @@ abstract class BaseBrowserFragment :
)
}

val shouldHideOnScroll =
!context.settings().shouldUseFixedTopToolbar && context.settings().isDynamicToolbarEnabled
_bottomToolbarContainerView = BottomToolbarContainerView(
context = context,
parent = binding.browserLayout,
hideOnScroll = shouldHideOnScroll,
composableContent = {
FirefoxTheme {
Column {
Expand Down Expand Up @@ -717,6 +722,8 @@ abstract class BaseBrowserFragment :
},
)

val bottomToolbarHeight = context.settings().getBottomToolbarHeight()

downloadFeature.onDownloadStopped = { downloadState, _, downloadJobStatus ->
handleOnDownloadFinished(downloadState, downloadJobStatus, downloadFeature::tryAgain)
}
Expand All @@ -725,7 +732,7 @@ abstract class BaseBrowserFragment :
getCurrentTab()?.id,
store,
context,
toolbarHeight,
bottomToolbarHeight,
)

shareDownloadsFeature.set(
Expand Down Expand Up @@ -1058,7 +1065,11 @@ abstract class BaseBrowserFragment :
owner = this,
view = view,
)
initializeEngineView(toolbarHeight)

initializeEngineView(
topToolbarHeight = context.settings().getTopToolbarHeight(),
bottomToolbarHeight = bottomToolbarHeight,
)
}

protected fun showUndoSnackbar(message: String) {
Expand Down Expand Up @@ -1194,7 +1205,7 @@ abstract class BaseBrowserFragment :
sessionId: String?,
store: BrowserStore,
context: Context,
toolbarHeight: Int,
bottomToolbarHeight: Int,
) {
val savedDownloadState =
sharedViewModel.downloadDialogState[sessionId]
Expand Down Expand Up @@ -1227,7 +1238,7 @@ abstract class BaseBrowserFragment :
showCannotOpenFileError(binding.dynamicSnackbarContainer, context, it)
},
binding = binding.viewDynamicDownloadDialog,
toolbarHeight = toolbarHeight,
bottomToolbarHeight = bottomToolbarHeight,
onDismiss = onDismiss,
).show()

Expand All @@ -1241,37 +1252,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
}
}

Expand Down Expand Up @@ -1349,9 +1381,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 {
Expand Down Expand Up @@ -1674,8 +1706,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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.mozilla.fenix.components.toolbar

import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.utils.Settings

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
) {

Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}
}
Loading

0 comments on commit d0841b2

Please sign in to comment.