Closed Bug 1393579 Opened 2 years ago Closed 2 years ago

AS top sites: what happens when you have a page with a path but don't have a page title?


(Firefox for Android :: Awesomescreen, enhancement, P1)




Firefox 57
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed


(Reporter: mcomella, Assigned: mcomella)



(Whiteboard: [mobileAS])


(2 files)

In bug 1390372, we determined, for top sites, if you have a page with a path, you should show a page title instead of "subdomain.domain", which is the standard for other top sites.

However, we didn't have specifications for when there is no page title so I opted to use the URL and it doesn't look great.

Bryan, Aaron, what do you think we should do? Should we just fall back to "subdomain.domain" if there's no page title?

btw, ignore the debug lines in the screenshots please! :)
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
> Bryan, Aaron, what do you think we should do? Should we just fall back to
> "subdomain.domain" if there's no page title?

Going back to "subdomain.domain" seems like the most reasonable thing to do for these odd ball pages with no title.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
tracking-fennec: ? → +
Priority: -- → P2
Rank: 3
This bug is simple: update to check if the title is empty before entering the first branch, else enter the second. And update the comment so it doesn't mirror the if statement (just explains why we want the results we do).

However, bug 1391413 modifies this statement so we should wait for it to land first.
Easy fix: let's rank it up.
Rank: 3 → 2
Assignee: nobody → michael.l.comella
Iteration: --- → 1.30
Rank: 2
Priority: P2 → P1
Comment on attachment 8907282 [details]
Bug 1393579: Show subdomain.domain for pages without titles in top sites.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/
(Diff revision 1)
>          // site titles were written for the old top sites, where we had more room to display titles: we want
>          // to provide them with more lines. However, it's complex to distinguish a distribution intended for
>          // the old top sites and the new one so for code simplicity, we allow all distributions more lines for titles.
>          title.setMaxLines(isSiteSuggestedFromDistribution ? 2 : 1);
> -        // At a high level, the logic is: if the path non-empty or the site is suggested by a distribution, use the page
> +        // From a UX perspective, we use titles because people refer to domains by their name ("it's on wikipedia")

This comment is useful, but I think it could be better if it was prefaced with something like "this is our preferred ordering of what is shown" and followed by an ordered list or something along those lines, so it's more explicit.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/
(Diff revision 1)
> -        // be shown. If we ever want better top site titles, we could create a heuristic to pull the title from parts
> -        // of the URL, page title, etc.
> -        if (wasException ||
> +        //
> +        // We use page titles with distributions because that's what those distributions expect to be shown.
> +        final String pageTitle = topSite.getTitle();
> +        if (isInvalidURI ||
>                  isSiteSuggestedFromDistribution ||
> -                !URIUtils.isPathEmpty(topSiteURI)) {
> +                (!URIUtils.isPathEmpty(topSiteURI) && !TextUtils.isEmpty(pageTitle))) {

I would break this link into a separate "else if" with an explanation because it's a little confusing that the pageTitle emptiness is being checked again - I'd sacrifice a tiny bit of efficiency for a more readable case statement.
Attachment #8907282 - Flags: review?(liuche) → review+
Pushed by
Show subdomain.domain for pages without titles in top sites. r=liuche
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.