Omnibar position option #4885

0nko · 2024-08-09T16:09:44Z

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208356719296150/f

Description

This is a feature branch for the omnibar position project.

Testing instructions

Dummy toolbar

Start the omnibar at the top
Use the Fire button and notice the dummy toolbar is briefly shown at the top
Change the omnibar to the bottom
Use the Fire button and notice the dummy toolbar is briefly shown at the bottom

Autocomplete

Change the omnibar position to the bottom
Start typing in the address bar
Notice the suggestions are correctly displayed above the toolbar

UI changes

Before	After

Snackbar

Change the omnibar position to the bottom
Trigger a condition to show a snackbar (see the diff below)
Notice the snackbar is displayed above the omnibar

This patch will triggers a snackbar after tapping the fire button:

Index: app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
--- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt	(revision Staged)
+++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt	(date 1725012737445)
@@ -3299,6 +3299,7 @@
     }
 
     fun onFireMenuSelected() {
+        showErrorWithAction()
         val cta = currentCtaViewState().cta
         if (cta is OnboardingDaxDialogCta.DaxFireButtonCta) {
             onUserDismissedCta(cta)

UI changes

Before	After

Privacy pop-up dialog

Go to settings and change the omnibar position to BOTTOM
Go back to the tab
Trigger the privacy popup condition by refreshing the page
Notice the dialog correctly appears at the bottom

UI changes

Before	After

Onboarding messages

Clear the app storage and trigger onboarding
Change the omnibar position to the bottom
Go to “google.com"
Notice that during onboarding steps, whenever there is the “hand” icon, it points to the bottom

UI changes

Before	After

0nko · 2024-08-09T16:09:58Z

Omnibar position option #4885 : 4 dependent PRs (#4886 , #4940 , #4941 and 1 other) 👈
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @0nko and the rest of your teammates on Graphite

malmstein

There are a couple of outstanding comments from previous PR.
The big issue is that when the omnibar is at the bottom we are over the web content, and not properly pushing it to the top. That makes it so any content at the bottom of the viewport is behind the omnibar.

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

malmstein · 2024-09-10T08:58:05Z

browser-api/src/main/java/com/duckduckgo/app/browser/omnibar/model/OmnibarPosition.kt

+ * limitations under the License.
+ */
+
+package com.duckduckgo.app.browser.omnibar.model


why is this in the browser-api module?

Based on what I observed, it seemed like the right place to put a common model that might be accessed by different modules in the future. If you feel it’s not the right place I’m open to suggestions.

If another module would need to access the omnibar position, they will do it via the SettingsDataStore. You have placed the model in the browser-api module, but the public accessor to that value is in the app module.

This module is specifically for things related to the browser. The omnibar and it’s position are separate from that. In other words, omnibar doesn’t need to know anything about the browser. I’d move it to the same app/omnibar package you have the rest of the logic.

0nko · 2024-09-12T10:24:59Z

The big issue is that when the omnibar is at the bottom we are over the web content, and not properly pushing it to the top. That makes it so any content at the bottom of the viewport is behind the omnibar.

@malmstein Thanks for the review. I will fix this in a separate PR. The rest of the comments should be addressed now.

I added steps for testing different scenarios in the PR description.

malmstein · 2024-09-18T21:11:18Z

Omnibar position option #4885 : 4 dependent PRs (#4886 , #4940 , #4941 and 1 other) 👈
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @0nko and the rest of your teammates on Graphite

Task/Issue URL: https://app.asana.com/0/1207418217763355/1206100017017597/f Figma link: https://www.figma.com/design/Z4TFKpzzc02naMpqYO9ehv/Bottom-URL-bar-(Android)?node-id=1-2&t=oeftk6SHYD0A2iwr-1 This PR adds a new Settings option to change the position of the browser address bar. The selected value is saved in the user preferences. - [x] Go to Settings - [x] Tap on Appearance - [x] Notice a new Address Bar option - [x] Tap on it and notice a dialog shows up - [x] Change the section and Save - [x] Notice the new selection is reflected in the setting description - [x] Leave the Settings - [x] Navigate back to Appearance settings - [x] Notice the selection was preserved https://github.com/user-attachments/assets/f2fb8635-6e96-4439-a130-dea91e7b11de --------- Co-authored-by: David González <[email protected]>

Task/Issue URL: https://app.asana.com/0/1207418217763355/1207988999525948/f This PR implements the bottom omnibar layout, including the hide animation on scroll. The position of the omnibar is determined based on the setting stored in the shared preferences. - [x] Go to Settings -> Appearance -> Address bar - [x] Change the position selection and tap Save - [x] Go back to the browser screen - [x] Notice the screen is reloaded with the omnibar repositioned - [x] Try scrolling and notice the omnibar is hidden/shown - [x] Repeat the steps above with the other position option https://github.com/user-attachments/assets/edfad444-e6df-4bee-bcd8-81cfe6d0e4f1 --------- Co-authored-by: David González <[email protected]>

…ppear

Task/Issue URL: https://app.asana.com/0/72649045549333/1208157245002308/f - [ ] Change the omnibar position to the bottom - [ ] Go the tab and tap on the menu button - [ ] Notice the navigation buttons are now at the bottom of the menu | Before | After | | ------ | ----- | ![image](https://github.com/user-attachments/assets/689658df-1efc-4a8d-a698-f51111d275c8)|![image](https://github.com/user-attachments/assets/9a069133-6b4e-47bf-8b4d-99cc1ea2e045)|

…#4933) Task/Issue URL: https://app.asana.com/0/1200581511062568/1208125695673129/f ### Description Added a new section in the New Tab Page and a view in the Legacy Tab Page. ### Steps to test this PR See https://app.asana.com/0/0/1208180779804579/f ### UI changes See https://app.asana.com/0/1200581511062568/1208180779804556/f

Task/Issue URL: https://app.asana.com/0/0/1208157864474373/f ### Description ### Steps to test this PR - [ ] Start the omnibar at the top - [ ] Reload the page and notice the dummy toolbar is shown at the top - [ ] Change the omnibar to the bottom - [ ] Reload the page and notice the dummy toolbar is shown at the bottom

Task/Issue URL: https://app.asana.com/0/0/1208157864474375/f ### Description ### Steps to test this PR - [ ] Clear the app storage and trigger onboarding - [ ] Change the omnibar position to the bottom - [ ] Go to “google.com" - [ ] Notice that during onboarding steps, whenever there is the “hand” icon, it points to the bottom ### UI changes | Before | After | | ------ | ----- | ![image](https://github.com/user-attachments/assets/f4bad558-02d6-48cb-bea9-ab028d88b8b5)|![image](https://github.com/user-attachments/assets/49c4e50d-037a-45cb-922d-ac36b655edb2)| --------- Co-authored-by: David González <[email protected]>

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208166986247266/f ### Description ### Steps to test this PR - [ ] Change the omnibar position to the bottom - [ ] Start typing in the address bar - [ ] Notice the suggestions are correctly displayed above the toolbar ### UI changes | Before | After | | ------ | ----- | ![image](https://github.com/user-attachments/assets/dfae5e47-a6e1-4d4d-bc25-4847ef4a0a32)|![image](https://github.com/user-attachments/assets/7ef7a0f9-cc74-41f5-8f28-d1d3462b740f)|

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208166986247270/f ### Description ### Steps to test this PR - [ ] Change the omnibar position to the bottom - [ ] Trigger a condition to show a snackbar (see the diff below) - [ ] Notice the snackbar is displayed above the omnibar This patch will triggers a snackbar after tapping the fire button: ```kotlin Index: app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt (revision Staged) +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt (date 1725012737445) @@ -3299,6 +3299,7 @@ } fun onFireMenuSelected() { + showErrorWithAction() val cta = currentCtaViewState().cta if (cta is OnboardingDaxDialogCta.DaxFireButtonCta) { onUserDismissedCta(cta) ``` ### UI changes | Before | After | | ------ | ----- | ![image](https://github.com/user-attachments/assets/2f7c3158-48b9-4ffa-bb73-bd86fcba47f7)|![image](https://github.com/user-attachments/assets/76400958-f7d6-4ca7-b91b-9a9308807e74)|

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208184077971807/f ### Description ### Steps to test this PR - [ ] Go to settings and change the omnibar position to BOTTOM - [ ] Go back to the tab - [ ] Trigger the privacy popup condition - [ ] Notice the dialog correctly appears at the bottom ### UI changes | Before | After | | ------ | ----- | ![image](https://github.com/user-attachments/assets/b67b92ab-2b8e-428c-8f10-444c23f613c2)|![image](https://github.com/user-attachments/assets/c409136e-1986-4844-bc68-3ccfe7c4c593)|

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208145303592222/f ### Description This PR implements a new plugin that allows the `m_browser_feature_daily_active_user_d` pixel to collect additional parameters from different parts of the app. This is used to report the omnibar position. ### Steps to test this PR _Omnibar position selection_ - [x] Go to Settings -> Appearance - [x] Tap on the Address bar option - [x] Verify that `ms_address_bar_position_setting_pressed` pixel is fired - [x] Select the Top option - [x] Verify that `ms_address_bar_position_setting_selected_top` pixel is fired - [x] Tap on the address bar again - [x] Select the Bottom option - [x] Verify that `ms_address_bar_position_setting_selected_bottom` pixel is fired _Omnibar position detection_ - [x] Uninstall the app, if already installed - [x] Run the app - [x] Verify that`m_browser_feature_daily_active_user_d` pixel is fired with params: `{default_browser=0, email=0, voice_search=0, address_bar=[[CURRENT OMNIBAR POSITION]], locale=en-US}` with the correct position

Task/Issue URL: https://app.asana.com/0/0/1208248474536286/f ### Description This PR contains the strings for the translation job. --------- Co-authored-by: Dax The Translator <[email protected]>

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208269999932720/f This PR contains multiple design fixes but mainly addresses an issue with a fixes-size websites that were covered by the bottom toolbar. Fixes issues: - Browser menu not shown at the correct position after tapping the menu button with the keyboard open - Incorrect autocomplete suggestion icons - Bottom toolbar covering up the web content of fixed-sized websites - Tapping on a bottom toolbar’s edges leaked clicks to the web view - Onboarding dialog and NTP had extra padding at the top when toolbar at the bootom - There was a zombie shadow of the toolbar https://github.com/user-attachments/assets/c5a7ae9a-f543-433b-9ef7-562f17f3af39 ![Screenshot_20240910_132343](https://github.com/user-attachments/assets/d7c864aa-0912-4ca7-b046-aa14b97a8202) ![Screenshot_20240910_132443](https://github.com/user-attachments/assets/48a78d4a-5860-4e00-a0dc-922aae7d3ef5) ![Screenshot_20240913_003744](https://github.com/user-attachments/assets/3cffb693-7eac-413f-a304-436e6b3795fa) ![Screenshot_20240913_003843](https://github.com/user-attachments/assets/63b1f5a9-fd03-4cbd-a089-5c6dae18ae04) ![Screenshot_20240913_003932](https://github.com/user-attachments/assets/8e6f9172-1833-4899-a544-1ec922926740) ![Screenshot_20240913_004024](https://github.com/user-attachments/assets/054fea28-267b-4d7f-a91b-c4e4df26d021) ![Screenshot_20240913_011751](https://github.com/user-attachments/assets/28a5ed01-91b7-480d-80cc-644d6a4e94f1) --------- Co-authored-by: Dax The Translator <[email protected]> Co-authored-by: David González <[email protected]>

malmstein

There are padding changes to the Omnibar that I was under the impression we weren’t going to add.
The autocomplete icon can’t be seen in dark mode. It’s black, when it should be using the daxColor for icons so dark theme is respected.
You shouldn’t check the site progress in a navigation change, but in progressChanged().
There’s a new Command that forces the Omnibar to be shown on navigation change. This wasn’t here before, why is this needed?

malmstein · 2024-09-19T20:40:56Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -1165,6 +1168,8 @@ class BrowserTabViewModel @Inject constructor(

        canAutofillSelectCredentialsDialogCanAutomaticallyShow = true

+        onLoadProgressChanged(progress = newWebNavigationState.progress ?: 0)


this should be in fun progressChanged() and checking that progress is 100.

malmstein · 2024-09-19T20:41:29Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -1186,6 +1191,9 @@ class BrowserTabViewModel @Inject constructor(
                        }
                    }
                }
+
+                // new page should always show an omnibar
+                command.value = ShowOmnibar


why is this needed?

malmstein · 2024-09-19T20:42:05Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -1218,6 +1226,12 @@ class BrowserTabViewModel @Inject constructor(
        showWebContent()
    }

+    private fun onLoadProgressChanged(progress: Int) {
+        if (progress == 100 && settingsDataStore.omnibarPosition == BOTTOM) {


ditto, this belongs to progressChanged()

malmstein · 2024-09-19T20:45:20Z

app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt

+
+    val appBarLayout: LegacyOmnibarView by lazy {
+        if (omnibarPosition == OmnibarPosition.TOP || !omnibarPositionFeature.self().isEnabled()) {
+            binding.rootView.removeView(binding.legacyOmnibarBottom)


why remove the view instead of making it gone?

malmstein · 2024-09-19T20:48:13Z

app/src/main/res/drawable/ic_autocomplete_down_24dp.xml

+    android:viewportHeight="20">
+  <path
+      android:pathData="M6.06,15h9.19a0.75,0.75 0,0 1,0 1.5h-11a0.75,0.75 0,0 1,-0.75 -0.75v-11a0.75,0.75 0,0 1,1.5 0v9.19L15.22,3.72a0.75,0.75 0,1 1,1.06 1.06L6.06,15"
+      android:fillColor="#000"/>


Suggested change

android:fillColor="#000"/>

android:fillColor="?attr/daxColorPrimaryIcon"/>

this needs to be changed, otherwise it will show black in dark mode.

malmstein · 2024-09-19T20:48:59Z

app/src/main/res/layout/fragment_browser_tab.xml

@@ -124,4 +120,13 @@
            android:focusableInTouchMode="true" />

    </RelativeLayout>
+
+    <com.duckduckgo.app.browser.omnibar.LegacyOmnibarView


this could be gone by default

malmstein · 2024-09-19T20:49:42Z

app/src/main/res/layout/include_omnibar_toolbar_mockup.xml

+        android:id="@+id/tabsMenuMockup"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        android:layout_marginHorizontal="@dimen/keyline_2"


are these changes intentional? I was under the assumption that there wouldn’t be padding changes in this PR

malmstein · 2024-09-19T20:51:27Z

app/src/main/res/layout/view_legacy_omnibar.xml

-                android:layout_marginStart="@dimen/keyline_2"
-                android:layout_marginEnd="@dimen/keyline_2"
-                android:background="?selectableItemBackgroundBorderless"
+                android:layout_gravity="center"


more padding changes. is this supposed to be here?

Holidays

0nko mentioned this pull request Aug 9, 2024

Omnibar position: Settings and store #4886

Merged

9 tasks

0nko force-pushed the feature/ondrej/omnibar-position branch 2 times, most recently from 99ecf5e to adaf626 Compare August 21, 2024 21:49

0nko mentioned this pull request Aug 22, 2024

Omnibar position: Bottom app bar layout #4930

Merged

6 tasks

0nko force-pushed the feature/ondrej/omnibar-position branch from adaf626 to e6f425a Compare August 23, 2024 10:21

0nko requested a review from malmstein August 23, 2024 10:37

This was referenced Aug 27, 2024

Omnibar position: Progress bar #4939

Merged

Omnibar position: Menu #4940

Merged

Omnibar position: Dummy toolbar #4941

Merged

Omnibar position: Onboarding messages #4942

Merged

0nko force-pushed the feature/ondrej/omnibar-position branch from efd808b to dcd1d5e Compare August 28, 2024 10:44

This was referenced Aug 28, 2024

Omnibar position: Autocomplete #4945

Merged

Omnibar position: Snackbar #4954

Merged

0nko force-pushed the feature/ondrej/omnibar-position branch from ed0ac91 to b906747 Compare August 29, 2024 15:12

0nko mentioned this pull request Aug 30, 2024

Omnibar position: Privacy popup dialog #4962

Merged

4 tasks

0nko force-pushed the feature/ondrej/omnibar-position branch 2 times, most recently from 1ab54e1 to 2b7eaca Compare September 2, 2024 12:37

0nko mentioned this pull request Sep 2, 2024

Omnibar position: Pixels #4974

Merged

11 tasks

0nko marked this pull request as ready for review September 9, 2024 16:00

0nko force-pushed the feature/ondrej/omnibar-position branch from 40ecd9e to b0dd837 Compare September 9, 2024 16:55

0nko mentioned this pull request Sep 9, 2024

Omnibar position: Translations #4993

Merged

malmstein requested changes Sep 10, 2024

View reviewed changes

0nko force-pushed the feature/ondrej/omnibar-position branch from 7bd0bd1 to cd4a537 Compare September 12, 2024 09:46

0nko requested a review from malmstein September 12, 2024 10:26

0nko mentioned this pull request Sep 12, 2024

Omnibar position: UI fixes #5003

Merged

0nko force-pushed the feature/ondrej/omnibar-position branch from 1672676 to 37114ff Compare September 17, 2024 14:33

0nko force-pushed the feature/ondrej/omnibar-position branch from 37114ff to 3864a65 Compare September 19, 2024 09:00

0nko and others added 25 commits September 19, 2024 16:31

Add a delay to showing the menu so that the keyboard has time to disa…

ce75378

…ppear

Restacking

9d3f6e9

Fix ktlint issues

605afad

Remove duplicate strings

b2c4d54

Fix the build after rebasing

7a125fe

Revert the warning fix

fe5e5a4

Replace a number literal with a constant

7e4da7b

Replace a delay number literal with a constant

c0094eb

Rename the popup menu method

1c383d4

Use a separate delay for the popup menu

3fd5cab

Move the OmnibarPosition to the app module

8c65bc9

Omnibar position: Translations (#4993)

dc3722b

Task/Issue URL: https://app.asana.com/0/0/1208248474536286/f ### Description This PR contains the strings for the translation job. --------- Co-authored-by: Dax The Translator <[email protected]>

Fix the tests

691741d

Fix the formatting

d47f7d5

0nko force-pushed the feature/ondrej/omnibar-position branch from 5dc3247 to d47f7d5 Compare September 19, 2024 14:36

malmstein self-assigned this Sep 19, 2024

malmstein reviewed Sep 19, 2024

View reviewed changes

malmstein previously approved these changes Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omnibar position option #4885

Omnibar position option #4885

0nko commented Aug 9, 2024 •

edited by malmstein

Loading

0nko commented Aug 9, 2024 •

edited

Loading

malmstein left a comment

malmstein Sep 10, 2024

0nko Sep 12, 2024

malmstein Sep 12, 2024

0nko commented Sep 12, 2024 •

edited

Loading

malmstein commented Sep 18, 2024

malmstein left a comment

malmstein Sep 19, 2024

malmstein Sep 19, 2024

malmstein Sep 19, 2024

malmstein Sep 19, 2024

malmstein Sep 19, 2024

malmstein Sep 19, 2024

malmstein Sep 19, 2024

malmstein Sep 19, 2024

		@@ -1165,6 +1168,8 @@ class BrowserTabViewModel @Inject constructor(

		canAutofillSelectCredentialsDialogCanAutomaticallyShow = true

		onLoadProgressChanged(progress = newWebNavigationState.progress ?: 0)

	android:fillColor="#000"/>
	android:fillColor="?attr/daxColorPrimaryIcon"/>

Omnibar position option #4885

Are you sure you want to change the base?

Omnibar position option #4885

Conversation

0nko commented Aug 9, 2024 • edited by malmstein Loading

Description

Testing instructions

Dummy toolbar

Autocomplete

UI changes

Snackbar

UI changes

Privacy pop-up dialog

UI changes

Onboarding messages

UI changes

0nko commented Aug 9, 2024 • edited Loading

malmstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0nko commented Sep 12, 2024 • edited Loading

malmstein commented Sep 18, 2024

malmstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0nko commented Aug 9, 2024 •

edited by malmstein

Loading

0nko commented Aug 9, 2024 •

edited

Loading

0nko commented Sep 12, 2024 •

edited

Loading