Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

FilippoZazzeroni · 2024-11-29T14:04:26Z

📜 Tickets

💡 Description

Bug fix App hang while opening the tab tray that is mainly caused by the TabTrayDisplayView.newState being called many times. Optimize scrollToTab method to wait for the collection view layout to finish, instead of forcing layout.
Bug fix swipe gesture to delete tab flashing back to normal position after the tab was dismissed.

Here is the link of the currently tracked app hangs Spreadsheet

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed, I updated documentation / comments for complex code and public methods
If needed, added a backport comment (example @Mergifyio backport release/v120)

…ix Swipe to close tab cell causing a blink when the animation is about to close

mobiletest-ci-bot · 2024-11-29T14:16:00Z

	Messages
📖	Project coverage: 33.1%
📖	Edited 5 files
📖	Created 0 files

Client.app: Coverage: 31.73

File	Coverage
TabDisplayPanelViewController.swift	72.41%	✅
TabDisplayView.swift	40.09%	⚠️
SwipeAnimator.swift	15.15%	⚠️
TabTrayViewController.swift	57.52%	✅
TabCell.swift	71.01%	✅

Generated by 🚫 Danger Swift against ec62f29

mergify · 2024-11-29T17:32:54Z

This pull request has conflicts when rebasing. Could you fix it @FilippoZazzeroni? 🙏

FilippoZazzeroni · 2024-11-29T17:44:58Z

@mergify backport release/v134

mergify · 2024-11-29T17:49:30Z

backport release/v134

✅ Backports have been created

#23657 Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open (backport #23474) has been created for branch release/v134

lmarceau · 2024-12-02T18:00:13Z

firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift

