899644 - Tab switcher UI reset issues

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

(See Bug 898782)

* Open the tab tray, pick Synced Tabs
* Tap one, and immediately open the tray again.
* You'll be back where you were... and then when the page hits some loading point, the tab tray resets.

If you're slow, it looks like the tab tray always opens in local tabs, with the position set to the current tab. If you're fast, you can see where we reset the interface.

The second item seems to be related to the first (same symptoms), and is probably something funky like the loading point being a page load event that can trigger in the background while you're actually back in the tab list, doing something else.

We should probably switch this behavior to reposition the tab list immediately on picking a new URL to visit, with the assumption that next time you come back you'll want to pick an existing tab. (We could also do it every time you open the tray, but that will really infuriate indecisive users!)

Comment 1

[Mark Capella [:capella]]

Attachment: bug899644.diff (v0)

Something like this seems to wfm ... avoids that "reset the interface" jank ...

Comment 2

[Mark Capella [:capella]]

The interesting thing is, I don't really see the need for the block of code I moved to exist in the first place ?!
+                    final TabsPanel.Panel panel = tab.isPrivate() ?
+                        TabsPanel.Panel.PRIVATE_TABS : TabsPanel.Panel.NORMAL_TABS;
+                    if (areTabsShown() && mTabsPanel.getCurrentPanel() != panel) {
+                        showTabs(panel);

Simply yanking it (patch not posted) seems to correct the whole issue. Additionally, the use-case for the code block, as described in the patch that created it Bug 834399, ... doesn't seem to regress it's original issue:

"After closing a private tab, new selected tab is not visible in tray if it is non-private"

Gonna tinker some more while I wait for you to respond

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Capella [:capella] from comment #2)
> The interesting thing is, I don't really see the need for the block of code
> I moved to exist in the first place ?!
> +                    final TabsPanel.Panel panel = tab.isPrivate() ?
> +                        TabsPanel.Panel.PRIVATE_TABS :
> TabsPanel.Panel.NORMAL_TABS;
> +                    if (areTabsShown() && mTabsPanel.getCurrentPanel() !=
> panel) {
> +                        showTabs(panel);
> 
> Simply yanking it (patch not posted) seems to correct the whole issue.
> Additionally, the use-case for the code block, as described in the patch
> that created it Bug 834399, ... doesn't seem to regress it's original issue:
> 
> "After closing a private tab, new selected tab is not visible in tray if it
> is non-private"

I feel like this should still be necessary.  The use case is when you're on tablet, tabs tray open, and you close a private tab (that takes you back to a normal tab).  We still need to update the tabs panel to reflect your state, which is that you're looking at a normal tab.

Can you explain how removing the code would maintain this behaviour?  Have I made a mistake?

Comment 4

[Mark Capella [:capella]]

Currently on my tablet, closing a single remaining private tab while the TabsTray / TabsPanel is displayed on my tablet hides them, and visually does the appropriate tab / page switch.

Are you thinking it would / should close the private tab, keep the tabsTray open, then switch to / display the next available non-private tab?

Comment 5

[Mark Capella [:capella]]

Attachment: bug899644.diff (v1)

Posting the patch that wfm ...

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Capella [:capella] from comment #5)
> Created attachment 8428011 [details] [diff] [review]
> bug899644.diff (v1)
> 
> Posting the patch that wfm ...

I'm not ready to review this just yet, because I haven't yet understood why the original code was in place.  (The change itself looks fine, meaning I believe it does what you say it does; I'm just not convinced that removing it maintains all the behaviour the old author expected.)

Comment 7

[Mark Capella [:capella]]

heh, yah, but I've had it in my build queue for almost a week and I can't think of a way to break it ... we can bake it on m-c for a bit and repair anything that falls out (worst-case is to replace the whole 2 lines of code)

;)

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

dupe bug 1009303?

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8428011 [details] [diff] [review]
bug899644.diff (v1)

Review of attachment 8428011 [details] [diff] [review]:
-----------------------------------------------------------------

I tried to understand the original bug as best I could, and compare to current behaviour, and I don't think the original case can happen anymore.  For the record, our event handling around the tabs tray has changed a lot.  The current behaviour is: closing a private tab takes you to the previous private tab; if there are no more private tabs, the tabs tray closes and you go to a normal tab.

So I think this is good to go.

Comment 11

[Mark Capella [:capella]]

Agreed :)
https://hg.mozilla.org/integration/fx-team/rev/df906f3a95b5

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/df906f3a95b5