Bugfix FXIOS-9416 Fix selected tab after undo tab close

[mattreaganmozilla]

:scroll: Tickets

Jira ticket Github issue

:bulb: Description

Fixes selected tab after undoing closing of a tab. Inserting the new tab as part of LegacyTabManager.undoCloseTab() changes the selected tab; the fix added is to check the selected tab before the undo and ensure it's still selected afterwards (unless the closed tab itself is selected).

Video

https://github.com/mozilla-mobile/firefox-ios/assets/145381717/a3eb257b-589a-438e-8668-30be37fbd972

:pencil: Checklist

You have to check all boxes before merging

• [x] Filled in the above information (tickets numbers and description of your work)
• [x] 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)

[mattreaganmozilla]

@dataports @OrlaM This is one of those fixes that seems too simple so I'm wondering if I'm missing something. But I tested all of the basic scenarios I could think of (closing + undoing tab at index 0, middle index, last index, selected tab, etc.) and wasn't able to see any issues. LMK if I might have overlooked something however. Thank you

[ih-codes]

Gave your branch a run with both the tab refactor enabled and disabled, appears to fix the issue! 👍

[ih-codes]

Actually @mattreaganmozilla, I was grabbing your fix for something I'm working on and noticed some issues. Can you test again against the latest main changes?

Reproduce (tab tray refactor active... seems ok if it's disabled):

1. Open 3 tabs
2. Select the last tab
3. Open tab tray again and close the active last tab
4. Undo close tab from toast
5. The second last tab is highlighted, but if you close the tab tray, the restored tab is actually active

Expected: The last (restored) tab should be highlighted in the tab tray after undo.

Example Video: https://github.com/mozilla-mobile/firefox-ios/assets/173110554/9426c5fc-e166-4b0b-b65c-9325e2006e02

I haven't looked that deep into it and I'm still learning about the app's tab management, but I think the TabModel in the middleware may have a different selection state than what's in the TabManager...

[mattreaganmozilla]

Thanks @ih-codes will take a look.

[mattreaganmozilla]

@ih-codes I can't repro this so far, have tried in a couple different scenarios and device types. Can you please double-check if there's anything missing from the repro steps (or if you have some other local change that might impact it)? Thanks

https://github.com/mozilla-mobile/firefox-ios/assets/145381717/1bc34403-1010-48cb-8544-1ce6fc29d7b0

[ih-codes]

Oh that's strange, I just tried resetting my sim / clearing derived data and I'm still seeing this... 🤔

Do you have TabTrayFlagManager.isRefactorEnabled returning true? I only see this behaviour when the tab tray refactor is enabled. Everything works as expected when it's false.

If it's at all helpful, I just pushed up ih/9416-cherry-pick in which I cherry-picked your commit + set the tab tray refactor to true for testing.

[mattreaganmozilla]

Do you have TabTrayFlagManager.isRefactorEnabled returning true? I only see this behaviour when the tab tray refactor is enabled. Everything works as expected when it's false.

@ih-codes I do have the flag enabled but I'm still not able to repro. 🤔

[ih-codes]

@mattreaganmozilla I've asked @dataports to take a look later when she has a chance!

[mattreaganmozilla]

@ih-codes Ok, so I can repro on your branch, and I can also repro on this branch if I rebase against main. It looks like this branch was behind and there were some recent changes there that further regressed something.

[ih-codes]

@ih-codes Ok, so I can repro on your branch, and I can also repro on this branch if I rebase against main. It looks like this branch was behind and there were some recent changes there that further regressed something.

Sorry I should have emphasized I was testing against the latest main changes! That was my guess too. I noticed this while working on another tab tray issue.

[mattreaganmozilla]

Sorry I should have emphasized I was testing against the latest main changes!

No that was my fault I didn't realize this branch was behind, I'll take a closer look at the bug, thanks for catching. 👍

[mattreaganmozilla]

Ok, so this regressed on main because of the change that was reverted which made tab selection synchronous. So even after calling tabManager.selectTab() for the tab which is restored in undoCloseTab(), the tabManager's selectTab isn't yet updated (because the related Task hasn't run).

So in the subsequent tab tray refresh, the tab model is updated incorrectly, even though we actually did select the correct tab in undoCloseTab. (I tested locally and confirmed that if we change it back to being synchronous, the bug no longer repros).

AFAIU @OrlaM will likely be updating main at some point soon make tab selection synchronous again, which will fix this particular issue. So I don't think it probably makes much sense for us to add a workaround/bandaid for this right now; my guess is we should probably just wait for the proper fix (synchronous tab selection). We will likely want the changes in this PR regardless.

If anyone has thoughts though LMK, I'll hold off on merging in the meantime. cc @dataports @ih-codes

[dataports]

Ok, so this regressed on main because of the change that was reverted which made tab selection synchronous. So even after calling tabManager.selectTab() for the tab which is restored in undoCloseTab(), the tabManager's selectTab isn't yet updated (because the related Task hasn't run).

So in the subsequent tab tray refresh, the tab model is updated incorrectly, even though we actually did select the correct tab in undoCloseTab. (I tested locally and confirmed that if we change it back to being synchronous, the bug no longer repros).

AFAIU @OrlaM will likely be updating main at some point soon make tab selection synchronous again, which will fix this particular issue. So I don't think it probably makes much sense for us to add a workaround/bandaid for this right now; my guess is we should probably just wait for the proper fix (synchronous tab selection). We will likely want the changes in this PR regardless.

If anyone has thoughts though LMK, I'll hold off on merging in the meantime. cc @dataports @ih-codes

Sounds good, I'm also running into some problems with other PRs with the synchronicity changes being reverted so I'm going to switch gears and see if I can look into that ahead of Orla getting back next week so hopefully we can move forward. Thanks for the update and looking into this!

[mobiletest-ci-bot]

	Messages
:book:	Project coverage: 31.81%
:book:	Edited 1 files
:book:	Created 0 files

Client.app: Coverage: 30.41

File	Coverage
LegacyTabManager.swift	29.09%	⚠️

Generated by :no_entry_sign: Danger Swift against 1f2ca1bc98c0dead185efae1ac0f8b6b282298fe

[dataports]

@Mergify backport release/v129

[mergify[bot]]

backport release/v129

✅ Backports have been created

• #21046 Bugfix FXIOS-9416 Fix selected tab after undo tab close (backport #20854) has been created for branch release/v129

[dataports]

@Mergify backport release/v128

[mergify[bot]]

backport release/v128

✅ Backports have been created

• #21047 Bugfix FXIOS-9416 Fix selected tab after undo tab close (backport #20854) has been created for branch release/v128