Fix omnibar scrolling and browser layout gap (#5131)

Task/Issue URL: https://app.asana.com/0/488551667048375/1208503210252671/f Also: https://app.asana.com/0/488551667048375/1208503210253722/f. ### Description This PR fixes 2 connected issues: - A gap at the bottom of a screen is visible when scroll flags are set on a toolbar container to disable toolbar scrolling - Bottom omnibar not visible or is incorrectly scrollable while an omnibar icon is highlighted The solution unifies how the scrolling is enabled/disabled for both the top and bottom bar. The scroll flags are not set anymore, which fixes the gap problem. ### Steps to test this PR _Bottom bar scrolling with highlighted icons_ - [x] Clear the app storage and run the app - [x] Set the omnibar position to bottom - [x] Go through the onboarding steps until you get to the blocked trackers - [x] Notice that when the shield is highlighted, the omnibar is expanded and not scrollable - [x] After completing the onboarding, notice that the omnibar is scrollable again _Extra gap at the bottom_ - [x] Clear the app storage and run the app - [x] Go through the onboarding steps until you get to the blocked trackers - [x] Notice there is no gap at the bottom ### UI changes _Bottom bar scrolling with highlighted icons_ https://github.com/user-attachments/assets/17405aff-34ca-4f6f-a705-94de45bc207d _Extra gap at the bottom_ https://github.com/user-attachments/assets/d44ccdcf-5c4c-49c7-8f01-a4ebc60f2088 --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1208503210252671
duckduckgo · Oct 14, 2024 · 54d2d1a · 54d2d1a
1 parent 2bacfb6
commit 54d2d1a
Show file tree

Hide file tree

Showing 6 changed files with 38 additions and 58 deletions.
diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
@@ -76,7 +76,6 @@ import androidx.core.net.toUri
 import androidx.core.text.HtmlCompat
 import androidx.core.text.HtmlCompat.FROM_HTML_MODE_LEGACY
 import androidx.core.text.toSpannable
-import androidx.core.view.isVisible
 import androidx.core.view.postDelayed
 import androidx.fragment.app.DialogFragment
 import androidx.fragment.app.Fragment
@@ -141,7 +140,6 @@ import com.duckduckgo.app.browser.newtab.NewTabPageProvider
 import com.duckduckgo.app.browser.omnibar.LegacyOmnibarView
 import com.duckduckgo.app.browser.omnibar.LegacyOmnibarView.ItemPressedListener
 import com.duckduckgo.app.browser.omnibar.Omnibar
-import com.duckduckgo.app.browser.omnibar.OmnibarScrolling
 import com.duckduckgo.app.browser.omnibar.animations.TrackersAnimatorListener
 import com.duckduckgo.app.browser.print.PrintDocumentAdapterFactory
 import com.duckduckgo.app.browser.print.PrintInjector
@@ -369,9 +367,6 @@ class BrowserTabFragment :
     @Inject
     lateinit var ctaViewModel: CtaViewModel
 
-    @Inject
-    lateinit var omnibarScrolling: OmnibarScrolling
-
     @Inject
     lateinit var previewGenerator: WebViewPreviewGenerator
 
@@ -3974,7 +3969,7 @@ class BrowserTabFragment :
                 .launchIn(lifecycleScope)
             newBrowserTab.newTabContainerLayout.show()
             newBrowserTab.newTabLayout.show()
-            omnibarScrolling.disableOmnibarScrolling(omnibar.toolbarContainer)
+            omnibar.isScrollingEnabled = false
             viewModel.onNewTabShown()
         }
 

diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/BottomAppBarBehavior.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/BottomAppBarBehavior.kt
@@ -40,7 +40,7 @@ import kotlin.math.roundToInt
  */
 class BottomAppBarBehavior<V : View>(
     context: Context,
-    private val toolbar: LegacyOmnibarView,
+    private val omnibar: LegacyOmnibarView,
     attrs: AttributeSet? = null,
 ) : CoordinatorLayout.Behavior<V>(context, attrs) {
     @NestedScrollType
@@ -57,9 +57,10 @@ class BottomAppBarBehavior<V : View>(
 
         if (dependency.id == R.id.browserLayout) {
             browserLayout = dependency as RelativeLayout
-            offsetBottomByToolbar(browserLayout)
         }
 
+        offsetBottomByToolbar(dependency)
+
         return super.layoutDependsOn(parent, child, dependency)
     }
 
@@ -89,21 +90,22 @@ class BottomAppBarBehavior<V : View>(
         consumed: IntArray,
         type: Int,
     ) {
-        super.onNestedPreScroll(coordinatorLayout, toolbar, target, dx, dy, consumed, type)
+        if (omnibar.isScrollingEnabled) {
+            super.onNestedPreScroll(coordinatorLayout, toolbar, target, dx, dy, consumed, type)
 
-        // only hide the app bar in the browser layout
-        if (target.id == R.id.browserWebView) {
-            toolbar.translationY = max(0f, min(toolbar.height.toFloat(), toolbar.translationY + dy))
-            offsetBottomByToolbar(browserLayout)
-        }
+            // only hide the app bar in the browser layout
+            if (target.id == R.id.browserWebView) {
+                toolbar.translationY = max(0f, min(toolbar.height.toFloat(), toolbar.translationY + dy))
+            }
 
-        offsetBottomByToolbar(target)
+            offsetBottomByToolbar(target)
+        }
     }
 
     private fun offsetBottomByToolbar(view: View?) {
         if (view?.layoutParams is CoordinatorLayout.LayoutParams) {
             view.updateLayoutParams<CoordinatorLayout.LayoutParams> {
-                this.bottomMargin = toolbar.measuredHeight - toolbar.translationY.roundToInt()
+                this.bottomMargin = omnibar.measuredHeight - omnibar.translationY.roundToInt()
             }
             view.requestLayout()
         }
@@ -135,12 +137,12 @@ class BottomAppBarBehavior<V : View>(
 
         offsetAnimator?.addUpdateListener { animation ->
             val animatedValue = animation.animatedValue as Float
-            toolbar.translationY = animatedValue
+            omnibar.translationY = animatedValue
             offsetBottomByToolbar(browserLayout)
         }
 
-        val targetTranslation = if (isVisible) 0f else toolbar.height.toFloat()
-        offsetAnimator?.setFloatValues(toolbar.translationY, targetTranslation)
+        val targetTranslation = if (isVisible) 0f else omnibar.height.toFloat()
+        offsetAnimator?.setFloatValues(omnibar.translationY, targetTranslation)
         offsetAnimator?.start()
     }
 

diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/LegacyOmnibarView.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/LegacyOmnibarView.kt
@@ -27,7 +27,6 @@ import android.widget.ImageView
 import android.widget.ProgressBar
 import androidx.appcompat.widget.Toolbar
 import androidx.coordinatorlayout.widget.CoordinatorLayout
-import androidx.coordinatorlayout.widget.CoordinatorLayout.Behavior
 import androidx.core.view.doOnLayout
 import androidx.core.view.isInvisible
 import androidx.core.view.isVisible
@@ -99,9 +98,6 @@ class LegacyOmnibarView @JvmOverloads constructor(
 
     private val omnibarPosition: OmnibarPosition
 
-    @Inject
-    lateinit var omnibarScrolling: OmnibarScrolling
-
     @Inject
     lateinit var privacyShieldView: PrivacyShieldAnimationHelper
 
@@ -140,6 +136,14 @@ class LegacyOmnibarView @JvmOverloads constructor(
 
     private var privacyShield: PrivacyShield? = null
 
+    var isScrollingEnabled: Boolean = true
+        set(value) {
+            field = value
+            if (!value) {
+                setExpanded(expanded = true, animate = true)
+            }
+        }
+
     init {
         val attr = context.theme.obtainStyledAttributes(attrs, R.styleable.LegacyOmnibarView, defStyle, 0)
         omnibarPosition = OmnibarPosition.entries[attr.getInt(R.styleable.LegacyOmnibarView_omnibarPosition, 0)]
@@ -256,16 +260,6 @@ class LegacyOmnibarView @JvmOverloads constructor(
         }
     }
 
-    fun setScrollingEnabled(enabled: Boolean) {
-        safeCall {
-            if (enabled) {
-                omnibarScrolling.enableOmnibarScrolling(toolbarContainer)
-            } else {
-                omnibarScrolling.disableOmnibarScrolling(toolbarContainer)
-            }
-        }
-    }
-
     private fun renderPulseAnimation(viewState: BrowserViewState) {
         val targetView = if (viewState.showMenuButton.isHighlighted()) {
             browserMenuImageView
@@ -279,15 +273,15 @@ class LegacyOmnibarView @JvmOverloads constructor(
 
         // omnibar only scrollable when browser showing and the fire button is not promoted
         if (targetView != null) {
-            setScrollingEnabled(false)
+            isScrollingEnabled = false
             doOnLayout {
                 if (this::pulseAnimation.isInitialized) {
                     pulseAnimation.playOn(targetView)
                 }
             }
         } else {
             if (viewState.browserShowing) {
-                setScrollingEnabled(true)
+                isScrollingEnabled = true
             }
             if (this::pulseAnimation.isInitialized) {
                 pulseAnimation.stop()

diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt
@@ -19,7 +19,6 @@ package com.duckduckgo.app.browser.omnibar
 import android.animation.Animator
 import android.annotation.SuppressLint
 import android.content.Context
-import android.content.res.TypedArray
 import android.graphics.Color
 import android.graphics.drawable.ColorDrawable
 import android.text.Editable
@@ -61,7 +60,6 @@ import com.duckduckgo.common.utils.extensions.isDifferent
 import com.duckduckgo.common.utils.extensions.replaceTextChangedListener
 import com.duckduckgo.common.utils.extractDomain
 import com.duckduckgo.common.utils.text.TextChangedWatcher
-import com.duckduckgo.mobile.android.R as CommonR
 import com.google.android.material.appbar.AppBarLayout.GONE
 import com.google.android.material.appbar.AppBarLayout.VISIBLE
 
@@ -77,13 +75,6 @@ class Omnibar(
         data object NewTab : ViewMode()
     }
 
-    private val actionBarSize: Int by lazy {
-        val array: TypedArray = binding.rootView.context.theme.obtainStyledAttributes(intArrayOf(android.R.attr.actionBarSize))
-        val actionBarSize = array.getDimensionPixelSize(0, -1)
-        array.recycle()
-        actionBarSize
-    }
-
     val legacyOmnibar: LegacyOmnibarView by lazy {
         when (omnibarPosition) {
             OmnibarPosition.TOP -> {
@@ -99,11 +90,6 @@ class Omnibar(
                 removeAppBarBehavior(binding.browserLayout)
                 removeAppBarBehavior(binding.focusedView)
 
-                // add padding to the NTP to prevent the bottom toolbar from overlapping the settings button
-                binding.includeNewBrowserTab.browserBackground.apply {
-                    setPadding(paddingLeft, context.resources.getDimensionPixelSize(CommonR.dimen.keyline_2), paddingRight, actionBarSize)
-                }
-
                 // prevent the touch event leaking to the webView below
                 binding.legacyOmnibarBottom.setOnTouchListener { _, _ -> true }
 
@@ -138,14 +124,20 @@ class Omnibar(
     val spacer = legacyOmnibar.spacer
     val textInputRootView = legacyOmnibar.omnibarTextInput.rootView
 
+    var isScrollingEnabled: Boolean
+        get() = legacyOmnibar.isScrollingEnabled
+        set(value) {
+            legacyOmnibar.isScrollingEnabled = value
+        }
+
     fun setViewMode(viewMode: ViewMode) {
         when (viewMode) {
             Error -> {
                 setExpanded(true)
                 shieldIcon.isInvisible = true
             }
             NewTab -> {
-                setScrollingEnabled(false)
+                isScrollingEnabled = false
                 setExpanded(true)
             }
             SSLWarning -> {
@@ -406,10 +398,6 @@ class Omnibar(
         legacyOmnibar.onNewProgress(newProgress, onAnimationEnd)
     }
 
-    fun setScrollingEnabled(enabled: Boolean) {
-        legacyOmnibar.setScrollingEnabled(enabled)
-    }
-
     fun configureCustomTab(
         context: Context,
         customTabToolbarColor: Int,

diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/TopAppBarBehavior.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/TopAppBarBehavior.kt
@@ -29,7 +29,7 @@ import com.google.android.material.appbar.AppBarLayout
  */
 class TopAppBarBehavior(
     context: Context,
-    private val toolbar: LegacyOmnibarView,
+    private val omnibar: LegacyOmnibarView,
     attrs: AttributeSet? = null,
 ) : AppBarLayout.Behavior(context, attrs) {
     override fun onNestedPreScroll(
@@ -41,7 +41,7 @@ class TopAppBarBehavior(
         consumed: IntArray,
         type: Int,
     ) {
-        if (target.id == R.id.browserWebView) {
+        if (target.id == R.id.browserWebView && omnibar.isScrollingEnabled) {
             super.onNestedPreScroll(coordinatorLayout, child, target, dx, dy, consumed, type)
         } else {
             offsetBottomByToolbar(target)
@@ -51,7 +51,7 @@ class TopAppBarBehavior(
     private fun offsetBottomByToolbar(view: View?) {
         if (view?.layoutParams is CoordinatorLayout.LayoutParams) {
             view.updateLayoutParams<CoordinatorLayout.LayoutParams> {
-                this.bottomMargin = toolbar.measuredHeight
+                this.bottomMargin = omnibar.measuredHeight
             }
             view.requestLayout()
         }

diff --git a/app/src/main/res/layout/view_legacy_omnibar.xml b/app/src/main/res/layout/view_legacy_omnibar.xml
@@ -28,7 +28,8 @@
         android:id="@+id/toolbarContainer"
         android:layout_width="match_parent"
         android:layout_height="match_parent"
-        android:background="?daxColorSurface">
+        android:background="?daxColorSurface"
+        app:layout_scrollFlags="scroll|enterAlways|snap">
 
         <androidx.appcompat.widget.Toolbar
             android:id="@+id/toolbar"