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

VERIFIED FIXED in Firefox 19

Status

()

defect
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

Trunk
Firefox 17
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox19 verified, fennec-)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Ian, is this something UX wants?
tracking-fennec: --- → ?
Keywords: uiwanted
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.
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.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Posted patch Patch (obsolete) — Splinter Review
Builds on code from patch 686528, should be pushed after that (currently checkin-needed).
Attachment #648142 - Flags: review?(mark.finkle)
Attachment #648142 - Attachment is patch: true
Comment on attachment 648142 [details] [diff] [review]
Patch

looks good to me. over to sriram for a second look.
Attachment #648142 - Flags: review?(sriram)
Attachment #648142 - Flags: review?(mark.finkle)
Attachment #648142 - Flags: review+
(In reply to Michael Comella (:mcomella) from comment #5)
> Created attachment 648143 [details]
> Screenshot after patch

Awesome. :)
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.
Attachment #648142 - Flags: review?(sriram) → review+
Posted patch Patch v2 (obsolete) — Splinter Review
Note: Previous patch had two r+.

In a similar vein to your recommendations, I made some additional changes to streamline the code.
Attachment #648142 - Attachment is obsolete: true
Attachment #648953 - Flags: review?(sriram)
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.
tracking-fennec: ? → -
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.
Attachment #648953 - Flags: review?(sriram) → review+
(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?
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.
Posted patch Patch v3Splinter Review
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.
Attachment #648953 - Attachment is obsolete: true
Attachment #653963 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3e2cbfcaccd4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.