1270865 - Top sites are not correctly displayed after changing device orientation

Status: VERIFIED FIXED

(Firefox for iOS :: General, defect)

Description

[Simon Basca [:SimonB]]

Attachment: Screenshot of the issue

Build 4.0.0b7

Steps to reproduce;
1. Launch Firefox
2. Open Tabs tray
3. Open 6 new tabs
4. Close 3 tabs by swiping all three at the same time
5. Change device orientation
6. Open a new tab by pressing the + button from tabs tray 

Actual results:
- The top default sites are not correctly displayed

Expected results:
- The defaults sites should be correctly displayed

Comment 1

[Simon Basca [:SimonB]]

Attachment: Screenshot of the issue in landscape

Comment 2

[Emily Toop (:fluffyemily)]

Can this be reproduced in fewer steps (i.e. can this be repod more easily)

Comment 3

[Catalin Suciu [:csuciu]]

Attachment: Screen Shot 2016-05-10 at 18.49.16.png

On latest master running on iPhone 6 Plus (9.3.1) the issue is reproducible when:

1. Open 3 tabs in portrait
2. Swipe off two of them with a single gesture
3. Rotate to landscape
4. Open a new tab

Comment 4

[Aaron Train [:aaronmt]]

That latest screenshot is awful. We should fix this.

Comment 5

[Bryan Munar (:bkmunar)]

Attachment: PR

Comment 6

[Brian Nicholson (:bnicholson)]

Did you check that this doesn't cause the top sites panel to flicker when switching panels? That issue seems to appear every time the refresh logic is changed.

Comment 7

[Bryan Munar (:bkmunar)]

I did check and as far as I can tell, it didn't show the flicker of the empty state in all the cases I tried to execute. Also, that empty state code has to do with querying the dataSource.count(), and collection.reloadData() doesn't affect whether or not the empty state will appear or not.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8753413 [details] [review]
PR

Alternate STR:
1) Open the home panel in portrait.
2) Tap the tabs button.
3) Rotate the phone to landscape.
4) Tap the new tab button at the bottom.

I don't think the fix for this should be to throw another reloadData() call into TopSitesPanel since we already do this in viewWillTransitionToSize. This bug happens only on new tabs, but not when returning to an existing home panel tab, so I think the bug should address what's going wrong in that particular situation instead.

Digging into this, I found that this is a regression from bug 1263974. didCommitNavigation is fired for the new tab, which calls updateUIForReaderHomeStateForTab, which calls scrollController.showToolbars, which updates the home panel layout as a side effect. A better fix, I think, would be to change showToolbars() to not trigger an animation in the first place if the UI is already in the state being animated to. That is, do nothing in showToolbars if toolbarState == .Visible already (and similar idea for hideToolbars).

Comment 9

[Bryan Munar (:bkmunar)]

https://github.com/mozilla/firefox-ios/pull/1822/commits/15b84796b45a504e46ff2470795986a3e6607f43

Comment 10

[Simon Basca [:SimonB]]

The top sites are correctly displayed after changing device orientation on build 3c8f33c5.