@@ -291,15 +291,19 @@ class TabTrayViewController: UIViewController,
    }

    func newState(state: TabTrayState) {
-        tabTrayState = state
-        updateTabCountImage(count: state.normalTabsCount)
+        defer {


Why do we need to defer the state?

Yes true we can avoid it. I added it in case in future we want to end the newState method on some if branch, so we don't forget to apply the state, since with this implementation it must be set as last method operation.

if state.shouldDismiss { delegate.didFinish() // having defer would be nice here so we don't forget applying the state return }

so, I'm confused here. We're not doing a check if the states are the same or not? what does defer get us vs setting tabTrayState = state at the beginning, and then updating, given that we're just basically update off state here??

Setting the state as first thing invalidates the checks because then tabTrayState will be always equal to state. Yes here the only state comparison is done on updateTabCountImage but yes you are right @adudenamedruby. i didn't implement the guard at the top cause i wasn't sure about any side effects it could cause. But i think that is the best idea

adudenamedruby · 2024-12-03T15:23:00Z

firefox-ios/Client/Frontend/Browser/Tabs/Views/TabDisplayPanelViewController.swift

@@ -162,9 +161,11 @@ class TabDisplayPanelViewController: UIViewController,
    }

    func newState(state: TabsPanelState) {
+        if state != tabsState {


would this be cleaner if it was a guard? so, guard tabsState != state else { return } and do the stuff only then? Or does the shouldShowEmptyView need to get called every time?

Yes i agree i'd be much more cleaner. The thing also for the other states i don't know if comparing the state and applying changes for different state could cause any side effects.
I believe it should be safe to do the comparison at the top since the state should be Equatable what do you think ?

I mean, if we're comparing at the top of newState we should be fine, I think, because it's only doing this particular state. And yeah, everything should be Equatable state wise, so we should be safe.

But maybe I didn't understand the question. What do you mean by "also for the other states i don't know if comparing the state and applying changes for different state could cause any side effects"

Sorry Rou i wasn't clear, i was referring to the other comment you wrote. So i'll go also for the TabTrayViewController.newState to have top comparison for the states

adudenamedruby · 2024-12-03T15:45:40Z

firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift

@@ -291,15 +291,19 @@ class TabTrayViewController: UIViewController,
    }

    func newState(state: TabTrayState) {
-        tabTrayState = state
-        updateTabCountImage(count: state.normalTabsCount)
+        defer {


so, I'm confused here. We're not doing a check if the states are the same or not? what does defer get us vs setting tabTrayState = state at the beginning, and then updating, given that we're just basically update off state here??

FilippoZazzeroni · 2024-12-09T10:55:14Z

@dataports i've merged main. You can test on new Xcode version if u want

…ny tabs open (#23474) * Little optimization on tab tray when updating counter image * Optimize TabDisplayPanel newState to avoid unnecessary view reload, Fix Swipe to close tab cell causing a blink when the animation is about to close * Refactor TabDisplayView to pipe collection view scroll after layout * Refactor as per code review * Refactor state to use tabState --------- Co-authored-by: Filippo <[email protected]> (cherry picked from commit 2d5125e)

…ny tabs open (backport #23474) (#23657) * Optimize TabDisplayPanel newState to avoid unnecessary view reload, Fix Swipe to close tab cell causing a blink when the animation is about to close * Refactor TabDisplayView to pipe collection view scroll after layout * Refactor state to use tabState --------- Co-authored-by: Filippo <[email protected]> (cherry picked from commit 2d5125e) Co-authored-by: FilippoZazzeroni <[email protected]>

dataports · 2024-12-11T03:25:09Z

@dataports i've merged main. You can test on new Xcode version if u want

I'm late, but I just did a quick test and everything looks good 👍 thanks!

…ny tabs open (backport #23474) (#23657) * Optimize TabDisplayPanel newState to avoid unnecessary view reload, Fix Swipe to close tab cell causing a blink when the animation is about to close * Refactor TabDisplayView to pipe collection view scroll after layout * Refactor state to use tabState --------- Co-authored-by: Filippo <[email protected]> (cherry picked from commit 2d5125e) Co-authored-by: FilippoZazzeroni <[email protected]>

FilippoZazzeroni · 2024-12-11T14:11:11Z

Ok thanks so much @dataports

Filippo added 3 commits November 29, 2024 09:58

Little optimization on tab tray when updating counter image

619de16

Optimize TabDisplayPanel newState to avoid unnecessary view reload, F…

c84b9c7

…ix Swipe to close tab cell causing a blink when the animation is about to close

Refactor TabDisplayView to pipe collection view scroll after layout

c46fc92

FilippoZazzeroni requested a review from a team as a code owner November 29, 2024 14:04

FilippoZazzeroni requested a review from lmarceau November 29, 2024 14:04

Merge branch 'main' into zazz/bugfix-tab-tray-opening-hangs

4752f98

lmarceau reviewed Dec 2, 2024

View reviewed changes

lmarceau changed the title ~~Bugfix FXIOS App hangs while opening Tab Tray when there are many tabs open~~ Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open Dec 2, 2024

FilippoZazzeroni requested a review from lmarceau December 3, 2024 10:16

lmarceau requested a review from adudenamedruby December 3, 2024 15:12

adudenamedruby reviewed Dec 3, 2024

View reviewed changes

Refactor as per code review

8d6d058

FilippoZazzeroni requested a review from adudenamedruby December 5, 2024 10:44

Refactor state to use tabState

c3ee9e5

adudenamedruby approved these changes Dec 5, 2024

View reviewed changes

FilippoZazzeroni requested a review from dataports December 6, 2024 10:26

dataports approved these changes Dec 9, 2024

View reviewed changes

Merge branch 'main' into zazz/bugfix-tab-tray-opening-hangs

ec62f29

FilippoZazzeroni merged commit 2d5125e into main Dec 10, 2024
10 checks passed

FilippoZazzeroni deleted the zazz/bugfix-tab-tray-opening-hangs branch December 10, 2024 11:51

mergify bot mentioned this pull request Dec 10, 2024

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open (backport #23474) #23657

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

FilippoZazzeroni commented Nov 29, 2024 •

edited

Loading

mobiletest-ci-bot commented Nov 29, 2024 •

edited

Loading

mergify bot commented Nov 29, 2024

FilippoZazzeroni commented Nov 29, 2024 •

edited

Loading

mergify bot commented Nov 29, 2024 •

edited

Loading

lmarceau Dec 2, 2024

FilippoZazzeroni Dec 3, 2024

adudenamedruby Dec 3, 2024

FilippoZazzeroni Dec 4, 2024 •

edited

Loading

adudenamedruby Dec 3, 2024

FilippoZazzeroni Dec 4, 2024

adudenamedruby Dec 4, 2024

FilippoZazzeroni Dec 4, 2024

adudenamedruby Dec 3, 2024

FilippoZazzeroni commented Dec 9, 2024

dataports commented Dec 11, 2024

FilippoZazzeroni commented Dec 11, 2024

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

Bugfix FXIOS-10756 App hangs while opening Tab Tray when there are many tabs open #23474

Conversation

FilippoZazzeroni commented Nov 29, 2024 • edited Loading

📜 Tickets

💡 Description

📝 Checklist

mobiletest-ci-bot commented Nov 29, 2024 • edited Loading

Client.app: Coverage: 31.73

mergify bot commented Nov 29, 2024

FilippoZazzeroni commented Nov 29, 2024 • edited Loading

mergify bot commented Nov 29, 2024 • edited Loading

✅ Backports have been created

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FilippoZazzeroni Dec 4, 2024 • edited Loading

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

FilippoZazzeroni commented Dec 9, 2024

dataports commented Dec 11, 2024

FilippoZazzeroni commented Dec 11, 2024

FilippoZazzeroni commented Nov 29, 2024 •

edited

Loading

mobiletest-ci-bot commented Nov 29, 2024 •

edited

Loading

FilippoZazzeroni commented Nov 29, 2024 •

edited

Loading

mergify bot commented Nov 29, 2024 •

edited

Loading

FilippoZazzeroni Dec 4, 2024 •

edited

Loading