From 0a0f75d2ab89412df38da0d1e91a2174a26e7e90 Mon Sep 17 00:00:00 2001 From: Mugurell Date: Fri, 19 Feb 2021 17:45:56 +0200 Subject: [PATCH] For #18027 - Also fix the bottom toolbar in place when a11y is enabled We previously only set the top toolbar as fixed if an a11y service was running. --- .../fenix/browser/BaseBrowserFragment.kt | 7 +--- .../components/toolbar/BrowserToolbarView.kt | 2 +- .../fenix/browser/BaseBrowserFragmentTest.kt | 20 ++++++++++ .../toolbar/BrowserToolbarViewTest.kt | 40 ++++++++++++++++++- 4 files changed, 60 insertions(+), 9 deletions(-) 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 3718b2dfe3..ecc49681e8 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -822,12 +822,7 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler, Activit internal fun initializeEngineView(toolbarHeight: Int) { val context = requireContext() - // If there is an a11y service enabled and the user hasn't explicitly set bottom toolbar - val isTopToolbarForced = - !context.settings().shouldUseBottomToolbar && - context.settings().shouldUseFixedTopToolbar - - if (!isTopToolbarForced && context.settings().isDynamicToolbarEnabled) { + if (!context.settings().shouldUseFixedTopToolbar && context.settings().isDynamicToolbarEnabled) { getEngineView().setDynamicToolbarMaxHeight(toolbarHeight) val toolbarPosition = if (context.settings().shouldUseBottomToolbar) { diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt index 1b25af105f..b3b97311d8 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt @@ -245,7 +245,7 @@ class BrowserToolbarView( fun setToolbarBehavior(shouldDisableScroll: Boolean = false) { when (settings.toolbarPosition) { ToolbarPosition.BOTTOM -> { - if (settings.isDynamicToolbarEnabled && !isPwaTabOrTwaTab) { + if (settings.isDynamicToolbarEnabled && !isPwaTabOrTwaTab && !settings.shouldUseFixedTopToolbar) { setDynamicToolbarBehavior(MozacToolbarPosition.BOTTOM) } else { expandToolbarAndMakeItFixed() 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 d65605fbd2..c0d486e175 100644 --- a/app/src/test/java/org/mozilla/fenix/browser/BaseBrowserFragmentTest.kt +++ b/app/src/test/java/org/mozilla/fenix/browser/BaseBrowserFragmentTest.kt @@ -60,6 +60,16 @@ class BaseBrowserFragmentTest { verify { engineView.setDynamicToolbarMaxHeight(0) } } + @Test + fun `initializeEngineView should setDynamicToolbarMaxHeight to 0 if bottom toolbar is forced for a11y`() { + every { testContext.settings().shouldUseBottomToolbar } returns true + every { testContext.settings().shouldUseFixedTopToolbar } returns true + + fragment.initializeEngineView(13) + + verify { engineView.setDynamicToolbarMaxHeight(0) } + } + @Test fun `initializeEngineView should setDynamicToolbarMaxHeight to toolbar height if dynamic toolbar is enabled`() { every { testContext.settings().shouldUseFixedTopToolbar } returns false @@ -116,6 +126,16 @@ class BaseBrowserFragmentTest { verify { (swipeRefreshLayout.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = 13 } } + @Test + fun `initializeEngineView should set toolbar height as EngineView parent's bottom margin if bottom toolbar is forced for a11y`() { + every { testContext.settings().shouldUseBottomToolbar } returns true + every { testContext.settings().shouldUseFixedTopToolbar } returns true + + fragment.initializeEngineView(13) + + verify { (swipeRefreshLayout.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = 13 } + } + @Test fun `WHEN status is equals to FAILED or COMPLETED and it is the same tab then shouldShowCompletedDownloadDialog will be true`() { every { fragment.getCurrentTab() } returns createTab(id = "1", url = "") diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarViewTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarViewTest.kt index df34475e59..727f4f6a86 100644 --- a/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarViewTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/BrowserToolbarViewTest.kt @@ -50,11 +50,12 @@ class BrowserToolbarViewTest { } @Test - fun `setToolbarBehavior(false) should setDynamicToolbarBehavior if bottom toolbar is dynamic and the tab is not for a PWA or TWA`() { + fun `setToolbarBehavior(false) should setDynamicToolbarBehavior if no a11y, bottom toolbar is dynamic and the tab is not for a PWA or TWA`() { val toolbarViewSpy = spyk(toolbarView) every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM every { testContext.settings().isDynamicToolbarEnabled } returns true every { toolbarViewSpy.isPwaTabOrTwaTab } returns false + every { testContext.settings().shouldUseFixedTopToolbar } returns false toolbarViewSpy.setToolbarBehavior(false) @@ -66,6 +67,8 @@ class BrowserToolbarViewTest { val toolbarViewSpy = spyk(toolbarView) every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM every { testContext.settings().isDynamicToolbarEnabled } returns false + every { toolbarViewSpy.isPwaTabOrTwaTab } returns false + every { testContext.settings().shouldUseFixedTopToolbar } returns false toolbarViewSpy.setToolbarBehavior(false) @@ -78,6 +81,20 @@ class BrowserToolbarViewTest { every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM every { testContext.settings().isDynamicToolbarEnabled } returns true every { toolbarViewSpy.isPwaTabOrTwaTab } returns true + every { testContext.settings().shouldUseFixedTopToolbar } returns false + + toolbarViewSpy.setToolbarBehavior(false) + + verify { toolbarViewSpy.expandToolbarAndMakeItFixed() } + } + + @Test + fun `setToolbarBehavior(false) should expandToolbarAndMakeItFixed if bottom toolbar is dynamic tab is not for a PWA or TWA but a11y is enabled`() { + val toolbarViewSpy = spyk(toolbarView) + every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM + every { testContext.settings().isDynamicToolbarEnabled } returns true + every { toolbarViewSpy.isPwaTabOrTwaTab } returns false + every { testContext.settings().shouldUseFixedTopToolbar } returns true toolbarViewSpy.setToolbarBehavior(false) @@ -85,11 +102,14 @@ class BrowserToolbarViewTest { } @Test - fun `setToolbarBehavior(true) should setDynamicToolbarBehavior if bottom toolbar is dynamic and the tab is not for a PWA or TWA`() { + fun `setToolbarBehavior(true) should expandToolbarAndMakeItFixed bottom toolbar is dynamic, the tab is not for a PWA or TWA and a11y is disabled`() { + // All intrinsic checks are met but the method was called with `shouldDisableScroll` = true + val toolbarViewSpy = spyk(toolbarView) every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM every { testContext.settings().isDynamicToolbarEnabled } returns true every { toolbarViewSpy.isPwaTabOrTwaTab } returns false + every { testContext.settings().shouldUseFixedTopToolbar } returns false toolbarViewSpy.setToolbarBehavior(false) @@ -101,6 +121,8 @@ class BrowserToolbarViewTest { val toolbarViewSpy = spyk(toolbarView) every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM every { testContext.settings().isDynamicToolbarEnabled } returns false + every { toolbarViewSpy.isPwaTabOrTwaTab } returns false + every { testContext.settings().shouldUseFixedTopToolbar } returns false toolbarViewSpy.setToolbarBehavior(false) @@ -113,6 +135,20 @@ class BrowserToolbarViewTest { every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM every { testContext.settings().isDynamicToolbarEnabled } returns true every { toolbarViewSpy.isPwaTabOrTwaTab } returns true + every { testContext.settings().shouldUseFixedTopToolbar } returns false + + toolbarViewSpy.setToolbarBehavior(false) + + verify { toolbarViewSpy.expandToolbarAndMakeItFixed() } + } + + @Test + fun `setToolbarBehavior(true) should expandToolbarAndMakeItFixed if bottom toolbar is dynamic, the tab is for a PWA or TWA and a11 is enabled`() { + val toolbarViewSpy = spyk(toolbarView) + every { testContext.settings().toolbarPosition } returns ToolbarPosition.BOTTOM + every { testContext.settings().isDynamicToolbarEnabled } returns true + every { toolbarViewSpy.isPwaTabOrTwaTab } returns false + every { testContext.settings().shouldUseFixedTopToolbar } returns true toolbarViewSpy.setToolbarBehavior(false)