Skip to content

Split Omnibar: Tab switcher bottom bar #7056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 10, 2025
Merged

Split Omnibar: Tab switcher bottom bar #7056

merged 4 commits into from
Nov 10, 2025

Conversation

@0nko
Copy link
Member

@0nko 0nko commented Nov 4, 2025 •
edited by malmstein
Loading

Task/Issue URL: https://app.asana.com/1/137249556945/project/1207418217763355/task/1211586528616254?focus=true

Description

This PR adds the bottom navigation bar to the tab switcher.

Steps to test this PR

  • Make sure the splitOmnibar feature flag is enabled in the internal settings
  • Go to Settings -> Appearance and enable the split omnibar
  • Go back to the browser
  • Open the tab switcher
  • Notice the bottom navigation bar is displayed
  • Verify the duck.ai button works as expected
  • Verify the New tab button works as expected
  • Verify the Fire button works as expected
  • Verify the Menu button shows the menu (with items reversed, like for the bottom omnibar)
  • Tap on the Select tabs menu item
  • Verify the bottom navigation bar is hidden
  • Verify the menu button is moved to the top toolbar
  • Try selecting tabs and verify it works as expected
  • Tap on the X button
  • Verify the bottom navigation bar is restored

This was referenced Nov 4, 2025
Merged
Merged
Merged
Merged
Merged
Merged
@0nko Graphite App
Copy link
Member Author

0nko commented Nov 4, 2025 •
edited
Loading

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

@0nko 0nko changed the title Add TabManager mode to the navigation bar Split Omnibar: Tab switcher bottom bar Nov 4, 2025
@0nko 0nko marked this pull request as ready for review November 4, 2025 16:43
@0nko 0nko requested a review from malmstein November 4, 2025 16:47
Base automatically changed from feature/ondrej/split-omnibar-ff-cleanup to develop November 5, 2025 10:43
@0nko 0nko force-pushed the feature/ondrej/split-omnibar-tab-switcher branch 2 times, most recently from 0bb063a to 7edb2eb Compare November 5, 2025 17:27
@0nko 0nko requested a review from mikescamell as a code owner November 5, 2025 17:27
@0nko 0nko mentioned this pull request Nov 5, 2025
Merged
12 tasks
@0nko 0nko removed the request for review from mikescamell November 5, 2025 17:33
@0nko
Copy link
Member Author

0nko commented Nov 5, 2025

I updated the bottom bar menu anchoring to cover the button here.

malmstein
malmstein reviewed Nov 5, 2025
View reviewed changes
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few UI things to discuss, but nothing blocking. Since there’s a discussion still open I’m just leaving the PR open to avoid confusion.

app/src/main/java/com/duckduckgo/app/appearance/AppearanceActivity.kt Outdated
OmnibarType.SINGLE_TOP, OmnibarType.SPLIT -> R.string.settingsAddressBarPositionTop
OmnibarType.SINGLE_TOP -> R.string.settingsAddressBarPositionTop
OmnibarType.SINGLE_BOTTOM -> R.string.settingsAddressBarPositionBottom
OmnibarType.SPLIT -> throw IllegalStateException("Split omnibar not supported in the position selection settings")
Copy link
Contributor

@malmstein malmstein Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you really wanna throw here? if this is some sort of state we don’t want to have, maybe it’s better to prevent it instead. Just crashing without doing or telling anything to the user is a bit drastic.

Copy link
Member Author

@0nko 0nko Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't meant to crash an app for a user, but rather let us find a bug early when testing (basically the fail fast principle). Also, once we remove the feature flags, we'll get rid of it.

This code is used to display the old settings where there's nothing to show for the SPLIT omnibar type. It's executed only when the required feature flags are disabled, meaning OmnibarFeatureRepository.isSplitOmnibarAvailable is false. We should never get here but we still need to "handle" the new omnibar type somehow. If we do get here, it means we have bigger problems/inconsistencies because the flags are used all over the place. There already is a mechanism to prevent this state.

Copy link
Member Author

@0nko 0nko Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna revert this, after all.

@0nko 0nko force-pushed the feature/ondrej/split-omnibar-tab-switcher branch 2 times, most recently from 4a5f878 to 4d95482 Compare November 7, 2025 14:40
@0nko 0nko mentioned this pull request Nov 7, 2025
Merged
@0nko 0nko requested a review from malmstein November 7, 2025 21:13
@0nko 0nko force-pushed the feature/ondrej/split-omnibar-tab-switcher branch from 1205ae9 to d5f78ae Compare November 7, 2025 22:03
@malmstein malmstein self-assigned this Nov 10, 2025
malmstein
malmstein approved these changes Nov 10, 2025
View reviewed changes
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works as expected, just a question about the pixel being fired

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt
}

fun onFireButtonTapped() {
pixel.fire(AppPixelName.FORGET_ALL_PRESSED_TABSWITCHING)
Copy link
Contributor

@malmstein malmstein Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the right pixel?

Copy link
Member Author

@0nko 0nko Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this pixel for the fire button has been around since 2019.

app/src/main/res/layout/activity_tab_switcher.xml
xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:tools="http://schemas.android.com/tools"
android:orientation="vertical"
android:animateLayoutChanges="true"
Copy link
Contributor

@malmstein malmstein Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what’s this for?

Copy link
Member Author

@0nko 0nko Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just to make the transition a bit nicer when you enter the selection mode and the bottom bar is hidden.

@0nko 0nko merged commit 7148198 into develop Nov 10, 2025
7 checks passed
@0nko 0nko deleted the feature/ondrej/split-omnibar-tab-switcher branch November 10, 2025 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malmstein malmstein malmstein approved these changes

Assignees

@malmstein malmstein

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@0nko @malmstein