Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(font-size): comment why source = Other happens #9363

Merged
merged 1 commit into from
Jul 15, 2019

Conversation

connorjclark
Copy link
Collaborator

Provide the steps to reproduce

  1. Run LH on http://m.sands-style.com/interviews-features (mobile)
node lighthouse-cli http://www.sands-style.com/ --only-audits=font-size

Or via DevTools.

What is the current behavior?

font-size will have an Unknown (10px).

What is the expected behavior?

It should not say Unknown.

how'd I find this?

I was curious when this audit resulted in Unknown, so I queried httparchive:

CREATE TEMP FUNCTION extractSources(itemsJson STRING)
RETURNS ARRAY<STRING>
LANGUAGE js AS """
  let items = JSON.parse(itemsJson);
  if (!Array.isArray(items)) return [];
  return JSON.parse(itemsJson).map(x=>x.source);
"""; 

SELECT COUNTIF(source = 'Unknown') UNKNOWN,
       COUNTIF(source = 'User Agent Stylesheet') USER_AGENT,
       COUNTIF(source = 'dynamic') DYNAMIC,
       COUNTIF(true) TOTAL
FROM (
  SELECT source
  FROM (
    SELECT extractSources(JSON_EXTRACT(report, '$.audits.font-size.details.items')) AS sources
    FROM `httparchive.latest.lighthouse_mobile`
--     LIMIT 1000
  ),
  UNNEST(sources) as source
)

result:

[
  {
    "UNKNOWN": "8626",
    "USER_AGENT": "111618",
    "DYNAMIC": "485029",
    "TOTAL": "14162843"
  }
]

That's ~0.05% (very few).

Gettings some URLs to work with:

SELECT DISTINCT url, source
FROM (
  SELECT url, extractSources(JSON_EXTRACT(report, '$.audits.font-size.details.items')) AS sources
  FROM `httparchive.latest.lighthouse_mobile`
),
UNNEST(sources) as source
WHERE source = 'Unknown'
LIMIT 10
[
  {
    "url": "http://m.sands-style.com/",
    "source": "Unknown"
  },
  {
    "url": "https://grayes.com/",
    "source": "Unknown"
  },
  {
    "url": "https://fins.money/",
    "source": "Unknown"
  },
  {
    "url": "http://m.espresso.repubblica.it/",
    "source": "Unknown"
  },
  {
    "url": "https://www.heimgourmet.com/",
    "source": "Unknown"
  },
  {
    "url": "https://www.albirplayahotel.com/",
    "source": "Unknown"
  },
  {
    "url": "https://www.twistys.com/",
    "source": "Unknown"
  },
  {
    "url": "http://www.kopi-ki.net/",
    "source": "Unknown"
  },
  {
    "url": "https://grayhawkgolf.com/",
    "source": "Unknown"
  },
  {
    "url": "http://thquanglong.pgdbadon.edu.vn/",
    "source": "Unknown"
  }
]

I picked the first site, saw the unknown font-size was 10, and ran this:

[...document.querySelectorAll('h3')].map(node => [parseFloat(window.getComputedStyle(node).fontSize), node]).sort((a,b) => a[0]-b[0])

and looked at some h3s set to 10. The CSS that sets the font-size to 10 is from a stlesheet, and is even mentioned in other items in the font-size table! weird.

I set a breakpoint in the font-size audit for Unknown, the stylesheetDeclaration is:

{
  "type": "Regular",
  "parentRule": {
    "origin": "regular",
    "selectors": [
      {
        "text": "#footer h3"
      }
    ]
  }
}

Note lack of a stylesheet property. This is set in the font-size gatherer here:

// @ts-ignore - guaranteed to exist from the filter immediately above

Finding the failing nodes for this:

analyzedFailingNodesData.map(n => n.cssRule).filter(r => r.parentRule.selectors.some(s => s.text === '#footer h3'))
[
  {
    "fontSize": 10,
    "textLength": 13,
    "node": { ... },
    "cssRule": {
      "type": "Regular",
      "parentRule": {
        "origin": "regular",
        "selectors": [
          {
            "text": "#footer h3"
          }
        ]
      }
    }
  },
  {
    "fontSize": 10,
    "textLength": 12,
    "node": {
        ...
      }
    },
    "cssRule": {
      "type": "Regular",
      "parentRule": {
        "origin": "regular",
        "selectors": [
          {
            "text": "#footer h3"
          }
        ]
      }
    }
  }
]

Note a lack of styleSheetId / styleSheet on the cssRules. Which I trace back to the limtiting we do:

.slice(0, MAX_NODES_SOURCE_RULE_FETCHED)

So that makes sense. I'll just leave an explanatory comment.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

aside, is PR lint bot broken @paulirish?

audits: isn't an approved type

lighthouse/.cz-config.js

Lines 13 to 22 in b925dbd

{value: 'new_audit', name: 'new_audit: A new audit'},
{value: 'core', name: 'core: Driver, gather, (non-new) audits, LHR JSON, etc'},
{value: 'tests', name: 'tests: Tests, smokehouse, etc'},
{value: 'i18n', name: 'i18n: Internationalization'},
{value: 'docs', name: 'docs: Documentation'},
{value: 'deps', name: 'deps: Dependency bumps only'},
{value: 'report', name: 'report: Report, UI, renderers'},
{value: 'cli', name: 'cli: CLI stuff'},
{value: 'clients', name: 'clients: Extension, DevTools, or LR stuff'},
{value: 'misc', name: 'misc: Something else entirely'}

@patrickhulce patrickhulce changed the title audits(font-size): comment why source = Other happens core(font-size): comment why source = Other happens Jul 13, 2019
@connorjclark connorjclark merged commit e6b3b8c into master Jul 15, 2019
@connorjclark connorjclark deleted the font-size-comment branch July 15, 2019 20:34
@brendankenny
Copy link
Member

comment why source = Other happens

er...

return {
  selector: '',
  source: 'Unknown',
};

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants