mozilla / activity-stream

A refreshed "new tab page" for Firefox
Mozilla Public License 2.0
182 stars 113 forks source link

icon displays wrong character on some articles #286

Closed edwindotcom closed 8 years ago

edwindotcom commented 8 years ago
  1. goto: https://news.ycombinator.com/item?id=11230084
  2. open new tab and look at icon in history

actual:

screen shot 2016-03-07 at 11 54 23 am

expected: should be the letter 'N' or 'Y' for subdomain or domain.

pdehaan commented 8 years ago

Ah yes, it's defaulting to the first char in the title. Looks like a byproduct of #284.

Using @k88hudson's awesome new debugger branch from #283, it looks like we're just not getting the embedly metadata injected into the response, which will make the <SiteIconFallback/> fail.

const SiteIconFallback = React.createClass({
  render() {
    const {favicon_colors, title, provider_name, provider_display} = this.props;
    const letter = (provider_name || provider_display || title || "")[0];

— via /content-src/components/SiteIcon/SiteIcon.js:58

And as you can see below, we have no provider_name, provider_display to fall back to, so it reverts to title[0] which in this case is sadly ". In reality, we should never get as far as title in the above case, but we could potentially get a bit fancier with a RegExp and try and use the first A-Z character from the title so we know it's a printable character. Or better yet, use something like this, which would fall back to the parsed URL's hostname before touching the title (because the hostname should always be there and not a response from embedly proxy):

const letter = (provider_display || provider_name || parsedUrl.hostname || title)[0];
{
  "raw": {
    "TopSites": {
      "rows": [
        {
          "url": "https://news.ycombinator.com/item?id=11230084",
          "title": "“I've been working at Mozilla for many years, from peak to decline” | Hacker News",
          "lastVisitDate": 1457568060212,
          "frecency": 2000,
          "favicon": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAABj0lEQVQ4jZWTP0gCURzHXf0DDaKoyZ06CTVHc3NLU1CrFpipd+dgTeXUFCFETbXU0pJr0SQotLS5VN69d2hpIaaWJIbfhtPr0tNsePDe7/H7/L6/fwbKMV3KsaAc06U8i4mOxseg/SAc07t71LeeTetjGFRAYm6QTYc2muIYcYDE3H8oiLkhJ/woJucVUN8enUZxdw5ywv8DURVoAVEX5K0ZdJqvKB+vQgxZIYXteNpfRKdWgrw9CxJ1jQBwDKjgQyFgQu3qAJ/FPMSQFeL6FFoPWbxdp1AImEAFn5rWsIJ+DXgWX80qnlNLKO0toFOvgAieX2npA3oqxDULqpc7aN1n8ZG/QTWdhBi0gAo+nS4MEAmngKSIE+3KI9ovIqSIc6ClYwEMaNyHQsCIRu4M9cwpCgEjaHw4ui6A8iyI4IUYNKORO0c9cwIxaAYRvLpTqQvoK3i/S6N5e6EqmBzAeyCF7SgfraB8uAwpbFdHeRigt0wco0A2bJDCNsVZm/+oUR5uqRdU8OoWb7yCf6zzN96LhjqimbbsAAAAAElFTkSuQmCC",
          "type": "history",
          "parsedUrl": {
            "hash": "",
            "query": {},
            "protocol": "https:",
            "pathname": "/item",
            "auth": "",
            "host": "news.ycombinator.com",
            "port": "",
            "hostname": "news.ycombinator.com",
            "password": "",
            "username": "",
            "href": "https://news.ycombinator.com/item"
          }
        }
      ],
      "error": false
    },
pdehaan commented 8 years ago

Yeah, looks like using parsedUrl.hostname as a last-ditch effort at least gives us "N" instead of double quotes.

const SiteIconFallback = React.createClass({
  render() {
    const {favicon_colors, title, provider_name, provider_display, parsedUrl} = this.props;
    const letter = (provider_name || provider_display || parsedUrl.hostname || title || "")[0];

new_tab

chuckharmston commented 8 years ago

For what it's worth, we're using some Services from the browser to do something similar on Universal Search:

const {utils: Cu} = Components;

Cu.import('resource://gre/modules/XPCOMUtils.jsm');
XPCOMUtils.defineLazyModuleGetter(this, 'Services',
  'resource://gre/modules/Services.jsm');

function firstLetterOfBaseDomain(url) {
  const uri_obj = Services.io.newURI(url, null, null);
  return Services.eTLD.getBaseDomain(uri_obj)[0];
}
k88hudson commented 8 years ago

This will be fixed by https://github.com/mozilla/activity-streams/pull/293