1475516 - The app switches to private mode when undoing closed normal tabs

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect, P2)

Description

[Catalin Suciu [:csuciu]]

master a0501543d
iPhone X (11.4)

1. Switch to private mode and open a private tab.
2. Open tabs tray in private mode and tap on the mask icon to switch to normal browsing mode.
3. While in tabs tray (normal browsing) select "Close all tabs"
4. When prompt, select "Undo" closed tabs.

Result: The app switches to private mode. Sometimes this happens at #4, right after selecting "Close all tabs"

Comment 1

[:garvan]

Confirmed is easy to repro, marked for p1, so we'll get it fixed ASAP

Comment 2

[damian.stanczyk]

Attachment: Pull request

So I have investigated this issue and this is what I found out:
- the difference between results depends which tab (private or non private) is now selected
- if selected is private tab (pink frame) and we will go to tabs tray (normal browsing) and than we will "Close all tabs" the result is: immediately switch to private tab 
- but if selected in normal tab (blue frame) but still we have to have at least one private tab opened and than we will "Close all tabs" only after pressing "Undo" we will switch to private tab

I fixed both issues, if anything is unclear please do not hesitate to ask. 

Also I would like to propose writing more tests for TabManager and maybe some refactoring? What you think about it?

Comment 3

[:garvan]

Nice work Damian!

Agreed TabManager.removeTab() needs more tests and refactoring, this is a critical function, and bugs here can break private mode behaviour.

I find the logic in this function hard to follow, because we have UI/UX restrictions that are reflecting in this lower-level class. And I can't tell which lines in this removeTab() function are fallbacks for unexpected cases vs. lines for well-defined UI states.

Thinking out loud: for instance, conditional code flows such as

if !tab.isPrivate && viableTabs.isEmpty {
 // do something
} else some other UI restriction {
 // do something
} ....

These UI states could be more clearly defined, maybe something like:
enum TabTrayEmptyState {
 case NormalModeNoTabs
 case PrivateModeNoTabs
 case NotEmpty
}

I am not suggesting this is the answer, but I feel this code will be less likely to break, and more likely to stay in-sync with future UI changes if it was written to follow well-defined UI states.

Comment 4

[:garvan]

My prev comment is not meant to be for this PR, agreed with landing a fix first.

Comment 5

[Farhan Patel [:farhan]]

landed