Closed Bug 1393579 Opened 7 years ago Closed 7 years ago

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

Categories

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

All
Android
enhancement

Tracking

(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

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

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Whiteboard: [mobileAS])

Attachments

(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 http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java#131 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.

https://reviewboard.mozilla.org/r/178958/#review184642

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java:135
(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/TopSitesCard.java:145
(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 michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/72a9bca22654
Show subdomain.domain for pages without titles in top sites. r=liuche
https://hg.mozilla.org/mozilla-central/rev/72a9bca22654
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.