Fire Mode: Themes and styles#8714
Conversation
f1373fa to
16acea2
Compare
512e75b to
cd5a7f4
Compare
cd5a7f4 to
9902db7
Compare
9902db7 to
f283aa0
Compare
f283aa0 to
8adda28
Compare
8adda28 to
ab607e5
Compare
malmstein
left a comment
There was a problem hiding this comment.
did not install or smoke-test the APK — these are static-only observations. solid, well-scoped theming change: the applyFireTheme hook on DuckDuckGoActivity with daxColorOmnibarAccent / daxOmnibarThemeOverlay driving the accent is clean, and pulling title resolution into a tested TabTitleResolver is a nice improvement (the at DuckDuckGo suffix fix even drops the old trailing-space bug). no rule violations and no blockers.
two things I'd want answered before approving: the grid↔list scroll-position behaviour change looks unrelated to theming and may be an accidental regression, and the empty-target-mode fade path looks like it can leave the recycler stuck at alpha 0 now that the fallback timer is gone. one minor nit on Uri.parse. also worth a glance: ic_fire_24.xml lost its trailing newline in this diff, looks accidental.
since I haven't exercised it on a device, I'd recommend smoke-testing the mode toggle (especially switching into an empty mode and toggling grid/list) before merging.
| scrollToActiveTab(viewModel.viewState.value.tabSwitcherItems) | ||
| attachOnScrolledListener() | ||
| } | ||
| scrollToActiveTab(viewModel.viewState.value.tabSwitcherItems) |
There was a problem hiding this comment.
we dropped getCurrentCenterOffset / scrollToPreviousCenterOffset here, so toggling grid↔list now jumps to the active tab instead of preserving the scroll position. that's a behaviour change that's unrelated to fire theming — is it intentional to fold it into this PR, or fallout from the recreate refactor?
There was a problem hiding this comment.
restored scroll preservation, rebuilt around what was actually breaking. the old getCurrentCenterOffset worked in estimated pixel metrics (computeVerticalScroll*) that differ between grid and list, so it drifted on every toggle. now it records the tab you last rested at the top (on scroll-settle) and re-pins that same tab after the swap, rather than re-deriving the anchor each time.
the grid was the tricky part: its post-swap re-layout (notifyDataSetChanged + the card re-measure) kept re-anchoring it a few rows off, so a one-shot scroll got undone. it now re-asserts the anchor on pre-draw for a short window after the toggle, which holds it. verified on device across repeated grid↔list toggles with 100+ tabs.
| if (scrolled) firstTimeLoadingTabsList = false | ||
| if (freshAfterModeSwitch) { | ||
| tabsRecycler.post(::revealNewMode) | ||
| if (fadingInAfterRecreate && !fadeInAnimationStarted && it.tabs.isNotEmpty()) { |
There was a problem hiding this comment.
the fade-in is guarded on it.tabs.isNotEmpty(), but configureRecycler already set tabsRecycler.alpha = 0f and itemAnimator = null. if we switch to a mode with no tabs the guard never fires, so the recycler stays invisible and fadingInAfterRecreate stays true until a tab eventually arrives. the old code had FADE_IN_FALLBACK_MS for exactly this "both modes empty" case — how does switching into an empty mode look now?
There was a problem hiding this comment.
the empty-target case is handled further up the stack: #8748 ("Fire Mode: Empty state pages") adds a dedicated empty-state view that replaces the recycler when a mode has no tabs, and skips the fade-out when switching out of an empty mode. so the recycler sitting at alpha 0 here is moot once that lands, and fire mode isn't user-enabled until the stack completes. leaving this as-is in 8714.
| val tabs = tabEntities.map { entity -> | ||
| val isActive = entity.tabId == activeTab?.tabId | ||
| val title = tabTitleResolver.resolveTitle(entity, browserMode) | ||
| val uri = entity.url?.let { Uri.parse(it) } |
There was a problem hiding this comment.
the selection branch above uses entity.url?.toUri() but here we still do entity.url?.let { Uri.parse(it) } — can we use toUri() in both for consistency?
There was a problem hiding this comment.
done — both branches now use entity.url?.toUri(), and dropped the now-unused android.net.Uri import.
# Conflicts: # app/src/main/res/values/donottranslate.xml
# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt
# Conflicts: # app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherActivity.kt
136f0b4 to
f3b79d4
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 136f0b4. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f3b79d4. Configure here.
|
restored the trailing newline on |


Task/Issue URL: https://app.asana.com/1/137249556945/project/1207418217763355/task/1215197767922294?focus=true
Description
Adds themes, styles and resources for use in the new Fire mode UI.
Note
Only applies to the Tab switcher and the normal omnibar UI, NOT the Input screen, native input widget. Fire mode empty screens in the Tab switcher and Browser are also not included.
Figam: https://www.figma.com/design/9S33EK9qIxZdN1FOIFHM5W/%F0%9F%94%A5-Fire-Mode-Tabs--iOS-Android-?node-id=1846-13998&m=dev
Steps to test this PR
Note
Fire-mode surfaces require a build where Fire mode is available (internal build with the feature enabled).
Fire mode disabled
Before starting the next tests, make sure to enable the
Fire tabsfeature flags in the internal settings and generate some regular tabs and Fire tabs in dev settings.Fire theme — Tab switcher
Fire theme — Browser
Tab titles
Undo snackbar action color
Fire theme — Dark mode
Note
Medium Risk
Touches core browser/tab-switcher UI and activity recreation on mode changes; regressions could affect theming, scrolling, or mode toggling outside Fire-only surfaces.
Overview
Introduces Fire mode visual theming for the browser and tab switcher: new
Theme.DuckDuckGo.*.Firestyles (mandarin accents, dark omnibar card overlay, fire tab icons/placeholders) and theme attrs such asdaxColorOmnibarAccent,daxColorSnackbarAction, anddaxOmnibarThemeOverlay.DuckDuckGoActivitygainsapplyFireTheme; Browser and TabSwitcher opt in whenBrowserMode.FIRE, and omnibar/progress/pull-to-refresh/cursor/snackbar styling reads those attrs instead of hard-coded blue.Tab switcher mode switching drops the bitmap overlay in favor of fade-out →
recreate()→ fade-in, with scroll-anchor preservation across grid/list toggles. Tab titles move toTabTitleResolver(e.g. “Fire Tab” vs “New Tab”) and flow throughTabSwitcherItem.title. ViewModel state consolidates layout type and toggle visibility intoviewState; bookmark undo snackbars can override action color to stay blue on fire-themed screens.Reviewed by Cursor Bugbot for commit f3b79d4. Bugbot is set up for automated code reviews on this repo. Configure here.