Fix: Tab switcher UI issues #5736

0nko · 2025-03-05T17:55:48Z

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

Description

This PR fixes a couple of minor UI issues in the tab switcher:

Centrally align all elements in the tab card
Updates to the New tab page card - Dax logo and title change
Tab preview not generated when tabs swiped away
Fixed favicon loading when a NTP becomes a regular web page tab

Steps to test this PR

Tab switcher item layout

Verify the tab layout matches the design here and here

Tab switcher item layout

Open the tab switcher
Verify the screen scrolls to the current tab
Scroll away from the current tab
Switch the tab layout
Verify the screen keeps the previous scroll position (doesn't scroll to current tab)

Tab preview image

Make sure you have a few tabs
Swipe between them
Open the tab switcher
Verify they have a preview image

Favicon

Clear all tabs
Start with a new tab in the browser
Go to the tab switcher
Verify the dax icon and dax logo are displayed
Go back to the browser
Enter cnn.com and don't wait for it to fully load and open the tab switcher
Verify the favicon is eventually updated to the correct icon

mikescamell

I've noticed that in some instances we animate to the selected tab rather than being on it immediately. I need to look into how to reproduce.

Screen_recording_20250306_093644.mp4

mikescamell · 2025-03-06T07:52:10Z

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

@@ -92,6 +92,18 @@
            app:layout_constraintStart_toStartOf="parent"
            app:layout_constraintTop_toBottomOf="@id/title" />

+        <ImageView


❓Why do we need a new ImageView to load the dax logo into? What was stopping us from using the existing tabPreview ImageView and adjusting as needed?

The webpage previews need to use matrix scaling, which doesn't work for the logo. I thought it'd be complicating things trying to change it on the fly when views are recycled. But after giving it another thought it's not that bad: Use a single ImageView for all tabs

mikescamell · 2025-03-06T08:01:47Z

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

@@ -219,6 +219,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
            if (wasSwipingStarted) {
                wasSwipingStarted = false

+                currentTab?.onTabSwipedAway()
                viewModel.onTabsSwiped()


💡Could we not make this part of onTabPageSwiped? Feels like this belongs there as well

mikescamell · 2025-03-06T08:10:23Z

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

            app:layout_constraintHorizontal_bias="0.0"
            app:typography="h5"
-            tools:text="Slashdot" />
+            tools:text="Slashdot test tes tes tes tes tes tew tes tes tes tes tes tes tes" />



🔍Maybe we can reduce this a bit? 😅

mikescamell · 2025-03-06T08:26:42Z

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

-    android:layout_marginEnd="7dp"
-    android:layout_marginBottom="10dp">
+    android:layout_marginHorizontal="@dimen/keyline_2"
+    android:layout_marginVertical="6dp"


As discussed on MM, it's probably worth following up and finding out why our List item does not currently match the designs and if there is any good reason why 👍

mikescamell · 2025-03-06T08:34:21Z

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

-            app:layout_constraintTop_toTopOf="parent" />
+            android:src="@drawable/ic_dax_icon"
+            android:layout_marginStart="@dimen/keyline_2"
+            app:layout_constraintTop_toTopOf="@id/close"


❓Shouldn't this be keyline_3 (12dp)?

~~I don't think so. It seems like the 12dp margin in the spec is used to compensate for the narrow icon used.~~ Yes. Fixed.

This reverts commit 25987d5.

This reverts commit bbf050f.

mikescamell · 2025-03-12T18:25:12Z

@0nko Still seeing the animated scroll every now and then with the selected tab but I still think it's better than not scrolling at all to the current selected tab

Screen_recording_20250312_181815.mp4

Also the favicon is not updating if I switch to the TabSwitcher before the website loads

This reverts commit 3ef7813.

This reverts commit 10ceb61.

0nko · 2025-03-19T13:22:19Z

@mikescamell I've removed the tab switcher scroll bug fix and updated the favicon loading. Ready for another round.

mikescamell · 2025-03-19T15:01:34Z

@0nko Grid and List items look good design wise now! ✅

But I'm still not seeing a favicon load when i switch to the tab switcher before the website has finished loading. Ign.com was the website I was using

0nko · 2025-03-19T20:30:53Z

@mikescamell It's a timing issue -- the favicon isn't fetched when the tab switcher is displayed. I've added 3 retries, which should take care for the majority of cases.

mikescamell · 2025-03-20T11:19:40Z

app/src/main/java/com/duckduckgo/app/browser/favicon/DuckDuckGoFaviconManager.kt

-                view.loadFavicon(bitmap, url, placeholder)
+
+        // sometimes it takes a while to fetch a favicon, so we try to load it from disk again
+        repeat(FAVICON_LOAD_RETRIES) {


This works now! But as discussed on MM this is probably not the best long term fix and we probably need a reactive solution that loads the Favicon when it's ready, as this may not work still on a slow connection. As we discussed probably worth creating a task in the Android backlog to look into this more on a hack days/quick win day.

Created a task for us to come back to: https://app.asana.com/0/1202552961248957/1209728703137860/f

0nko requested a review from mikescamell March 5, 2025 17:55

mikescamell self-assigned this Mar 6, 2025

mikescamell reviewed Mar 6, 2025

View reviewed changes

0nko requested review from malmstein and nalcalag as code owners March 6, 2025 11:13

0nko requested a review from mikescamell March 6, 2025 11:13

0nko added 14 commits March 11, 2025 12:51

Fix the scroll to current tab

10ceb61

Fix tab favicon and preview for new tabs

f5c1286

Generate tab preview image when a tab is swiped

868a3c3

Fix the scroll to current tab

3ef7813

Fix the tab switcher grid item paddings

7d767d2

Fix the list item paddings

cccaec5

Revert "Fix the list item paddings"

715f7ff

This reverts commit 25987d5.

Use a single ImageView for all tabs

9f38fea

Clean up the code

94becfb

Fix the grid item logo and paddings to match the specs

5dbf677

Reapply "Fix the list item paddings"

4c3971b

This reverts commit bbf050f.

Fix the list item dimensions and paddings

6c81ce4

Clean up the tab switcher adapter

270dd69

Fix the favicon and preview loading

f80c7ce

0nko force-pushed the fix/ondrej/tab-switcher-ui-issues branch from ad1e231 to f80c7ce Compare March 11, 2025 19:44

Fix the unit tests

14fc760

0nko added 4 commits March 19, 2025 13:35

Revert "Fix the scroll to current tab"

5d29d5c

This reverts commit 3ef7813.

Revert "Fix the scroll to current tab"

c789b40

This reverts commit 10ceb61.

Retry the favicon load after 2 seconds if not available

4590bf8

Update the NTP check condition

661920c

Merge branch 'develop' into fix/ondrej/tab-switcher-ui-issues

9ba4ff3

Retry the favicon load 3 times

56089cc

mikescamell reviewed Mar 20, 2025

View reviewed changes

mikescamell approved these changes Mar 20, 2025

View reviewed changes

0nko merged commit 02811ad into develop Mar 20, 2025
8 checks passed

0nko deleted the fix/ondrej/tab-switcher-ui-issues branch March 20, 2025 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Tab switcher UI issues #5736

Fix: Tab switcher UI issues #5736

0nko commented Mar 5, 2025 •

edited by mikescamell

Loading

mikescamell left a comment

mikescamell Mar 6, 2025

0nko Mar 6, 2025

mikescamell Mar 6, 2025

mikescamell Mar 6, 2025

mikescamell Mar 6, 2025

mikescamell Mar 6, 2025

0nko Mar 6, 2025 •

edited

Loading

mikescamell commented Mar 12, 2025

0nko commented Mar 19, 2025

mikescamell commented Mar 19, 2025

0nko commented Mar 19, 2025

mikescamell Mar 20, 2025

0nko Mar 20, 2025

Fix: Tab switcher UI issues #5736

Fix: Tab switcher UI issues #5736

Conversation

0nko commented Mar 5, 2025 • edited by mikescamell Loading

Description

Steps to test this PR

mikescamell 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

0nko Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

mikescamell commented Mar 12, 2025

0nko commented Mar 19, 2025

mikescamell commented Mar 19, 2025

0nko commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0nko commented Mar 5, 2025 •

edited by mikescamell

Loading

0nko Mar 6, 2025 •

edited

Loading