778811 - "No Top Sites" text on about:home can be removed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

The text that appears when you have no top sites present is rarely seen – the user must clear browser history (or have never browsed before) and remove all bookmarks including the two that come pre-installed on the browser.

Having this piece of text in the layout (R.id.no_top_sites_text) complicates it unnecessarily, given the limited visibility. It can be removed.

Comment 1

[Aaron Train [:aaronmt]]

Ian, is this something UX wants?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Just to summarize:

We can still get into a situation where no top sites can be shown. It is very rare. We'd rather remove the overhead of having the placeholder for the text in the layout. This means when the rare situation happens, the area would just be empty... no text would display.

Removing the layout overhead and complexity is more valuable than having the "No Top Sites" hint display.

Comment 3

[Ian Barlow (:ibarlow)]

I agree. Let's treat Top Sites the same way we treat other sections on about:home. If there isn't anything to show, then hide the section.

Comment 4

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

Attachment: Patch

Builds on code from patch 686528, should be pushed after that (currently checkin-needed).

Comment 5

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

Attachment: Screenshot after patch

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 648142 [details] [diff] [review]
Patch

looks good to me. over to sriram for a second look.

Comment 7

[Ian Barlow (:ibarlow)]

(In reply to Michael Comella (:mcomella) from comment #5)
> Created attachment 648143 [details]
> Screenshot after patch

Awesome. :)

Comment 8

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 648142 [details] [diff] [review]
Patch

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

This looks good to me. Couple of changes are needed here:

1. visibilityWithoutTopSites is not used. You can remove this.
2. R.id.top_sites_title will alone be shown even if there are no top sites. I believe, the idea was to "show / hide" the entire section. In which case, it should take visibilityWithTopSites and not visibility.

Ideally we should be able to hide entire container -- and make sure if doesn't affect the layout of other containers.

Comment 9

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

Attachment: Patch v2

Note: Previous patch had two r+.

In a similar vein to your recommendations, I made some additional changes to streamline the code.

Comment 10

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

I should also mention that there were some comments I removed about the old intended behavior of the sync box where it would only display on the first run. I also streamlined this code in favor of the current functionality, where the sync box is shown on about:home every time until sync is set up.

Bug 766389 will probably be replacing the sync box with a "promo box" that can display multiple types of snippets (in particular, one for the Apps Marketplace) so I think it would be okay to leave the sync box behavior as it is currently and update the sync/promo box behavior with UX feedback in bug 766389.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 648953 [details] [diff] [review]
Patch v2

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

Looks good to me.
Ideally I would just turn the switch on entire container.. instead of individual elements. See if you can show/hide topsites altogether.
r+ with that possible change.

Comment 12

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #11) 
> Looks good to me.
> Ideally I would just turn the switch on entire container.. instead of
> individual elements. See if you can show/hide topsites altogether.
> r+ with that possible change.

The two ways I see to do this are 1) Put the top sites items in a new ViewGroup (which is not preferable due to performance considerations) or 2) create a new class in the same style as AboutHomeSection.

Are these what you had in mind? Which approach would you prefer?

Comment 13

[Sriram Ramasubramanian [:sriram]]

It's better to put it in ViewGroup -- there won't be a big performance regression, as this is one and only container for the entire life span of Firefox. If we are unnecessarily inflating a ViewGroup for a repeating thing, then yes, there is a performance regression. Not for this. Please use a ViewGroup.

Comment 14

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

Attachment: Patch v3

Moved r+, rebase to trunk. As discussed with Sriram in person, we are not combining the top sites elements into one section since the about:home logo divides any possible section into multiple parts.

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2cbfcaccd4

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3e2cbfcaccd4

Comment 17

[Cristian Nicolae (:xti)]

I cleared all history and removed all bookmarks and about:home looks like the sample from the screenshot. Closing bug as verified fixed on:

Firefox 19.0a1 (2012-10-09)
Device: Galaxy Note
OS: Android 4.0.4