From 4cc44c10c8cd7b4f33a88162235023492e1980bf Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 14 Sep 2017 00:29:22 +0200 Subject: [PATCH 01/65] Font Size audit - WIP --- lighthouse-core/audits/seo/font-size.js | 57 +++++++++++++++++++ .../gather/gatherers/seo/font-size.js | 24 ++++++++ 2 files changed, 81 insertions(+) create mode 100644 lighthouse-core/audits/seo/font-size.js create mode 100644 lighthouse-core/gather/gatherers/seo/font-size.js diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js new file mode 100644 index 000000000000..1a8b25c7fdd1 --- /dev/null +++ b/lighthouse-core/audits/seo/font-size.js @@ -0,0 +1,57 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const Audit = require('../audit'); +const ViewportAudit = require('../viewport'); +const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; + +class FontSize extends Audit { + /** + * @return {!AuditMeta} + */ + static get meta() { + return { + category: 'Mobile friendly', + name: 'font-size', + description: 'Document uses legible font sizes.', + failureDescription: 'Document doesn\'t use legible font sizes: ' + + '`target font-size >= 16px`, found', + helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + + 'visitors to “pinch to zoom” in order to read. ' + + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', + requiredArtifacts: ['FontSize'] + }; + } + + /** + * @param {!Artifacts} artifacts + * @return {!AuditResult} + */ + static audit(artifacts) { + const hasViewportSet = ViewportAudit.audit(artifacts).rawValue; + if (!hasViewportSet) { + return { + rawValue: false, + debugString: 'Text is illegible because of a missing viewport config', + }; + } + + const fontSizePx = artifacts.FontSize && parseInt(artifacts.FontSize, 10); + if (fontSizePx && fontSizePx < MINIMAL_LEGIBLE_FONT_SIZE_PX) { + return { + rawValue: false, + displayValue: `${fontSizePx}px`, + }; + } + + return { + rawValue: true, + }; + } +} + +module.exports = FontSize; diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js new file mode 100644 index 000000000000..250787a53800 --- /dev/null +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -0,0 +1,24 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const Gatherer = require('../gatherer'); + +class FontSize extends Gatherer { + + /** + * @param {{driver: !Object}} options Run options + * @return {!Promise} The font-size value of the document body + */ + afterPass(options) { + const driver = options.driver; + + return driver.evaluateAsync('getComputedStyle(document.body).fontSize'); + } +} + +module.exports = FontSize; + From ad797fc4e5f2ce4e241c25066edeeefa4b930876 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Sat, 16 Sep 2017 18:18:18 +0200 Subject: [PATCH 02/65] Calculate percentage of ilegible text on page. Show failing elements in a table. --- lighthouse-core/audits/seo/font-size.js | 48 +++++++++++++++++-- lighthouse-core/config/seo.js | 6 ++- .../gather/gatherers/seo/font-size.js | 29 +++++++++-- 3 files changed, 76 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 1a8b25c7fdd1..6862c1ef7086 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -8,6 +8,7 @@ const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; +const MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT = 75; class FontSize extends Audit { /** @@ -40,16 +41,57 @@ class FontSize extends Audit { }; } - const fontSizePx = artifacts.FontSize && parseInt(artifacts.FontSize, 10); - if (fontSizePx && fontSizePx < MINIMAL_LEGIBLE_FONT_SIZE_PX) { + let passingText = 0; + let failingText = 0; + const failingElements = []; + + for(const key in artifacts.FontSize) { + if (!artifacts.FontSize.hasOwnProperty(key)) { + continue; + } + + const result = artifacts.FontSize[key]; + + if (result.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + passingText += result.textLength; + } else { + failingText += result.textLength; + result.elements.forEach(element => { + failingElements.push({element, fontSize: result.fontSize}); + }); + } + } + + if (passingText === 0 && failingText === 0) { + return { + rawValue: true, + debugString: 'Page contains no text', + }; + } + + const percentageOfPassingText = passingText / (passingText + failingText) * 100; + + if (percentageOfPassingText < MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT) { + const headings = [ + {key: 'element', itemType: 'text', text: 'Element'}, + {key: 'fontSize', itemType: 'text', text: 'Font Size'} + ]; + + const details = Audit.makeTableDetails(headings, failingElements); + return { rawValue: false, - displayValue: `${fontSizePx}px`, + extendedInfo: { + value: failingElements + }, + details, + debugString: `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, }; } return { rawValue: true, + debugString: `${percentageOfPassingText.toFixed(2)}% of text is legible` }; } } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 3f9e18f078c3..09eafe8b0540 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -6,16 +6,19 @@ 'use strict'; module.exports = { - extends: 'lighthouse:default', + // extends: 'lighthouse:default', passes: [{ passName: 'defaultPass', gatherers: [ + 'viewport', 'seo/meta-description', + 'seo/font-size', ], }], audits: [ 'seo/meta-description', 'seo/http-status-code', + 'seo/font-size', ], groups: { 'seo-mobile': { @@ -41,6 +44,7 @@ module.exports = { {id: 'document-title', weight: 1, group: 'seo-content'}, {id: 'meta-description', weight: 1, group: 'seo-content'}, {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, + {id: 'font-size', weight: 1, group: 'seo-mobile'}, ], }, }, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 250787a53800..a4ba8ce693ba 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -6,17 +6,40 @@ 'use strict'; const Gatherer = require('../gatherer'); +const DOMHelpers = require('../../../lib/dom-helpers.js'); class FontSize extends Gatherer { /** * @param {{driver: !Object}} options Run options - * @return {!Promise} The font-size value of the document body + * @return {!Promise>} The font-size value of the document body */ afterPass(options) { - const driver = options.driver; + const expression = `(function() { + ${DOMHelpers.getElementsInDocumentFnString}; // define function on page + const elements = getElementsInDocument('body *'); - return driver.evaluateAsync('getComputedStyle(document.body).fontSize'); + return elements.reduce((result, element) => { + const textLength = Array.from(element.childNodes) + .filter(node => node.nodeType === Node.TEXT_NODE) + .reduce((sum, textNode) => sum + textNode.nodeValue.trim().length, 0); + + if(textLength) { + const fontSize = parseInt(getComputedStyle(element)["font-size"], 10); + + if (!result[fontSize]) { + result[fontSize] = {fontSize, textLength: 0, elements: []}; + } + + result[fontSize].textLength += textLength; + result[fontSize].elements.push(element.tagName); + } + + return result; + }, {}); + })()`; + + return options.driver.evaluateAsync(expression); } } From b2aa5c834355bc5aeaf1556436f1179230dd0a65 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Sun, 17 Sep 2017 21:51:13 +0200 Subject: [PATCH 03/65] Getting font-size information togheter with associated CSS rules - WIP --- .../gather/gatherers/seo/font-size.js | 77 +++++++++++++------ 1 file changed, 52 insertions(+), 25 deletions(-) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index a4ba8ce693ba..1eead5fd9304 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -6,7 +6,53 @@ 'use strict'; const Gatherer = require('../gatherer'); -const DOMHelpers = require('../../../lib/dom-helpers.js'); + +function findBody(node) { + const queue = [node]; + + while(queue.length > 0) { + const currentNode = queue.shift(); + + if(currentNode.nodeName === 'BODY') { + return currentNode; + } + + if(currentNode.children) { + currentNode.children.forEach(child => queue.push(child)); + } + } + + return null; +} + +function flattenTree(node) { + const flat = [node]; + const queue = [node]; + + while(queue.length > 0) { + const currentNode = queue.shift(); + + if(currentNode.children) { + currentNode.children.forEach(child => { + flat.push(child); + queue.push(child); + }); + } + } + + return flat; +} + +function getAllNodesFromBody(driver) { + return driver.sendCommand('DOM.enable') + .then(_ => driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true})) + .then(result => { + driver.sendCommand('DOM.disable'); + + const body = findBody(result.root); + return body ? flattenTree(body) : []; + }); +} class FontSize extends Gatherer { @@ -15,31 +61,12 @@ class FontSize extends Gatherer { * @return {!Promise>} The font-size value of the document body */ afterPass(options) { - const expression = `(function() { - ${DOMHelpers.getElementsInDocumentFnString}; // define function on page - const elements = getElementsInDocument('body *'); - - return elements.reduce((result, element) => { - const textLength = Array.from(element.childNodes) - .filter(node => node.nodeType === Node.TEXT_NODE) - .reduce((sum, textNode) => sum + textNode.nodeValue.trim().length, 0); - - if(textLength) { - const fontSize = parseInt(getComputedStyle(element)["font-size"], 10); - - if (!result[fontSize]) { - result[fontSize] = {fontSize, textLength: 0, elements: []}; - } - - result[fontSize].textLength += textLength; - result[fontSize].elements.push(element.tagName); - } - - return result; - }, {}); - })()`; + return getAllNodesFromBody(options.driver).then(result => { + console.log('result', result.length); - return options.driver.evaluateAsync(expression); + //TODO + // driver.sendCommand('CSS.enable') + }); } } From 64360dc1236a93671525ba67e2a1873405205afc Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 19 Sep 2017 01:30:09 +0200 Subject: [PATCH 04/65] Matched rules - WIP --- .../gather/gatherers/seo/font-size.js | 55 +++++++++++++++---- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 1eead5fd9304..56edbd5fb9a8 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -6,6 +6,7 @@ 'use strict'; const Gatherer = require('../gatherer'); +const TEXT_NODE = 3; function findBody(node) { const queue = [node]; @@ -27,15 +28,14 @@ function findBody(node) { function flattenTree(node) { const flat = [node]; - const queue = [node]; - while(queue.length > 0) { - const currentNode = queue.shift(); + for (let i = 0; i < flat.length; i++) { + const currentNode = flat[i]; if(currentNode.children) { currentNode.children.forEach(child => { + child.parentNode = currentNode; flat.push(child); - queue.push(child); }); } } @@ -44,16 +44,37 @@ function flattenTree(node) { } function getAllNodesFromBody(driver) { - return driver.sendCommand('DOM.enable') - .then(_ => driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true})) + return driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}) .then(result => { - driver.sendCommand('DOM.disable'); - const body = findBody(result.root); return body ? flattenTree(body) : []; }); } +function getFontSizeRule({inlineStyle, matchedCSSRules, inherited}) { + + // TODO https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMatchedStyles.js?type=cs&q=SDK.CSSMatchedS&sq=package:chromium&l=1 + + return {}; +} + +function getFontSizeInformation(driver, node) { + const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.nodeId}); + const matchedRules = driver.sendCommand('CSS.getMatchedStylesForNode', {nodeId: node.parentId}); + + return Promise.all([computedStyles, matchedRules]) + .then(result => { + const [{computedStyle}, matchedRules] = result; + const fontSizeProperty = computedStyle.find(({name}) => name === 'font-size'); + + return { + fontSize: parseInt(fontSizeProperty.value, 10), + textLength: node.nodeValue.trim().length, + cssRule: getFontSizeRule(matchedRules) + }; + }); +} + class FontSize extends Gatherer { /** @@ -61,11 +82,21 @@ class FontSize extends Gatherer { * @return {!Promise>} The font-size value of the document body */ afterPass(options) { - return getAllNodesFromBody(options.driver).then(result => { - console.log('result', result.length); + const enableDOM = options.driver.sendCommand('DOM.enable'); + const enableCSS = options.driver.sendCommand('CSS.enable'); + + return Promise.all([enableDOM, enableCSS]) + .then(() => getAllNodesFromBody(options.driver)) + .then(nodes => nodes.filter(node => node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0)) + .then(textNodes => Promise.all(textNodes.map(node => getFontSizeInformation(options.driver, node)))) + .then(fontSizeInfo => { + console.log(fontSizeInfo); + const result = {}; + + options.driver.sendCommand('DOM.disable'); + options.driver.sendCommand('CSS.disable'); - //TODO - // driver.sendCommand('CSS.enable') + return result; }); } } From 3f2effce1e4a99f3b43d1b851e2e0e30fe8d83fd Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 20 Sep 2017 22:50:34 +0200 Subject: [PATCH 05/65] Extracting style info - WIP --- lighthouse-core/gather/gatherers/seo/font-size.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 56edbd5fb9a8..7c2a35ee195e 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -55,6 +55,12 @@ function getFontSizeRule({inlineStyle, matchedCSSRules, inherited}) { // TODO https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMatchedStyles.js?type=cs&q=SDK.CSSMatchedS&sq=package:chromium&l=1 + console.log(JSON.stringify(inlineStyle)); + console.log('----'); + console.log(JSON.stringify(matchedCSSRules)); + console.log('----'); + console.log(JSON.stringify(inherited)); + return {}; } From fdf64736b7442b7ff433f4934bc884359b394d4d Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 26 Sep 2017 17:24:57 +0200 Subject: [PATCH 06/65] Expose effective rule --- lighthouse-core/audits/seo/font-size.js | 41 +++++++++++-------- .../gather/gatherers/seo/font-size.js | 35 +++++++++------- lighthouse-core/lib/web-inspector.js | 28 +++++++++++++ 3 files changed, 74 insertions(+), 30 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 6862c1ef7086..78584e976122 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -43,24 +43,26 @@ class FontSize extends Audit { let passingText = 0; let failingText = 0; - const failingElements = []; + const failingRules = new Map(); - for(const key in artifacts.FontSize) { - if (!artifacts.FontSize.hasOwnProperty(key)) { - continue; - } + artifacts.FontSize.forEach(item => { + if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + passingText += item.textLength; + } else { + failingText += item.textLength; - const result = artifacts.FontSize[key]; + const ruleKey = `${item.cssRule.styleSheetId}@${item.cssRule.range.startLine}:${item.cssRule.range.startColumn}`; - if (result.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { - passingText += result.textLength; - } else { - failingText += result.textLength; - result.elements.forEach(element => { - failingElements.push({element, fontSize: result.fontSize}); - }); + if(!failingRules.has(ruleKey)) { + failingRules.set(ruleKey, { + styleSheetId: item.cssRule.styleSheetId, + range: item.cssRule.range, + selectors: item.cssRule.parentRule.selectors, + fontSize: item.fontSize + }); + } } - } + }); if (passingText === 0 && failingText === 0) { return { @@ -77,12 +79,19 @@ class FontSize extends Audit { {key: 'fontSize', itemType: 'text', text: 'Font Size'} ]; - const details = Audit.makeTableDetails(headings, failingElements); + const details = Audit.makeTableDetails(headings, Array.from(failingRules).map(rule => { + const value = rule[1]; + + return { + element: value.selectors.map(item => item.text).join(', '), + fontSize: `${value.fontSize}px` + }; + })); return { rawValue: false, extendedInfo: { - value: failingElements + value: failingRules }, details, debugString: `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 7c2a35ee195e..ec2e79dc9746 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -5,8 +5,10 @@ */ 'use strict'; +const CSSMatchedStyles = require('../../../lib/web-inspector').CSSMatchedStyles; const Gatherer = require('../gatherer'); const TEXT_NODE = 3; +const FONT_SIZE_PROPERTY_NAME = 'font-size'; function findBody(node) { const queue = [node]; @@ -51,17 +53,25 @@ function getAllNodesFromBody(driver) { }); } -function getFontSizeRule({inlineStyle, matchedCSSRules, inherited}) { +function getFontSizeRule(node, {inlineStyle, matchedCSSRules, inherited}) { + const cssModel = { + styleSheetHeaderForId: id => { + return {id: id}; + } + }; - // TODO https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMatchedStyles.js?type=cs&q=SDK.CSSMatchedS&sq=package:chromium&l=1 + const nodeType = node.nodeType; + node.nodeType = () => nodeType; + const matchedStyles = new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); - console.log(JSON.stringify(inlineStyle)); - console.log('----'); - console.log(JSON.stringify(matchedCSSRules)); - console.log('----'); - console.log(JSON.stringify(inherited)); + const nodeStyles = matchedStyles.nodeStyles(); + const matchingRule = nodeStyles.find(style => { + const property = style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); + return property && + matchedStyles.propertyState(property) !== CSSMatchedStyles.PropertyState.Overloaded; + }); - return {}; + return matchingRule; } function getFontSizeInformation(driver, node) { @@ -71,12 +81,12 @@ function getFontSizeInformation(driver, node) { return Promise.all([computedStyles, matchedRules]) .then(result => { const [{computedStyle}, matchedRules] = result; - const fontSizeProperty = computedStyle.find(({name}) => name === 'font-size'); + const fontSizeProperty = computedStyle.find(({name}) => name === FONT_SIZE_PROPERTY_NAME); return { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, - cssRule: getFontSizeRule(matchedRules) + cssRule: getFontSizeRule(node, matchedRules) }; }); } @@ -96,13 +106,10 @@ class FontSize extends Gatherer { .then(nodes => nodes.filter(node => node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0)) .then(textNodes => Promise.all(textNodes.map(node => getFontSizeInformation(options.driver, node)))) .then(fontSizeInfo => { - console.log(fontSizeInfo); - const result = {}; - options.driver.sendCommand('DOM.disable'); options.driver.sendCommand('CSS.disable'); - return result; + return fontSizeInfo; }); } } diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index c10c39a065cd..0ab45d6ffae1 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -24,6 +24,21 @@ module.exports = (function() { global.window = global; } + global.Node = { + ELEMENT_NODE: 1 + }; + + global.CSSAgent = {}; + global.CSSAgent.StyleSheetOrigin = { + INJECTED: 'injected', + USER_AGENT: 'user-agent', + INSPECTOR: 'inspector', + REGULAR: 'regular' + }; + + global.CSS = {}; + global.CSS.supports = () => true; + global.Runtime = global.Runtime || {}; // Required for devtools-timeline-model @@ -304,5 +319,18 @@ module.exports = (function() { return ast; }; + // Dependecies for efective CSS rule calculation. + require('chrome-devtools-frontend/front_end/sdk/CSSMatchedStyles.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSMedia.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSMetadata.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSProperty.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSRule.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSStyleDeclaration.js'); + + WebInspector.CSSMetadata._generatedProperties = [ + {"name":"font-size","inherited":true}, + {"name":"color","inherited":true} + ]; + return WebInspector; })(); From 35599d3665ba57c456acce5b67d97f48e316d223 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 28 Sep 2017 17:52:06 +0200 Subject: [PATCH 07/65] Show what percentage of text each rule affected. Show table with failing selectors even if test passed. Clean up. --- lighthouse-core/audits/seo/font-size.js | 126 +++++++++++------- .../gather/gatherers/seo/font-size.js | 81 ++++++++--- lighthouse-core/lib/web-inspector.js | 14 +- 3 files changed, 147 insertions(+), 74 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 78584e976122..34a2ccdf55d3 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -10,6 +10,40 @@ const ViewportAudit = require('../viewport'); const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; const MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT = 75; +function getTotalTextLength(rules) { + return rules.reduce((sum, item) => sum + item.textLength, 0); +} + +function getFailingRules(fontSizeArtifact) { + const failingRules = new Map(); + + fontSizeArtifact.forEach(item => { + if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + return; + } + + const cssRule = item.cssRule; + const ruleKey = + `${cssRule.styleSheetId}@${cssRule.range.startLine}:${cssRule.range.startColumn}`; + let failingRule = failingRules.get(ruleKey); + + if (!failingRule) { + failingRule = { + styleSheetId: cssRule.styleSheetId, + range: cssRule.range, + selectors: cssRule.parentRule.selectors, + fontSize: item.fontSize, + textLength: 0 + }; + failingRules.set(ruleKey, failingRule); + } + + failingRule.textLength += item.textLength; + }); + + return failingRules.valuesArray(); +} + class FontSize extends Audit { /** * @return {!AuditMeta} @@ -19,11 +53,10 @@ class FontSize extends Audit { category: 'Mobile friendly', name: 'font-size', description: 'Document uses legible font sizes.', - failureDescription: 'Document doesn\'t use legible font sizes: ' + - '`target font-size >= 16px`, found', + failureDescription: 'Document doesn\'t use legible font sizes.', helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + - 'visitors to “pinch to zoom” in order to read. ' + - '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', + 'visitors to “pinch to zoom” in order to read. ' + + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', requiredArtifacts: ['FontSize'] }; } @@ -41,66 +74,59 @@ class FontSize extends Audit { }; } - let passingText = 0; - let failingText = 0; - const failingRules = new Map(); - - artifacts.FontSize.forEach(item => { - if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { - passingText += item.textLength; - } else { - failingText += item.textLength; - - const ruleKey = `${item.cssRule.styleSheetId}@${item.cssRule.range.startLine}:${item.cssRule.range.startColumn}`; - - if(!failingRules.has(ruleKey)) { - failingRules.set(ruleKey, { - styleSheetId: item.cssRule.styleSheetId, - range: item.cssRule.range, - selectors: item.cssRule.parentRule.selectors, - fontSize: item.fontSize - }); - } - } - }); + const totalTextLenght = getTotalTextLength(artifacts.FontSize); - if (passingText === 0 && failingText === 0) { + if (totalTextLenght === 0) { return { rawValue: true, debugString: 'Page contains no text', }; } - const percentageOfPassingText = passingText / (passingText + failingText) * 100; + const failingRules = getFailingRules(artifacts.FontSize); + const failingTextLength = getTotalTextLength(failingRules); + const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; - if (percentageOfPassingText < MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT) { - const headings = [ - {key: 'element', itemType: 'text', text: 'Element'}, - {key: 'fontSize', itemType: 'text', text: 'Font Size'} - ]; + const headings = [ + { + key: 'selector', + itemType: 'text', + text: 'CSS selector', + }, + { + key: 'percentage', + itemType: 'text', + text: '% of text', + }, + { + key: 'fontSize', + itemType: 'text', + text: 'Font Size', + } + ]; - const details = Audit.makeTableDetails(headings, Array.from(failingRules).map(rule => { - const value = rule[1]; + const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) + .map(rule => { + const percentageOfAffectedText = rule.textLength / totalTextLenght * 100; return { - element: value.selectors.map(item => item.text).join(', '), - fontSize: `${value.fontSize}px` + selector: rule.selectors.map(item => item.text).join(', '), + percentage: `${percentageOfAffectedText.toFixed(2)}%`, + fontSize: `${rule.fontSize}px`, }; - })); - - return { - rawValue: false, - extendedInfo: { - value: failingRules - }, - details, - debugString: `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, - }; - } + }); + const details = Audit.makeTableDetails(headings, tableData); + const passed = percentageOfPassingText >= MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT; return { - rawValue: true, - debugString: `${percentageOfPassingText.toFixed(2)}% of text is legible` + rawValue: passed, + extendedInfo: { + value: failingRules + }, + details, + debugString: passed ? + `${percentageOfPassingText.toFixed(2)}% of text is legible` : + `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.` }; } } diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index ec2e79dc9746..a8dca8ceabc6 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -10,17 +10,21 @@ const Gatherer = require('../gatherer'); const TEXT_NODE = 3; const FONT_SIZE_PROPERTY_NAME = 'font-size'; +/** + * @param {!Node} node top document node + * @returns {Node} body node + */ function findBody(node) { const queue = [node]; - while(queue.length > 0) { + while (queue.length > 0) { const currentNode = queue.shift(); - if(currentNode.nodeName === 'BODY') { + if (currentNode.nodeName === 'BODY') { return currentNode; } - if(currentNode.children) { + if (currentNode.children) { currentNode.children.forEach(child => queue.push(child)); } } @@ -28,13 +32,17 @@ function findBody(node) { return null; } +/** + * @param {!Node} node + * @returns {!Array} + */ function flattenTree(node) { const flat = [node]; for (let i = 0; i < flat.length; i++) { const currentNode = flat[i]; - if(currentNode.children) { + if (currentNode.children) { currentNode.children.forEach(child => { child.parentNode = currentNode; flat.push(child); @@ -45,6 +53,12 @@ function flattenTree(node) { return flat; } +/** + * Get list of all nodes from the document body. + * + * @param {!Object} driver + * @returns {!Array} + */ function getAllNodesFromBody(driver) { return driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}) .then(result => { @@ -53,27 +67,43 @@ function getAllNodesFromBody(driver) { }); } -function getFontSizeRule(node, {inlineStyle, matchedCSSRules, inherited}) { +/** + * Returns effective CSS rule for the font-size property + * + * @param {!string} CSS property name + * @param {!Node} node + * @param {!Object} matched CSS rules + * @returns + */ +function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherited}) { const cssModel = { - styleSheetHeaderForId: id => { - return {id: id}; - } + styleSheetHeaderForId: id => ({id}), }; const nodeType = node.nodeType; node.nodeType = () => nodeType; - const matchedStyles = new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); + const matchedStyles = + new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); const nodeStyles = matchedStyles.nodeStyles(); const matchingRule = nodeStyles.find(style => { - const property = style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); + const property = + style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); return property && matchedStyles.propertyState(property) !== CSSMatchedStyles.PropertyState.Overloaded; }); + debugger; return matchingRule; } +/** + * + * + * @param {!Object} driver + * @param {!Node} node + * @returns {!{fontSize: number, textLength: number, cssRule: !Object}} + */ function getFontSizeInformation(driver, node) { const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.nodeId}); const matchedRules = driver.sendCommand('CSS.getMatchedStylesForNode', {nodeId: node.parentId}); @@ -86,11 +116,19 @@ function getFontSizeInformation(driver, node) { return { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, - cssRule: getFontSizeRule(node, matchedRules) + cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node, matchedRules) }; }); } +/** + * @param {Node} node + * @returns {boolean} + */ +function isNonEmptyTextNode(node) { + return node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0; +} + class FontSize extends Gatherer { /** @@ -102,15 +140,18 @@ class FontSize extends Gatherer { const enableCSS = options.driver.sendCommand('CSS.enable'); return Promise.all([enableDOM, enableCSS]) - .then(() => getAllNodesFromBody(options.driver)) - .then(nodes => nodes.filter(node => node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0)) - .then(textNodes => Promise.all(textNodes.map(node => getFontSizeInformation(options.driver, node)))) - .then(fontSizeInfo => { - options.driver.sendCommand('DOM.disable'); - options.driver.sendCommand('CSS.disable'); - - return fontSizeInfo; - }); + .then(() => getAllNodesFromBody(options.driver)) + .then(nodes => nodes.filter(isNonEmptyTextNode)) + .then(textNodes => Promise.all( + textNodes.map(node => getFontSizeInformation(options.driver, node)) + )) + .then(fontSizeInfo => { + options.driver.sendCommand('DOM.disable'); + options.driver.sendCommand('CSS.disable'); + + debugger; + return fontSizeInfo; + }); } } diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index 0ab45d6ffae1..9fda42b66828 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -25,7 +25,7 @@ module.exports = (function() { } global.Node = { - ELEMENT_NODE: 1 + ELEMENT_NODE: 1, }; global.CSSAgent = {}; @@ -33,7 +33,7 @@ module.exports = (function() { INJECTED: 'injected', USER_AGENT: 'user-agent', INSPECTOR: 'inspector', - REGULAR: 'regular' + REGULAR: 'regular', }; global.CSS = {}; @@ -328,8 +328,14 @@ module.exports = (function() { require('chrome-devtools-frontend/front_end/sdk/CSSStyleDeclaration.js'); WebInspector.CSSMetadata._generatedProperties = [ - {"name":"font-size","inherited":true}, - {"name":"color","inherited":true} + { + name: 'font-size', + inherited: true, + }, + { + name: 'color', + inherited: true, + }, ]; return WebInspector; From 483a5c289c41b461c43a5dcf2ffde92b3260b4a4 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 3 Oct 2017 01:31:10 +0200 Subject: [PATCH 08/65] Work on inline styles, clean up jsdoc --- lighthouse-core/audits/seo/font-size.js | 88 +++++++++++++------ .../gather/gatherers/seo/font-size.js | 32 +++---- 2 files changed, 76 insertions(+), 44 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 34a2ccdf55d3..d527c17f5112 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -7,43 +7,82 @@ const Audit = require('../audit'); const ViewportAudit = require('../viewport'); +const CSSStyleDeclaration = require('../../lib/web-inspector').CSSStyleDeclaration; const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; const MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT = 75; +/** + * @param {!Array<{textLength:number}>} rules + * @returns number + */ function getTotalTextLength(rules) { return rules.reduce((sum, item) => sum + item.textLength, 0); } +/** + * @param {Array<{cssRule: WebInspector.CSSStyleDeclaration, fontSize: number, textLength: number, node: Node}>} fontSizeArtifact + * @returns {Array<{cssRule: WebInspector.CSSStyleDeclaration, fontSize: number, textLength: number, node: Node}>} + */ function getFailingRules(fontSizeArtifact) { const failingRules = new Map(); - fontSizeArtifact.forEach(item => { - if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + fontSizeArtifact.forEach(({cssRule, fontSize, textLength, node}) => { + if (fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { return; } - const cssRule = item.cssRule; - const ruleKey = - `${cssRule.styleSheetId}@${cssRule.range.startLine}:${cssRule.range.startColumn}`; - let failingRule = failingRules.get(ruleKey); + const artifactId = getFontArtifactId(cssRule, node); + const failingRule = failingRules.get(artifactId); if (!failingRule) { - failingRule = { - styleSheetId: cssRule.styleSheetId, - range: cssRule.range, - selectors: cssRule.parentRule.selectors, - fontSize: item.fontSize, - textLength: 0 - }; - failingRules.set(ruleKey, failingRule); + failingRules.set(artifactId, { + node, + cssRule, + fontSize, + textLength + }); + } else { + failingRule.textLength += textLength; } - - failingRule.textLength += item.textLength; }); return failingRules.valuesArray(); } +/** + * @param {WebInspector.CSSStyleDeclaration} rule + * @param {Node} node + * @returns string + */ +function ruleToSource(rule, node) { + if (rule.type === CSSStyleDeclaration.Type.Regular) { + return rule.selectors.map(item => item.text).join(', '); + } + + const attributes = node.attributes.map((item, idx) => { + if (idx % 2 === 0) { + return ` ${item}`; + } else { + return item ? `="${item}"` : ''; + } + }).join(''); + + return `<${node.localName}${attributes}>`; +} + +/** + * @param {WebInspector.CSSStyleDeclaration} rule + * @param {Node} node + * @return string + */ +function getFontArtifactId(rule, node) { + if (rule.type === CSSStyleDeclaration.Type.Regular) { + return `${rule.styleSheetId}@${rule.range.startLine}:${rule.range.startColumn}`; + } else { + return `node_${node.id}`; + } +} + class FontSize extends Audit { /** * @return {!AuditMeta} @@ -89,9 +128,9 @@ class FontSize extends Audit { const headings = [ { - key: 'selector', - itemType: 'text', - text: 'CSS selector', + key: 'source', + itemType: 'code', + text: 'Source', }, { key: 'percentage', @@ -106,13 +145,13 @@ class FontSize extends Audit { ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) - .map(rule => { - const percentageOfAffectedText = rule.textLength / totalTextLenght * 100; + .map(({cssRule, node, textLength, fontSize}) => { + const percentageOfAffectedText = textLength / totalTextLenght * 100; return { - selector: rule.selectors.map(item => item.text).join(', '), + source: ruleToSource(cssRule, node), percentage: `${percentageOfAffectedText.toFixed(2)}%`, - fontSize: `${rule.fontSize}px`, + fontSize: `${fontSize}px`, }; }); const details = Audit.makeTableDetails(headings, tableData); @@ -120,9 +159,6 @@ class FontSize extends Audit { return { rawValue: passed, - extendedInfo: { - value: failingRules - }, details, debugString: passed ? `${percentageOfPassingText.toFixed(2)}% of text is legible` : diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index a8dca8ceabc6..bdc4f36c7d24 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -11,8 +11,8 @@ const TEXT_NODE = 3; const FONT_SIZE_PROPERTY_NAME = 'font-size'; /** - * @param {!Node} node top document node - * @returns {Node} body node + * @param {!Node} node Top document node + * @returns {Node} */ function findBody(node) { const queue = [node]; @@ -68,12 +68,12 @@ function getAllNodesFromBody(driver) { } /** - * Returns effective CSS rule for the font-size property + * Returns effective CSS rule for given CSS property * - * @param {!string} CSS property name + * @param {!string} property CSS property name * @param {!Node} node - * @param {!Object} matched CSS rules - * @returns + * @param {!Object} matched CSS rule + * @returns {WebInspector.CSSStyleDeclaration} */ function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherited}) { const cssModel = { @@ -87,25 +87,21 @@ function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherit const nodeStyles = matchedStyles.nodeStyles(); const matchingRule = nodeStyles.find(style => { - const property = - style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); - return property && - matchedStyles.propertyState(property) !== CSSMatchedStyles.PropertyState.Overloaded; + const foundProperty = style.allProperties.find(item => item.name === property); + return foundProperty && + matchedStyles.propertyState(foundProperty) !== CSSMatchedStyles.PropertyState.Overloaded; }); - debugger; return matchingRule; } /** - * - * * @param {!Object} driver * @param {!Node} node - * @returns {!{fontSize: number, textLength: number, cssRule: !Object}} + * @returns {!{fontSize: number, textLength: number, cssRule: WebInspector.CSSStyleDeclaration}} */ function getFontSizeInformation(driver, node) { - const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.nodeId}); + const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.parentId}); const matchedRules = driver.sendCommand('CSS.getMatchedStylesForNode', {nodeId: node.parentId}); return Promise.all([computedStyles, matchedRules]) @@ -116,7 +112,8 @@ function getFontSizeInformation(driver, node) { return { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, - cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node, matchedRules) + node: node.parentNode, + cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node.parentNode, matchedRules) }; }); } @@ -133,7 +130,7 @@ class FontSize extends Gatherer { /** * @param {{driver: !Object}} options Run options - * @return {!Promise>} The font-size value of the document body + * @return {!Promise>} The font-size value of the document body */ afterPass(options) { const enableDOM = options.driver.sendCommand('DOM.enable'); @@ -149,7 +146,6 @@ class FontSize extends Gatherer { options.driver.sendCommand('DOM.disable'); options.driver.sendCommand('CSS.disable'); - debugger; return fontSizeInfo; }); } From 9a10a10c5d0c898a679bb82e686ee908f0b28904 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 3 Oct 2017 17:47:04 +0200 Subject: [PATCH 09/65] Handle inline styels, attribute styles and user agent styles. --- lighthouse-core/audits/seo/font-size.js | 57 +++++++++---------- .../gather/gatherers/seo/font-size.js | 19 ++++++- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index d527c17f5112..f77d9fbe13cd 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -49,25 +49,34 @@ function getFailingRules(fontSizeArtifact) { return failingRules.valuesArray(); } +function nodeToString(node) { + return node.localName; +} + /** * @param {WebInspector.CSSStyleDeclaration} rule * @param {Node} node * @returns string */ -function ruleToSource(rule, node) { - if (rule.type === CSSStyleDeclaration.Type.Regular) { - return rule.selectors.map(item => item.text).join(', '); +function getOriginDescription(rule, node) { + if (!rule) { + return 'User Agent Stylesheet'; } - const attributes = node.attributes.map((item, idx) => { - if (idx % 2 === 0) { - return ` ${item}`; - } else { - return item ? `="${item}"` : ''; - } - }).join(''); + if (rule.type === CSSStyleDeclaration.Type.Attributes) { + return `Attributes Style: ${nodeToString(node)}`; + } + + if (rule.type === CSSStyleDeclaration.Type.Inline) { + return `Inline Style: ${nodeToString(node)}`; + } + + if (rule.type === CSSStyleDeclaration.Type.Regular && rule.parentRule) { + const selector = rule.parentRule.selectors.map(item => item.text).join(', '); + return `CSS Selector: ${selector}`; + } - return `<${node.localName}${attributes}>`; + return 'unknown'; } /** @@ -76,10 +85,12 @@ function ruleToSource(rule, node) { * @return string */ function getFontArtifactId(rule, node) { - if (rule.type === CSSStyleDeclaration.Type.Regular) { + if (!rule) { + return 'user-agent'; + } else if (rule.type === CSSStyleDeclaration.Type.Regular) { return `${rule.styleSheetId}@${rule.range.startLine}:${rule.range.startColumn}`; } else { - return `node_${node.id}`; + return `node_${node.nodeId}`; } } @@ -127,21 +138,9 @@ class FontSize extends Audit { const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; const headings = [ - { - key: 'source', - itemType: 'code', - text: 'Source', - }, - { - key: 'percentage', - itemType: 'text', - text: '% of text', - }, - { - key: 'fontSize', - itemType: 'text', - text: 'Font Size', - } + {key: 'origin', itemType: 'code', text: 'Origin'}, + {key: 'percentage', itemType: 'text', text: '% of text'}, + {key: 'fontSize', itemType: 'text', text: 'Font Size'} ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) @@ -149,7 +148,7 @@ class FontSize extends Audit { const percentageOfAffectedText = textLength / totalTextLenght * 100; return { - source: ruleToSource(cssRule, node), + origin: getOriginDescription(cssRule, node), percentage: `${percentageOfAffectedText.toFixed(2)}%`, fontSize: `${fontSize}px`, }; diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index bdc4f36c7d24..3abbb2152707 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -75,15 +75,28 @@ function getAllNodesFromBody(driver) { * @param {!Object} matched CSS rule * @returns {WebInspector.CSSStyleDeclaration} */ -function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherited}) { +function getEffectiveRule(property, node, { + inlineStyle, + attributesStyle, + matchedCSSRules, + inherited +}) { const cssModel = { styleSheetHeaderForId: id => ({id}), }; const nodeType = node.nodeType; node.nodeType = () => nodeType; - const matchedStyles = - new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); + const matchedStyles = new CSSMatchedStyles( + cssModel, + node, + inlineStyle, + attributesStyle, + matchedCSSRules, + null, + inherited, + null + ); const nodeStyles = matchedStyles.nodeStyles(); const matchingRule = nodeStyles.find(style => { From 4b0863cc6d1312ee86a7974196364e3f9cfb84ca Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 4 Oct 2017 16:33:43 +0200 Subject: [PATCH 10/65] Expose stylesheet URL, line number and column. --- lighthouse-core/audits/seo/font-size.js | 85 +++++++++++++++++++------ lighthouse-core/config/seo.js | 2 + 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index f77d9fbe13cd..131fbc5e1c62 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -5,6 +5,7 @@ */ 'use strict'; +const {URL} = require('url'); const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const CSSStyleDeclaration = require('../../lib/web-inspector').CSSStyleDeclaration; @@ -50,33 +51,75 @@ function getFailingRules(fontSizeArtifact) { } function nodeToString(node) { - return node.localName; + const attributesString = node.attributes.map((value, idx) => { + if(idx % 2 === 0) { + return ` ${value}`; + } + + return value ? `='${value}'` : ''; + }).join(''); + + return `<${node.localName}${attributesString}>`; } /** - * @param {WebInspector.CSSStyleDeclaration} rule + * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets * @param {Node} node + * @param {string} baseURL + * @param {WebInspector.CSSStyleDeclaration} styleDeclaration * @returns string */ -function getOriginDescription(rule, node) { - if (!rule) { - return 'User Agent Stylesheet'; +function getOrigin(stylesheets, node, baseURL, styleDeclaration) { + if (!styleDeclaration) { + return { + details: 'User Agent Stylesheet' + }; } - if (rule.type === CSSStyleDeclaration.Type.Attributes) { - return `Attributes Style: ${nodeToString(node)}`; + const range = styleDeclaration && styleDeclaration.range; + let file = baseURL; + let location = ''; + + if(range) { + location = `${range.startLine}:${range.startColumn}`; } - if (rule.type === CSSStyleDeclaration.Type.Inline) { - return `Inline Style: ${nodeToString(node)}`; + if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes) { + return { + details: `Attributes Style: ${nodeToString(node)}`, + file, + location + }; } - if (rule.type === CSSStyleDeclaration.Type.Regular && rule.parentRule) { - const selector = rule.parentRule.selectors.map(item => item.text).join(', '); - return `CSS Selector: ${selector}`; + if (styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { + return { + details: `Inline Style: ${nodeToString(node)}`, + file, + location + }; + } + + if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular && styleDeclaration.parentRule) { + const rule = styleDeclaration.parentRule; + const selector = rule.selectors.map(item => item.text).join(', '); + const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); + + if (stylesheetMeta) { + const url = new URL(stylesheetMeta.header.sourceURL, baseURL); + file = `${url.href}`; + } + + return { + details: `CSS Selector: ${selector}`, + file, + location + }; } - return 'unknown'; + return { + details: 'Unknown' + }; } /** @@ -107,7 +150,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize'] + requiredArtifacts: ['FontSize', 'Styles', 'URL'] }; } @@ -136,19 +179,25 @@ class FontSize extends Audit { const failingRules = getFailingRules(artifacts.FontSize); const failingTextLength = getTotalTextLength(failingRules); const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; + const pageUrl = artifacts.URL.finalUrl; const headings = [ - {key: 'origin', itemType: 'code', text: 'Origin'}, - {key: 'percentage', itemType: 'text', text: '% of text'}, + {key: 'file', itemType: 'url', text: 'File'}, + {key: 'location', itemType: 'url', text: 'Line and Column'}, + {key: 'details', itemType: 'code', text: 'Details'}, + {key: 'percentage', itemType: 'text', text: '% of Text'}, {key: 'fontSize', itemType: 'text', text: 'Font Size'} ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) .map(({cssRule, node, textLength, fontSize}) => { const percentageOfAffectedText = textLength / totalTextLenght * 100; + const origin = getOrigin(artifacts.Styles, node, pageUrl, cssRule); return { - origin: getOriginDescription(cssRule, node), + file: origin.file, + location: origin.location, + details: origin.details, percentage: `${percentageOfAffectedText.toFixed(2)}%`, fontSize: `${fontSize}px`, }; @@ -160,7 +209,7 @@ class FontSize extends Audit { rawValue: passed, details, debugString: passed ? - `${percentageOfPassingText.toFixed(2)}% of text is legible` : + `${percentageOfPassingText.toFixed(2)}% of text is legible.` : `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.` }; } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 09eafe8b0540..c7cd01ec3404 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -13,6 +13,8 @@ module.exports = { 'viewport', 'seo/meta-description', 'seo/font-size', + 'url', + 'styles', ], }], audits: [ From 005d30840306eec7154c130924c5e77557317d9a Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 10 Oct 2017 02:25:13 +0200 Subject: [PATCH 11/65] Update output table according to the latst decisions. --- lighthouse-core/audits/seo/font-size.js | 74 ++++++------------- lighthouse-core/config/seo.js | 8 +- .../gather/gatherers/seo/font-size.js | 2 +- 3 files changed, 29 insertions(+), 55 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 131fbc5e1c62..e403e119f357 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -40,7 +40,7 @@ function getFailingRules(fontSizeArtifact) { node, cssRule, fontSize, - textLength + textLength, }); } else { failingRule.textLength += textLength; @@ -50,53 +50,30 @@ function getFailingRules(fontSizeArtifact) { return failingRules.valuesArray(); } -function nodeToString(node) { - const attributesString = node.attributes.map((value, idx) => { - if(idx % 2 === 0) { - return ` ${value}`; - } - - return value ? `='${value}'` : ''; - }).join(''); - - return `<${node.localName}${attributesString}>`; -} - /** * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets - * @param {Node} node * @param {string} baseURL * @param {WebInspector.CSSStyleDeclaration} styleDeclaration - * @returns string + * @returns {{source:!string, selector:string}} */ -function getOrigin(stylesheets, node, baseURL, styleDeclaration) { +function getOrigin(stylesheets, baseURL, styleDeclaration) { if (!styleDeclaration) { return { - details: 'User Agent Stylesheet' + source: 'User Agent Stylesheet', }; } const range = styleDeclaration && styleDeclaration.range; - let file = baseURL; - let location = ''; + let source = baseURL; if(range) { - location = `${range.startLine}:${range.startColumn}`; - } - - if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes) { - return { - details: `Attributes Style: ${nodeToString(node)}`, - file, - location - }; + source += `:${range.startLine}:${range.startColumn}`; } - if (styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { + if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || + styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { return { - details: `Inline Style: ${nodeToString(node)}`, - file, - location + source, }; } @@ -107,18 +84,17 @@ function getOrigin(stylesheets, node, baseURL, styleDeclaration) { if (stylesheetMeta) { const url = new URL(stylesheetMeta.header.sourceURL, baseURL); - file = `${url.href}`; + source = `${url.href}`; } return { - details: `CSS Selector: ${selector}`, - file, - location + selector, + source, }; } return { - details: 'Unknown' + source: 'Unknown', }; } @@ -150,7 +126,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize', 'Styles', 'URL'] + requiredArtifacts: ['FontSize', 'Styles', 'URL'], }; } @@ -182,23 +158,21 @@ class FontSize extends Audit { const pageUrl = artifacts.URL.finalUrl; const headings = [ - {key: 'file', itemType: 'url', text: 'File'}, - {key: 'location', itemType: 'url', text: 'Line and Column'}, - {key: 'details', itemType: 'code', text: 'Details'}, - {key: 'percentage', itemType: 'text', text: '% of Text'}, - {key: 'fontSize', itemType: 'text', text: 'Font Size'} + {key: 'source', itemType: 'code', text: 'Source'}, + {key: 'selector', itemType: 'code', text: 'Selector'}, + {key: 'coverage', itemType: 'text', text: 'Coverage'}, + {key: 'fontSize', itemType: 'text', text: 'Font Size'}, ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) - .map(({cssRule, node, textLength, fontSize}) => { + .map(({cssRule, textLength, fontSize}) => { const percentageOfAffectedText = textLength / totalTextLenght * 100; - const origin = getOrigin(artifacts.Styles, node, pageUrl, cssRule); + const origin = getOrigin(artifacts.Styles, pageUrl, cssRule); return { - file: origin.file, - location: origin.location, - details: origin.details, - percentage: `${percentageOfAffectedText.toFixed(2)}%`, + source: origin.source, + selector: origin.selector, + coverage: `${percentageOfAffectedText.toFixed(2)}%`, fontSize: `${fontSize}px`, }; }); @@ -210,7 +184,7 @@ class FontSize extends Audit { details, debugString: passed ? `${percentageOfPassingText.toFixed(2)}% of text is legible.` : - `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.` + `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, }; } } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index c7cd01ec3404..09232a18ca34 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -42,10 +42,10 @@ module.exports = { name: 'SEO', description: 'These ensure your app is able to be understood by search engine crawlers.', audits: [ - {id: 'viewport', weight: 1, group: 'seo-mobile'}, - {id: 'document-title', weight: 1, group: 'seo-content'}, - {id: 'meta-description', weight: 1, group: 'seo-content'}, - {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, + // {id: 'viewport', weight: 1, group: 'seo-mobile'}, + // {id: 'document-title', weight: 1, group: 'seo-content'}, + // {id: 'meta-description', weight: 1, group: 'seo-content'}, + // {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, {id: 'font-size', weight: 1, group: 'seo-mobile'}, ], }, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 3abbb2152707..8ae89d8c4db1 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -79,7 +79,7 @@ function getEffectiveRule(property, node, { inlineStyle, attributesStyle, matchedCSSRules, - inherited + inherited, }) { const cssModel = { styleSheetHeaderForId: id => ({id}), From a4ec14efd3cf54af868e2697e77e17883ce2ed29 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 10:03:34 +0200 Subject: [PATCH 12/65] Changes to the result table. Clean up. --- lighthouse-core/audits/seo/font-size.js | 72 ++++++++++++------- .../gather/gatherers/seo/font-size.js | 9 ++- lighthouse-core/lib/web-inspector.js | 1 + 3 files changed, 51 insertions(+), 31 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index e403e119f357..9f0b50f60294 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -50,47 +50,67 @@ function getFailingRules(fontSizeArtifact) { return failingRules.valuesArray(); } +/** + * @param {Node} node + * @return {{type:string, snippet:string}} + */ +function nodeToTableNode(node) { + const attributesString = node.attributes.map((value, idx) => + (idx % 2 === 0) ? ` ${value}` : `="${value}"` + ).join(''); + + return { + type: 'node', + snippet: `<${node.localName}${attributesString}>`, + }; +} + /** * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets * @param {string} baseURL * @param {WebInspector.CSSStyleDeclaration} styleDeclaration + * @param {Node} node * @returns {{source:!string, selector:string}} */ -function getOrigin(stylesheets, baseURL, styleDeclaration) { - if (!styleDeclaration) { +function getOrigin(stylesheets, baseURL, styleDeclaration, node) { + if (styleDeclaration.parentRule && + styleDeclaration.parentRule.origin === CSSAgent.StyleSheetOrigin.USER_AGENT) { return { + selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '), source: 'User Agent Stylesheet', }; } - const range = styleDeclaration && styleDeclaration.range; - let source = baseURL; - - if(range) { - source += `:${range.startLine}:${range.startColumn}`; - } - if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { return { - source, + source: baseURL, + selector: nodeToTableNode(node), }; } if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular && styleDeclaration.parentRule) { const rule = styleDeclaration.parentRule; - const selector = rule.selectors.map(item => item.text).join(', '); const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); if (stylesheetMeta) { const url = new URL(stylesheetMeta.header.sourceURL, baseURL); - source = `${url.href}`; - } + const range = styleDeclaration.range; + const selector = rule.selectors.map(item => item.text).join(', '); + let source = `${url.href}`; - return { - selector, - source, - }; + if(range) { + const absoluteStartLine = range.startLine + stylesheetMeta.header.startLine + 1; + const absoluteStartColumn = range.startColumn + stylesheetMeta.header.startColumn + 1; + + source += `:${absoluteStartLine}:${absoluteStartColumn}`; + } + + return { + selector, + source, + }; + } } return { @@ -99,15 +119,15 @@ function getOrigin(stylesheets, baseURL, styleDeclaration) { } /** - * @param {WebInspector.CSSStyleDeclaration} rule + * @param {WebInspector.CSSStyleDeclaration} styleDeclaration * @param {Node} node * @return string */ -function getFontArtifactId(rule, node) { - if (!rule) { - return 'user-agent'; - } else if (rule.type === CSSStyleDeclaration.Type.Regular) { - return `${rule.styleSheetId}@${rule.range.startLine}:${rule.range.startColumn}`; +function getFontArtifactId(styleDeclaration, node) { + if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular) { + const startLine = styleDeclaration.range ? styleDeclaration.range.startLine : 0; + const startColumn = styleDeclaration.range ? styleDeclaration.range.startColumn : 0; + return `${styleDeclaration.styleSheetId}@${startLine}:${startColumn}`; } else { return `node_${node.nodeId}`; } @@ -158,16 +178,16 @@ class FontSize extends Audit { const pageUrl = artifacts.URL.finalUrl; const headings = [ - {key: 'source', itemType: 'code', text: 'Source'}, + {key: 'source', itemType: 'url', text: 'Source'}, {key: 'selector', itemType: 'code', text: 'Selector'}, {key: 'coverage', itemType: 'text', text: 'Coverage'}, {key: 'fontSize', itemType: 'text', text: 'Font Size'}, ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) - .map(({cssRule, textLength, fontSize}) => { + .map(({cssRule, textLength, fontSize, node}) => { const percentageOfAffectedText = textLength / totalTextLenght * 100; - const origin = getOrigin(artifacts.Styles, pageUrl, cssRule); + const origin = getOrigin(artifacts.Styles, pageUrl, cssRule, node); return { source: origin.source, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 8ae89d8c4db1..789fbed2c0cb 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -7,7 +7,6 @@ const CSSMatchedStyles = require('../../../lib/web-inspector').CSSMatchedStyles; const Gatherer = require('../gatherer'); -const TEXT_NODE = 3; const FONT_SIZE_PROPERTY_NAME = 'font-size'; /** @@ -110,8 +109,8 @@ function getEffectiveRule(property, node, { /** * @param {!Object} driver - * @param {!Node} node - * @returns {!{fontSize: number, textLength: number, cssRule: WebInspector.CSSStyleDeclaration}} + * @param {!Node} node text node + * @returns {!{fontSize: number, textLength: number, node: Node, cssRule: WebInspector.CSSStyleDeclaration}} */ function getFontSizeInformation(driver, node) { const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.parentId}); @@ -126,7 +125,7 @@ function getFontSizeInformation(driver, node) { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, node: node.parentNode, - cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node.parentNode, matchedRules) + cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node.parentNode, matchedRules), }; }); } @@ -136,7 +135,7 @@ function getFontSizeInformation(driver, node) { * @returns {boolean} */ function isNonEmptyTextNode(node) { - return node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0; + return node.nodeType === Node.TEXT_NODE && node.nodeValue.trim().length > 0; } class FontSize extends Gatherer { diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index 9fda42b66828..ff890d424ce2 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -26,6 +26,7 @@ module.exports = (function() { global.Node = { ELEMENT_NODE: 1, + TEXT_NODE: 3, }; global.CSSAgent = {}; From 7298ae64946e122abc13d979ab820386ee62ebfe Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 10:03:43 +0200 Subject: [PATCH 13/65] Gatherer test. --- .../gather/gatherers/seo/font-size-test.js | 75 +++++++++++++++++++ .../test/lib/web-inspector-test.js | 1 + 2 files changed, 76 insertions(+) create mode 100644 lighthouse-core/test/gather/gatherers/seo/font-size-test.js diff --git a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js new file mode 100644 index 000000000000..2250d689ca21 --- /dev/null +++ b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js @@ -0,0 +1,75 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/* eslint-env mocha */ + +const FontSizeGather = require('../../../../gather/gatherers/seo/font-size'); +const assert = require('assert'); +let fontSizeGather; +const body = { + nodeName: 'BODY', + children: [ + {nodeValue: ' text ', nodeType: Node.TEXT_NODE, parentId: 1}, + {nodeValue: ' text ', nodeType: Node.ELEMENT_NODE}, + {nodeValue: ' ', nodeType: Node.TEXT_NODE}, + {nodeValue: 'texttext', nodeType: Node.TEXT_NODE, parentId: 2}, + ], +}; +const dom = { + root: { + nodeName: 'HTML', + children: [body], + }, +}; + +describe('Font size gatherer', () => { + // Reset the Gatherer before each test. + beforeEach(() => { + fontSizeGather = new FontSizeGather(); + }); + + it('returns information about font size\'s used on page', () => { + return fontSizeGather.afterPass({ + driver: { + sendCommand(command, params) { + let result; + if (command === 'DOM.getDocument') { + result = dom; + } else if (command === 'CSS.getComputedStyleForNode') { + result = {computedStyle: [ + {name: 'font-size', value: params.nodeId === 1 ? 10 : 20}, + ]}; + } else if (command === 'CSS.getMatchedStylesForNode') { + result = { + inlineStyle: null, + attributesStyle: null, + matchedCSSRules: [], + inherited: [], + }; + } + + return Promise.resolve(result); + }, + }, + }).then(artifact => { + assert.deepEqual(artifact, [ + { + fontSize: 10, + textLength: 4, + cssRule: undefined, + node: body, + }, + { + fontSize: 20, + textLength: 8, + cssRule: undefined, + node: body, + }, + ]); + }); + }); +}); diff --git a/lighthouse-core/test/lib/web-inspector-test.js b/lighthouse-core/test/lib/web-inspector-test.js index 01cc059180fd..5308344bb6a1 100644 --- a/lighthouse-core/test/lib/web-inspector-test.js +++ b/lighthouse-core/test/lib/web-inspector-test.js @@ -19,6 +19,7 @@ describe('Web Inspector lib', function() { assert.ok(WebInspector.TimelineAggregator); assert.ok(WebInspector.NetworkManager); assert.ok(WebInspector.Color); + assert.ok(WebInspector.CSSMetadata); }); // Our implementation of using DevTools doesn't sandbox natives From acb9aa875c6506da30f6023fc1b806825a614718 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 22:54:28 +0200 Subject: [PATCH 14/65] Smoke tests. Fixing edgecase when size is inherited from attribute styles of a parent node. Fixing linting errors, improvic jsdoc. --- .../test/fixtures/seo/seo-tester.html | 14 +++++++++- .../test/smokehouse/seo/expectations.js | 8 ++++++ lighthouse-core/audits/seo/font-size.js | 27 ++++++++++--------- lighthouse-core/config/seo.js | 16 ++++++----- .../gather/gatherers/seo/font-size.js | 14 +++++----- .../gather/gatherers/seo/font-size-test.js | 8 +++--- 6 files changed, 56 insertions(+), 31 deletions(-) diff --git a/lighthouse-cli/test/fixtures/seo/seo-tester.html b/lighthouse-cli/test/fixtures/seo/seo-tester.html index 9b2d45754325..4ad7686b809f 100644 --- a/lighthouse-cli/test/fixtures/seo/seo-tester.html +++ b/lighthouse-cli/test/fixtures/seo/seo-tester.html @@ -11,8 +11,20 @@ + - crawlz me + +

123451234512345

+ + +

1

+ 2 + 34 +

5

diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 831cc6951362..cf8bd3593085 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -25,6 +25,10 @@ module.exports = [ 'http-status-code': { score: true, }, + 'font-size': { + rawValue: true, + debugString: '75% of text is legible.', + }, }, }, { @@ -49,6 +53,10 @@ module.exports = [ score: false, displayValue: '403', }, + 'font-size': { + rawValue: false, + debugString: 'Text is illegible because of a missing viewport config', + }, }, }, ]; diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 9f0b50f60294..0de23e6010e9 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -73,19 +73,22 @@ function nodeToTableNode(node) { * @returns {{source:!string, selector:string}} */ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { - if (styleDeclaration.parentRule && - styleDeclaration.parentRule.origin === CSSAgent.StyleSheetOrigin.USER_AGENT) { + if ( + !styleDeclaration || + styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || + styleDeclaration.type === CSSStyleDeclaration.Type.Inline + ) { return { - selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '), - source: 'User Agent Stylesheet', + source: baseURL, + selector: nodeToTableNode(node), }; } - if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || - styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { + if (styleDeclaration.parentRule && + styleDeclaration.parentRule.origin === global.CSSAgent.StyleSheetOrigin.USER_AGENT) { return { - source: baseURL, - selector: nodeToTableNode(node), + selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '), + source: 'User Agent Stylesheet', }; } @@ -124,7 +127,7 @@ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { * @return string */ function getFontArtifactId(styleDeclaration, node) { - if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular) { + if (styleDeclaration && styleDeclaration.type === CSSStyleDeclaration.Type.Regular) { const startLine = styleDeclaration.range ? styleDeclaration.range.startLine : 0; const startColumn = styleDeclaration.range ? styleDeclaration.range.startColumn : 0; return `${styleDeclaration.styleSheetId}@${startLine}:${startColumn}`; @@ -198,13 +201,13 @@ class FontSize extends Audit { }); const details = Audit.makeTableDetails(headings, tableData); const passed = percentageOfPassingText >= MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT; + const debugString = passed ? + null : `${parseFloat((100 - percentageOfPassingText).toFixed(2))}% of text is too small.`; return { rawValue: passed, details, - debugString: passed ? - `${percentageOfPassingText.toFixed(2)}% of text is legible.` : - `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, + debugString, }; } } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 09232a18ca34..099d9e6ed7d3 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -6,14 +6,16 @@ 'use strict'; module.exports = { - // extends: 'lighthouse:default', + extends: 'lighthouse:default', passes: [{ passName: 'defaultPass', gatherers: [ - 'viewport', 'seo/meta-description', + ], + }, { + passName: 'extraPass', + gatherers: [ 'seo/font-size', - 'url', 'styles', ], }], @@ -42,10 +44,10 @@ module.exports = { name: 'SEO', description: 'These ensure your app is able to be understood by search engine crawlers.', audits: [ - // {id: 'viewport', weight: 1, group: 'seo-mobile'}, - // {id: 'document-title', weight: 1, group: 'seo-content'}, - // {id: 'meta-description', weight: 1, group: 'seo-content'}, - // {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, + {id: 'viewport', weight: 1, group: 'seo-mobile'}, + {id: 'document-title', weight: 1, group: 'seo-content'}, + {id: 'meta-description', weight: 1, group: 'seo-content'}, + {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, {id: 'font-size', weight: 1, group: 'seo-mobile'}, ], }, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 789fbed2c0cb..d3b2486aaaf4 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -71,7 +71,7 @@ function getAllNodesFromBody(driver) { * * @param {!string} property CSS property name * @param {!Node} node - * @param {!Object} matched CSS rule + * @param {!Object} matched CSS rules * @returns {WebInspector.CSSStyleDeclaration} */ function getEffectiveRule(property, node, { @@ -135,14 +135,14 @@ function getFontSizeInformation(driver, node) { * @returns {boolean} */ function isNonEmptyTextNode(node) { - return node.nodeType === Node.TEXT_NODE && node.nodeValue.trim().length > 0; + return node.nodeType === global.Node.TEXT_NODE && node.nodeValue.trim().length > 0; } class FontSize extends Gatherer { /** * @param {{driver: !Object}} options Run options - * @return {!Promise>} The font-size value of the document body + * @return {!Promise>} font-size analysis */ afterPass(options) { const enableDOM = options.driver.sendCommand('DOM.enable'); @@ -155,10 +155,10 @@ class FontSize extends Gatherer { textNodes.map(node => getFontSizeInformation(options.driver, node)) )) .then(fontSizeInfo => { - options.driver.sendCommand('DOM.disable'); - options.driver.sendCommand('CSS.disable'); - - return fontSizeInfo; + return Promise.all([ + options.driver.sendCommand('DOM.disable'), + options.driver.sendCommand('CSS.disable'), + ]).then(_ => fontSizeInfo); }); } } diff --git a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js index 2250d689ca21..81ad822bd3b2 100644 --- a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js +++ b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js @@ -13,10 +13,10 @@ let fontSizeGather; const body = { nodeName: 'BODY', children: [ - {nodeValue: ' text ', nodeType: Node.TEXT_NODE, parentId: 1}, - {nodeValue: ' text ', nodeType: Node.ELEMENT_NODE}, - {nodeValue: ' ', nodeType: Node.TEXT_NODE}, - {nodeValue: 'texttext', nodeType: Node.TEXT_NODE, parentId: 2}, + {nodeValue: ' text ', nodeType: global.Node.TEXT_NODE, parentId: 1}, + {nodeValue: ' text ', nodeType: global.Node.ELEMENT_NODE}, + {nodeValue: ' ', nodeType: global.Node.TEXT_NODE}, + {nodeValue: 'texttext', nodeType: global.Node.TEXT_NODE, parentId: 2}, ], }; const dom = { From a61106e752e20cd8f1d2879edc60e717bc15e2f9 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 23:48:24 +0200 Subject: [PATCH 15/65] Add unit tests. --- lighthouse-core/audits/seo/font-size.js | 3 +- .../test/audits/seo/font-size-test.js | 111 ++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 lighthouse-core/test/audits/seo/font-size-test.js diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 0de23e6010e9..4414fb5aa9f0 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -149,7 +149,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize', 'Styles', 'URL'], + requiredArtifacts: ['FontSize', 'Styles', 'URL', 'Viewport'], }; } @@ -171,7 +171,6 @@ class FontSize extends Audit { if (totalTextLenght === 0) { return { rawValue: true, - debugString: 'Page contains no text', }; } diff --git a/lighthouse-core/test/audits/seo/font-size-test.js b/lighthouse-core/test/audits/seo/font-size-test.js new file mode 100644 index 000000000000..3c58ef20e906 --- /dev/null +++ b/lighthouse-core/test/audits/seo/font-size-test.js @@ -0,0 +1,111 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const FontSizeAudit = require('../../../audits/seo/font-size.js'); +const assert = require('assert'); +const CSSStyleDeclaration = require('../../../lib/web-inspector').CSSStyleDeclaration; + +const URL = 'https://example.com'; +const Styles = []; +const validViewport = 'width=device-width'; + +/* eslint-env mocha */ + +describe('SEO: Font size audit', () => { + it('fails when viewport is not set', () => { + const artifacts = { + URL, + Viewport: null, + Styles, + FontSize: [], + }; + + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, false); + assert.ok(auditResult.debugString.includes('missing viewport')); + }); + + it('fails when less than 75% of text is legible', () => { + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 1, fontSize: 15, node: {nodeId: 1, localName: 'p', attributes: []}}, + {textLength: 2, fontSize: 16, node: {nodeId: 2, localName: 'p', attributes: []}}, + ], + }; + + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, false); + assert.ok(auditResult.debugString.includes('33.33%')); + }); + + it('passes when there is no text', () => { + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 0}, + {textLength: 0}, + ], + }; + + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, true); + }); + + it('passes when more than 75% of text is legible', () => { + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 1, fontSize: 15, node: {nodeId: 1, localName: 'p', attributes: []}}, + {textLength: 2, fontSize: 16, node: {nodeId: 2, localName: 'p', attributes: []}}, + {textLength: 2, fontSize: 24, node: {nodeId: 3, localName: 'p', attributes: []}}, + ], + }; + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, true); + }); + + it('groups entries with same source, sorts them by coverage', () => { + const style1 = { + styleSheetId: 1, + type: CSSStyleDeclaration.Type.Regular, + range: { + startLine: 123, + startColumn: 10, + }, + }; + const style2 = { + styleSheetId: 1, + type: CSSStyleDeclaration.Type.Regular, + range: { + startLine: 0, + startColumn: 10, + }, + }; + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 3, fontSize: 15, node: {nodeId: 1}, cssRule: style1}, + {textLength: 2, fontSize: 14, node: {nodeId: 2}, cssRule: style2}, + {textLength: 2, fontSize: 14, node: {nodeId: 3}, cssRule: style2}, + ], + }; + const auditResult = FontSizeAudit.audit(artifacts); + + assert.equal(auditResult.rawValue, false); + assert.equal(auditResult.details.items.length, 2); + assert.equal(auditResult.details.items[0][2].text, '57.14%'); + }); +}); From eaf454b550684a670868fcf3832a0e9358fb3ab6 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 00:11:30 +0200 Subject: [PATCH 16/65] Remove info about color prop. --- lighthouse-core/lib/web-inspector.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index ff890d424ce2..2f40d1b0cc95 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -333,10 +333,6 @@ module.exports = (function() { name: 'font-size', inherited: true, }, - { - name: 'color', - inherited: true, - }, ]; return WebInspector; From 6d1b5182d3b375ddd79e9355d05a780f5a0a83ba Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 00:43:38 +0200 Subject: [PATCH 17/65] Remove debugMessage from passing test expectations as it doesn't exist. --- lighthouse-cli/test/smokehouse/seo/expectations.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index cf8bd3593085..5a30a34f1283 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -27,7 +27,6 @@ module.exports = [ }, 'font-size': { rawValue: true, - debugString: '75% of text is legible.', }, }, }, From e07a3f80379b0d23e6fd2eec184eef6af11710a0 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 11:05:11 +0200 Subject: [PATCH 18/65] Replace URL with parseURL --- lighthouse-core/audits/seo/font-size.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 4414fb5aa9f0..cbaa93094cda 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -5,7 +5,7 @@ */ 'use strict'; -const {URL} = require('url'); +const parseURL = require('url').parse; const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const CSSStyleDeclaration = require('../../lib/web-inspector').CSSStyleDeclaration; @@ -97,7 +97,7 @@ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); if (stylesheetMeta) { - const url = new URL(stylesheetMeta.header.sourceURL, baseURL); + const url = parseURL(stylesheetMeta.header.sourceURL, baseURL); const range = styleDeclaration.range; const selector = rule.selectors.map(item => item.text).join(', '); let source = `${url.href}`; From 242024d91eac85acf3342dde2376f3db96f6bec5 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 11:17:21 +0200 Subject: [PATCH 19/65] Make linter happy. --- lighthouse-core/audits/seo/font-size.js | 2 +- lighthouse-core/gather/gatherers/seo/font-size.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index cbaa93094cda..23629b15ce23 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -102,7 +102,7 @@ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { const selector = rule.selectors.map(item => item.text).join(', '); let source = `${url.href}`; - if(range) { + if (range) { const absoluteStartLine = range.startLine + stylesheetMeta.header.startLine + 1; const absoluteStartColumn = range.startColumn + stylesheetMeta.header.startColumn + 1; diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index d3b2486aaaf4..4a452cb8843e 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -139,7 +139,6 @@ function isNonEmptyTextNode(node) { } class FontSize extends Gatherer { - /** * @param {{driver: !Object}} options Run options * @return {!Promise>} font-size analysis From 3c660334c0982875d5975e05abc64b5540f6b7cf Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Mon, 16 Oct 2017 00:01:46 +0200 Subject: [PATCH 20/65] Check number of failing items in the smoke test. --- lighthouse-cli/test/smokehouse/seo/expectations.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 5a30a34f1283..bf13afd9b3b5 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -27,6 +27,11 @@ module.exports = [ }, 'font-size': { rawValue: true, + details: { + items: { + length: 5, + }, + }, }, }, }, From 99d89ba84f3b4772b165e5b1d9511d00752f8134 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Mon, 16 Oct 2017 00:07:51 +0200 Subject: [PATCH 21/65] getOrigin -> findStyleRuleSource s/Lenght/Length --- lighthouse-core/audits/seo/font-size.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 23629b15ce23..67a9e9f6c1b3 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -72,7 +72,7 @@ function nodeToTableNode(node) { * @param {Node} node * @returns {{source:!string, selector:string}} */ -function getOrigin(stylesheets, baseURL, styleDeclaration, node) { +function findStyleRuleSource(stylesheets, baseURL, styleDeclaration, node) { if ( !styleDeclaration || styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || @@ -166,9 +166,9 @@ class FontSize extends Audit { }; } - const totalTextLenght = getTotalTextLength(artifacts.FontSize); + const totalTextLength = getTotalTextLength(artifacts.FontSize); - if (totalTextLenght === 0) { + if (totalTextLength === 0) { return { rawValue: true, }; @@ -176,7 +176,7 @@ class FontSize extends Audit { const failingRules = getFailingRules(artifacts.FontSize); const failingTextLength = getTotalTextLength(failingRules); - const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; + const percentageOfPassingText = (totalTextLength - failingTextLength) / totalTextLength * 100; const pageUrl = artifacts.URL.finalUrl; const headings = [ @@ -188,8 +188,8 @@ class FontSize extends Audit { const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) .map(({cssRule, textLength, fontSize, node}) => { - const percentageOfAffectedText = textLength / totalTextLenght * 100; - const origin = getOrigin(artifacts.Styles, pageUrl, cssRule, node); + const percentageOfAffectedText = textLength / totalTextLength * 100; + const origin = findStyleRuleSource(artifacts.Styles, pageUrl, cssRule, node); return { source: origin.source, From 018e2bf4f69b7951423b0cabe765af71324a2195 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Mon, 16 Oct 2017 01:26:36 +0200 Subject: [PATCH 22/65] Collect stylesheet metadata in font-size gatherer. Get rid of separate run. --- lighthouse-core/audits/seo/font-size.js | 17 ++++++++--------- lighthouse-core/config/seo.js | 5 ----- .../gather/gatherers/seo/font-size.js | 10 ++++++++++ .../test/gather/gatherers/seo/font-size-test.js | 2 ++ 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 67a9e9f6c1b3..a0a239b636a8 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -66,13 +66,12 @@ function nodeToTableNode(node) { } /** - * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets * @param {string} baseURL * @param {WebInspector.CSSStyleDeclaration} styleDeclaration * @param {Node} node * @returns {{source:!string, selector:string}} */ -function findStyleRuleSource(stylesheets, baseURL, styleDeclaration, node) { +function findStyleRuleSource(baseURL, styleDeclaration, node) { if ( !styleDeclaration || styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || @@ -94,17 +93,17 @@ function findStyleRuleSource(stylesheets, baseURL, styleDeclaration, node) { if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular && styleDeclaration.parentRule) { const rule = styleDeclaration.parentRule; - const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); + const stylesheet = styleDeclaration.stylesheet; - if (stylesheetMeta) { - const url = parseURL(stylesheetMeta.header.sourceURL, baseURL); + if (stylesheet) { + const url = parseURL(stylesheet.sourceURL, baseURL); const range = styleDeclaration.range; const selector = rule.selectors.map(item => item.text).join(', '); let source = `${url.href}`; if (range) { - const absoluteStartLine = range.startLine + stylesheetMeta.header.startLine + 1; - const absoluteStartColumn = range.startColumn + stylesheetMeta.header.startColumn + 1; + const absoluteStartLine = range.startLine + stylesheet.startLine + 1; + const absoluteStartColumn = range.startColumn + stylesheet.startColumn + 1; source += `:${absoluteStartLine}:${absoluteStartColumn}`; } @@ -149,7 +148,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize', 'Styles', 'URL', 'Viewport'], + requiredArtifacts: ['FontSize', 'URL', 'Viewport'], }; } @@ -189,7 +188,7 @@ class FontSize extends Audit { const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) .map(({cssRule, textLength, fontSize, node}) => { const percentageOfAffectedText = textLength / totalTextLength * 100; - const origin = findStyleRuleSource(artifacts.Styles, pageUrl, cssRule, node); + const origin = findStyleRuleSource(pageUrl, cssRule, node); return { source: origin.source, diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 099d9e6ed7d3..ad4cae7e158b 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -11,12 +11,7 @@ module.exports = { passName: 'defaultPass', gatherers: [ 'seo/meta-description', - ], - }, { - passName: 'extraPass', - gatherers: [ 'seo/font-size', - 'styles', ], }], audits: [ diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 4a452cb8843e..a9cdc7f8ca78 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -144,6 +144,10 @@ class FontSize extends Gatherer { * @return {!Promise>} font-size analysis */ afterPass(options) { + const stylesheets = new Map(); + const onStylesheetAdd = sheet => stylesheets.set(sheet.header.styleSheetId, sheet.header); + options.driver.on('CSS.styleSheetAdded', onStylesheetAdd); + const enableDOM = options.driver.sendCommand('DOM.enable'); const enableCSS = options.driver.sendCommand('CSS.enable'); @@ -154,6 +158,12 @@ class FontSize extends Gatherer { textNodes.map(node => getFontSizeInformation(options.driver, node)) )) .then(fontSizeInfo => { + options.driver.off('CSS.styleSheetAdded', onStylesheetAdd); + + fontSizeInfo + .filter(info => info.cssRule && info.cssRule.styleSheetId) + .forEach(info => info.cssRule.stylesheet = stylesheets.get(info.cssRule.styleSheetId)); + return Promise.all([ options.driver.sendCommand('DOM.disable'), options.driver.sendCommand('CSS.disable'), diff --git a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js index 81ad822bd3b2..e4cc5f7722fb 100644 --- a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js +++ b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js @@ -35,6 +35,8 @@ describe('Font size gatherer', () => { it('returns information about font size\'s used on page', () => { return fontSizeGather.afterPass({ driver: { + on() {}, + off() {}, sendCommand(command, params) { let result; if (command === 'DOM.getDocument') { From 4e1f171c840735a267bba0cc826e3e23e10c08c8 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 14 Sep 2017 00:29:22 +0200 Subject: [PATCH 23/65] Font Size audit - WIP --- lighthouse-core/audits/seo/font-size.js | 57 +++++++++++++++++++ .../gather/gatherers/seo/font-size.js | 24 ++++++++ 2 files changed, 81 insertions(+) create mode 100644 lighthouse-core/audits/seo/font-size.js create mode 100644 lighthouse-core/gather/gatherers/seo/font-size.js diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js new file mode 100644 index 000000000000..1a8b25c7fdd1 --- /dev/null +++ b/lighthouse-core/audits/seo/font-size.js @@ -0,0 +1,57 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const Audit = require('../audit'); +const ViewportAudit = require('../viewport'); +const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; + +class FontSize extends Audit { + /** + * @return {!AuditMeta} + */ + static get meta() { + return { + category: 'Mobile friendly', + name: 'font-size', + description: 'Document uses legible font sizes.', + failureDescription: 'Document doesn\'t use legible font sizes: ' + + '`target font-size >= 16px`, found', + helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + + 'visitors to “pinch to zoom” in order to read. ' + + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', + requiredArtifacts: ['FontSize'] + }; + } + + /** + * @param {!Artifacts} artifacts + * @return {!AuditResult} + */ + static audit(artifacts) { + const hasViewportSet = ViewportAudit.audit(artifacts).rawValue; + if (!hasViewportSet) { + return { + rawValue: false, + debugString: 'Text is illegible because of a missing viewport config', + }; + } + + const fontSizePx = artifacts.FontSize && parseInt(artifacts.FontSize, 10); + if (fontSizePx && fontSizePx < MINIMAL_LEGIBLE_FONT_SIZE_PX) { + return { + rawValue: false, + displayValue: `${fontSizePx}px`, + }; + } + + return { + rawValue: true, + }; + } +} + +module.exports = FontSize; diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js new file mode 100644 index 000000000000..250787a53800 --- /dev/null +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -0,0 +1,24 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const Gatherer = require('../gatherer'); + +class FontSize extends Gatherer { + + /** + * @param {{driver: !Object}} options Run options + * @return {!Promise} The font-size value of the document body + */ + afterPass(options) { + const driver = options.driver; + + return driver.evaluateAsync('getComputedStyle(document.body).fontSize'); + } +} + +module.exports = FontSize; + From dc13c146c9b581a06f8db10ed9ed3885b48b39fc Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Sat, 16 Sep 2017 18:18:18 +0200 Subject: [PATCH 24/65] Calculate percentage of ilegible text on page. Show failing elements in a table. --- lighthouse-core/audits/seo/font-size.js | 48 +++++++++++++++++-- lighthouse-core/config/seo.js | 6 ++- .../gather/gatherers/seo/font-size.js | 29 +++++++++-- 3 files changed, 76 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 1a8b25c7fdd1..6862c1ef7086 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -8,6 +8,7 @@ const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; +const MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT = 75; class FontSize extends Audit { /** @@ -40,16 +41,57 @@ class FontSize extends Audit { }; } - const fontSizePx = artifacts.FontSize && parseInt(artifacts.FontSize, 10); - if (fontSizePx && fontSizePx < MINIMAL_LEGIBLE_FONT_SIZE_PX) { + let passingText = 0; + let failingText = 0; + const failingElements = []; + + for(const key in artifacts.FontSize) { + if (!artifacts.FontSize.hasOwnProperty(key)) { + continue; + } + + const result = artifacts.FontSize[key]; + + if (result.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + passingText += result.textLength; + } else { + failingText += result.textLength; + result.elements.forEach(element => { + failingElements.push({element, fontSize: result.fontSize}); + }); + } + } + + if (passingText === 0 && failingText === 0) { + return { + rawValue: true, + debugString: 'Page contains no text', + }; + } + + const percentageOfPassingText = passingText / (passingText + failingText) * 100; + + if (percentageOfPassingText < MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT) { + const headings = [ + {key: 'element', itemType: 'text', text: 'Element'}, + {key: 'fontSize', itemType: 'text', text: 'Font Size'} + ]; + + const details = Audit.makeTableDetails(headings, failingElements); + return { rawValue: false, - displayValue: `${fontSizePx}px`, + extendedInfo: { + value: failingElements + }, + details, + debugString: `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, }; } return { rawValue: true, + debugString: `${percentageOfPassingText.toFixed(2)}% of text is legible` }; } } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 3f9e18f078c3..09eafe8b0540 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -6,16 +6,19 @@ 'use strict'; module.exports = { - extends: 'lighthouse:default', + // extends: 'lighthouse:default', passes: [{ passName: 'defaultPass', gatherers: [ + 'viewport', 'seo/meta-description', + 'seo/font-size', ], }], audits: [ 'seo/meta-description', 'seo/http-status-code', + 'seo/font-size', ], groups: { 'seo-mobile': { @@ -41,6 +44,7 @@ module.exports = { {id: 'document-title', weight: 1, group: 'seo-content'}, {id: 'meta-description', weight: 1, group: 'seo-content'}, {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, + {id: 'font-size', weight: 1, group: 'seo-mobile'}, ], }, }, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 250787a53800..a4ba8ce693ba 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -6,17 +6,40 @@ 'use strict'; const Gatherer = require('../gatherer'); +const DOMHelpers = require('../../../lib/dom-helpers.js'); class FontSize extends Gatherer { /** * @param {{driver: !Object}} options Run options - * @return {!Promise} The font-size value of the document body + * @return {!Promise>} The font-size value of the document body */ afterPass(options) { - const driver = options.driver; + const expression = `(function() { + ${DOMHelpers.getElementsInDocumentFnString}; // define function on page + const elements = getElementsInDocument('body *'); - return driver.evaluateAsync('getComputedStyle(document.body).fontSize'); + return elements.reduce((result, element) => { + const textLength = Array.from(element.childNodes) + .filter(node => node.nodeType === Node.TEXT_NODE) + .reduce((sum, textNode) => sum + textNode.nodeValue.trim().length, 0); + + if(textLength) { + const fontSize = parseInt(getComputedStyle(element)["font-size"], 10); + + if (!result[fontSize]) { + result[fontSize] = {fontSize, textLength: 0, elements: []}; + } + + result[fontSize].textLength += textLength; + result[fontSize].elements.push(element.tagName); + } + + return result; + }, {}); + })()`; + + return options.driver.evaluateAsync(expression); } } From 66d4b73104c7fd35f9281792d089da23b606c3e5 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Sun, 17 Sep 2017 21:51:13 +0200 Subject: [PATCH 25/65] Getting font-size information togheter with associated CSS rules - WIP --- .../gather/gatherers/seo/font-size.js | 77 +++++++++++++------ 1 file changed, 52 insertions(+), 25 deletions(-) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index a4ba8ce693ba..1eead5fd9304 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -6,7 +6,53 @@ 'use strict'; const Gatherer = require('../gatherer'); -const DOMHelpers = require('../../../lib/dom-helpers.js'); + +function findBody(node) { + const queue = [node]; + + while(queue.length > 0) { + const currentNode = queue.shift(); + + if(currentNode.nodeName === 'BODY') { + return currentNode; + } + + if(currentNode.children) { + currentNode.children.forEach(child => queue.push(child)); + } + } + + return null; +} + +function flattenTree(node) { + const flat = [node]; + const queue = [node]; + + while(queue.length > 0) { + const currentNode = queue.shift(); + + if(currentNode.children) { + currentNode.children.forEach(child => { + flat.push(child); + queue.push(child); + }); + } + } + + return flat; +} + +function getAllNodesFromBody(driver) { + return driver.sendCommand('DOM.enable') + .then(_ => driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true})) + .then(result => { + driver.sendCommand('DOM.disable'); + + const body = findBody(result.root); + return body ? flattenTree(body) : []; + }); +} class FontSize extends Gatherer { @@ -15,31 +61,12 @@ class FontSize extends Gatherer { * @return {!Promise>} The font-size value of the document body */ afterPass(options) { - const expression = `(function() { - ${DOMHelpers.getElementsInDocumentFnString}; // define function on page - const elements = getElementsInDocument('body *'); - - return elements.reduce((result, element) => { - const textLength = Array.from(element.childNodes) - .filter(node => node.nodeType === Node.TEXT_NODE) - .reduce((sum, textNode) => sum + textNode.nodeValue.trim().length, 0); - - if(textLength) { - const fontSize = parseInt(getComputedStyle(element)["font-size"], 10); - - if (!result[fontSize]) { - result[fontSize] = {fontSize, textLength: 0, elements: []}; - } - - result[fontSize].textLength += textLength; - result[fontSize].elements.push(element.tagName); - } - - return result; - }, {}); - })()`; + return getAllNodesFromBody(options.driver).then(result => { + console.log('result', result.length); - return options.driver.evaluateAsync(expression); + //TODO + // driver.sendCommand('CSS.enable') + }); } } From 507c78dc336c012a59fcc7157defe31ef08b6236 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 19 Sep 2017 01:30:09 +0200 Subject: [PATCH 26/65] Matched rules - WIP --- .../gather/gatherers/seo/font-size.js | 55 +++++++++++++++---- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 1eead5fd9304..56edbd5fb9a8 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -6,6 +6,7 @@ 'use strict'; const Gatherer = require('../gatherer'); +const TEXT_NODE = 3; function findBody(node) { const queue = [node]; @@ -27,15 +28,14 @@ function findBody(node) { function flattenTree(node) { const flat = [node]; - const queue = [node]; - while(queue.length > 0) { - const currentNode = queue.shift(); + for (let i = 0; i < flat.length; i++) { + const currentNode = flat[i]; if(currentNode.children) { currentNode.children.forEach(child => { + child.parentNode = currentNode; flat.push(child); - queue.push(child); }); } } @@ -44,16 +44,37 @@ function flattenTree(node) { } function getAllNodesFromBody(driver) { - return driver.sendCommand('DOM.enable') - .then(_ => driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true})) + return driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}) .then(result => { - driver.sendCommand('DOM.disable'); - const body = findBody(result.root); return body ? flattenTree(body) : []; }); } +function getFontSizeRule({inlineStyle, matchedCSSRules, inherited}) { + + // TODO https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMatchedStyles.js?type=cs&q=SDK.CSSMatchedS&sq=package:chromium&l=1 + + return {}; +} + +function getFontSizeInformation(driver, node) { + const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.nodeId}); + const matchedRules = driver.sendCommand('CSS.getMatchedStylesForNode', {nodeId: node.parentId}); + + return Promise.all([computedStyles, matchedRules]) + .then(result => { + const [{computedStyle}, matchedRules] = result; + const fontSizeProperty = computedStyle.find(({name}) => name === 'font-size'); + + return { + fontSize: parseInt(fontSizeProperty.value, 10), + textLength: node.nodeValue.trim().length, + cssRule: getFontSizeRule(matchedRules) + }; + }); +} + class FontSize extends Gatherer { /** @@ -61,11 +82,21 @@ class FontSize extends Gatherer { * @return {!Promise>} The font-size value of the document body */ afterPass(options) { - return getAllNodesFromBody(options.driver).then(result => { - console.log('result', result.length); + const enableDOM = options.driver.sendCommand('DOM.enable'); + const enableCSS = options.driver.sendCommand('CSS.enable'); + + return Promise.all([enableDOM, enableCSS]) + .then(() => getAllNodesFromBody(options.driver)) + .then(nodes => nodes.filter(node => node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0)) + .then(textNodes => Promise.all(textNodes.map(node => getFontSizeInformation(options.driver, node)))) + .then(fontSizeInfo => { + console.log(fontSizeInfo); + const result = {}; + + options.driver.sendCommand('DOM.disable'); + options.driver.sendCommand('CSS.disable'); - //TODO - // driver.sendCommand('CSS.enable') + return result; }); } } From 045fc6a67e6e46df97058fd3ee52ae97952f9e3c Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 20 Sep 2017 22:50:34 +0200 Subject: [PATCH 27/65] Extracting style info - WIP --- lighthouse-core/gather/gatherers/seo/font-size.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 56edbd5fb9a8..7c2a35ee195e 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -55,6 +55,12 @@ function getFontSizeRule({inlineStyle, matchedCSSRules, inherited}) { // TODO https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMatchedStyles.js?type=cs&q=SDK.CSSMatchedS&sq=package:chromium&l=1 + console.log(JSON.stringify(inlineStyle)); + console.log('----'); + console.log(JSON.stringify(matchedCSSRules)); + console.log('----'); + console.log(JSON.stringify(inherited)); + return {}; } From 696f926c08bb03a78a4e0d6ea83f4be8723a8cb9 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 26 Sep 2017 17:24:57 +0200 Subject: [PATCH 28/65] Expose effective rule --- lighthouse-core/audits/seo/font-size.js | 41 +++++++++++-------- .../gather/gatherers/seo/font-size.js | 35 +++++++++------- lighthouse-core/lib/web-inspector.js | 28 +++++++++++++ 3 files changed, 74 insertions(+), 30 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 6862c1ef7086..78584e976122 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -43,24 +43,26 @@ class FontSize extends Audit { let passingText = 0; let failingText = 0; - const failingElements = []; + const failingRules = new Map(); - for(const key in artifacts.FontSize) { - if (!artifacts.FontSize.hasOwnProperty(key)) { - continue; - } + artifacts.FontSize.forEach(item => { + if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + passingText += item.textLength; + } else { + failingText += item.textLength; - const result = artifacts.FontSize[key]; + const ruleKey = `${item.cssRule.styleSheetId}@${item.cssRule.range.startLine}:${item.cssRule.range.startColumn}`; - if (result.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { - passingText += result.textLength; - } else { - failingText += result.textLength; - result.elements.forEach(element => { - failingElements.push({element, fontSize: result.fontSize}); - }); + if(!failingRules.has(ruleKey)) { + failingRules.set(ruleKey, { + styleSheetId: item.cssRule.styleSheetId, + range: item.cssRule.range, + selectors: item.cssRule.parentRule.selectors, + fontSize: item.fontSize + }); + } } - } + }); if (passingText === 0 && failingText === 0) { return { @@ -77,12 +79,19 @@ class FontSize extends Audit { {key: 'fontSize', itemType: 'text', text: 'Font Size'} ]; - const details = Audit.makeTableDetails(headings, failingElements); + const details = Audit.makeTableDetails(headings, Array.from(failingRules).map(rule => { + const value = rule[1]; + + return { + element: value.selectors.map(item => item.text).join(', '), + fontSize: `${value.fontSize}px` + }; + })); return { rawValue: false, extendedInfo: { - value: failingElements + value: failingRules }, details, debugString: `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 7c2a35ee195e..ec2e79dc9746 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -5,8 +5,10 @@ */ 'use strict'; +const CSSMatchedStyles = require('../../../lib/web-inspector').CSSMatchedStyles; const Gatherer = require('../gatherer'); const TEXT_NODE = 3; +const FONT_SIZE_PROPERTY_NAME = 'font-size'; function findBody(node) { const queue = [node]; @@ -51,17 +53,25 @@ function getAllNodesFromBody(driver) { }); } -function getFontSizeRule({inlineStyle, matchedCSSRules, inherited}) { +function getFontSizeRule(node, {inlineStyle, matchedCSSRules, inherited}) { + const cssModel = { + styleSheetHeaderForId: id => { + return {id: id}; + } + }; - // TODO https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMatchedStyles.js?type=cs&q=SDK.CSSMatchedS&sq=package:chromium&l=1 + const nodeType = node.nodeType; + node.nodeType = () => nodeType; + const matchedStyles = new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); - console.log(JSON.stringify(inlineStyle)); - console.log('----'); - console.log(JSON.stringify(matchedCSSRules)); - console.log('----'); - console.log(JSON.stringify(inherited)); + const nodeStyles = matchedStyles.nodeStyles(); + const matchingRule = nodeStyles.find(style => { + const property = style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); + return property && + matchedStyles.propertyState(property) !== CSSMatchedStyles.PropertyState.Overloaded; + }); - return {}; + return matchingRule; } function getFontSizeInformation(driver, node) { @@ -71,12 +81,12 @@ function getFontSizeInformation(driver, node) { return Promise.all([computedStyles, matchedRules]) .then(result => { const [{computedStyle}, matchedRules] = result; - const fontSizeProperty = computedStyle.find(({name}) => name === 'font-size'); + const fontSizeProperty = computedStyle.find(({name}) => name === FONT_SIZE_PROPERTY_NAME); return { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, - cssRule: getFontSizeRule(matchedRules) + cssRule: getFontSizeRule(node, matchedRules) }; }); } @@ -96,13 +106,10 @@ class FontSize extends Gatherer { .then(nodes => nodes.filter(node => node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0)) .then(textNodes => Promise.all(textNodes.map(node => getFontSizeInformation(options.driver, node)))) .then(fontSizeInfo => { - console.log(fontSizeInfo); - const result = {}; - options.driver.sendCommand('DOM.disable'); options.driver.sendCommand('CSS.disable'); - return result; + return fontSizeInfo; }); } } diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index c10c39a065cd..0ab45d6ffae1 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -24,6 +24,21 @@ module.exports = (function() { global.window = global; } + global.Node = { + ELEMENT_NODE: 1 + }; + + global.CSSAgent = {}; + global.CSSAgent.StyleSheetOrigin = { + INJECTED: 'injected', + USER_AGENT: 'user-agent', + INSPECTOR: 'inspector', + REGULAR: 'regular' + }; + + global.CSS = {}; + global.CSS.supports = () => true; + global.Runtime = global.Runtime || {}; // Required for devtools-timeline-model @@ -304,5 +319,18 @@ module.exports = (function() { return ast; }; + // Dependecies for efective CSS rule calculation. + require('chrome-devtools-frontend/front_end/sdk/CSSMatchedStyles.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSMedia.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSMetadata.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSProperty.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSRule.js'); + require('chrome-devtools-frontend/front_end/sdk/CSSStyleDeclaration.js'); + + WebInspector.CSSMetadata._generatedProperties = [ + {"name":"font-size","inherited":true}, + {"name":"color","inherited":true} + ]; + return WebInspector; })(); From 1da6cfec958109aed8fab84de893c779950f6735 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 28 Sep 2017 17:52:06 +0200 Subject: [PATCH 29/65] Show what percentage of text each rule affected. Show table with failing selectors even if test passed. Clean up. --- lighthouse-core/audits/seo/font-size.js | 126 +++++++++++------- .../gather/gatherers/seo/font-size.js | 81 ++++++++--- lighthouse-core/lib/web-inspector.js | 14 +- 3 files changed, 147 insertions(+), 74 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 78584e976122..34a2ccdf55d3 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -10,6 +10,40 @@ const ViewportAudit = require('../viewport'); const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; const MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT = 75; +function getTotalTextLength(rules) { + return rules.reduce((sum, item) => sum + item.textLength, 0); +} + +function getFailingRules(fontSizeArtifact) { + const failingRules = new Map(); + + fontSizeArtifact.forEach(item => { + if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + return; + } + + const cssRule = item.cssRule; + const ruleKey = + `${cssRule.styleSheetId}@${cssRule.range.startLine}:${cssRule.range.startColumn}`; + let failingRule = failingRules.get(ruleKey); + + if (!failingRule) { + failingRule = { + styleSheetId: cssRule.styleSheetId, + range: cssRule.range, + selectors: cssRule.parentRule.selectors, + fontSize: item.fontSize, + textLength: 0 + }; + failingRules.set(ruleKey, failingRule); + } + + failingRule.textLength += item.textLength; + }); + + return failingRules.valuesArray(); +} + class FontSize extends Audit { /** * @return {!AuditMeta} @@ -19,11 +53,10 @@ class FontSize extends Audit { category: 'Mobile friendly', name: 'font-size', description: 'Document uses legible font sizes.', - failureDescription: 'Document doesn\'t use legible font sizes: ' + - '`target font-size >= 16px`, found', + failureDescription: 'Document doesn\'t use legible font sizes.', helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + - 'visitors to “pinch to zoom” in order to read. ' + - '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', + 'visitors to “pinch to zoom” in order to read. ' + + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', requiredArtifacts: ['FontSize'] }; } @@ -41,66 +74,59 @@ class FontSize extends Audit { }; } - let passingText = 0; - let failingText = 0; - const failingRules = new Map(); - - artifacts.FontSize.forEach(item => { - if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { - passingText += item.textLength; - } else { - failingText += item.textLength; - - const ruleKey = `${item.cssRule.styleSheetId}@${item.cssRule.range.startLine}:${item.cssRule.range.startColumn}`; - - if(!failingRules.has(ruleKey)) { - failingRules.set(ruleKey, { - styleSheetId: item.cssRule.styleSheetId, - range: item.cssRule.range, - selectors: item.cssRule.parentRule.selectors, - fontSize: item.fontSize - }); - } - } - }); + const totalTextLenght = getTotalTextLength(artifacts.FontSize); - if (passingText === 0 && failingText === 0) { + if (totalTextLenght === 0) { return { rawValue: true, debugString: 'Page contains no text', }; } - const percentageOfPassingText = passingText / (passingText + failingText) * 100; + const failingRules = getFailingRules(artifacts.FontSize); + const failingTextLength = getTotalTextLength(failingRules); + const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; - if (percentageOfPassingText < MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT) { - const headings = [ - {key: 'element', itemType: 'text', text: 'Element'}, - {key: 'fontSize', itemType: 'text', text: 'Font Size'} - ]; + const headings = [ + { + key: 'selector', + itemType: 'text', + text: 'CSS selector', + }, + { + key: 'percentage', + itemType: 'text', + text: '% of text', + }, + { + key: 'fontSize', + itemType: 'text', + text: 'Font Size', + } + ]; - const details = Audit.makeTableDetails(headings, Array.from(failingRules).map(rule => { - const value = rule[1]; + const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) + .map(rule => { + const percentageOfAffectedText = rule.textLength / totalTextLenght * 100; return { - element: value.selectors.map(item => item.text).join(', '), - fontSize: `${value.fontSize}px` + selector: rule.selectors.map(item => item.text).join(', '), + percentage: `${percentageOfAffectedText.toFixed(2)}%`, + fontSize: `${rule.fontSize}px`, }; - })); - - return { - rawValue: false, - extendedInfo: { - value: failingRules - }, - details, - debugString: `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, - }; - } + }); + const details = Audit.makeTableDetails(headings, tableData); + const passed = percentageOfPassingText >= MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT; return { - rawValue: true, - debugString: `${percentageOfPassingText.toFixed(2)}% of text is legible` + rawValue: passed, + extendedInfo: { + value: failingRules + }, + details, + debugString: passed ? + `${percentageOfPassingText.toFixed(2)}% of text is legible` : + `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.` }; } } diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index ec2e79dc9746..a8dca8ceabc6 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -10,17 +10,21 @@ const Gatherer = require('../gatherer'); const TEXT_NODE = 3; const FONT_SIZE_PROPERTY_NAME = 'font-size'; +/** + * @param {!Node} node top document node + * @returns {Node} body node + */ function findBody(node) { const queue = [node]; - while(queue.length > 0) { + while (queue.length > 0) { const currentNode = queue.shift(); - if(currentNode.nodeName === 'BODY') { + if (currentNode.nodeName === 'BODY') { return currentNode; } - if(currentNode.children) { + if (currentNode.children) { currentNode.children.forEach(child => queue.push(child)); } } @@ -28,13 +32,17 @@ function findBody(node) { return null; } +/** + * @param {!Node} node + * @returns {!Array} + */ function flattenTree(node) { const flat = [node]; for (let i = 0; i < flat.length; i++) { const currentNode = flat[i]; - if(currentNode.children) { + if (currentNode.children) { currentNode.children.forEach(child => { child.parentNode = currentNode; flat.push(child); @@ -45,6 +53,12 @@ function flattenTree(node) { return flat; } +/** + * Get list of all nodes from the document body. + * + * @param {!Object} driver + * @returns {!Array} + */ function getAllNodesFromBody(driver) { return driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}) .then(result => { @@ -53,27 +67,43 @@ function getAllNodesFromBody(driver) { }); } -function getFontSizeRule(node, {inlineStyle, matchedCSSRules, inherited}) { +/** + * Returns effective CSS rule for the font-size property + * + * @param {!string} CSS property name + * @param {!Node} node + * @param {!Object} matched CSS rules + * @returns + */ +function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherited}) { const cssModel = { - styleSheetHeaderForId: id => { - return {id: id}; - } + styleSheetHeaderForId: id => ({id}), }; const nodeType = node.nodeType; node.nodeType = () => nodeType; - const matchedStyles = new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); + const matchedStyles = + new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); const nodeStyles = matchedStyles.nodeStyles(); const matchingRule = nodeStyles.find(style => { - const property = style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); + const property = + style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); return property && matchedStyles.propertyState(property) !== CSSMatchedStyles.PropertyState.Overloaded; }); + debugger; return matchingRule; } +/** + * + * + * @param {!Object} driver + * @param {!Node} node + * @returns {!{fontSize: number, textLength: number, cssRule: !Object}} + */ function getFontSizeInformation(driver, node) { const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.nodeId}); const matchedRules = driver.sendCommand('CSS.getMatchedStylesForNode', {nodeId: node.parentId}); @@ -86,11 +116,19 @@ function getFontSizeInformation(driver, node) { return { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, - cssRule: getFontSizeRule(node, matchedRules) + cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node, matchedRules) }; }); } +/** + * @param {Node} node + * @returns {boolean} + */ +function isNonEmptyTextNode(node) { + return node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0; +} + class FontSize extends Gatherer { /** @@ -102,15 +140,18 @@ class FontSize extends Gatherer { const enableCSS = options.driver.sendCommand('CSS.enable'); return Promise.all([enableDOM, enableCSS]) - .then(() => getAllNodesFromBody(options.driver)) - .then(nodes => nodes.filter(node => node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0)) - .then(textNodes => Promise.all(textNodes.map(node => getFontSizeInformation(options.driver, node)))) - .then(fontSizeInfo => { - options.driver.sendCommand('DOM.disable'); - options.driver.sendCommand('CSS.disable'); - - return fontSizeInfo; - }); + .then(() => getAllNodesFromBody(options.driver)) + .then(nodes => nodes.filter(isNonEmptyTextNode)) + .then(textNodes => Promise.all( + textNodes.map(node => getFontSizeInformation(options.driver, node)) + )) + .then(fontSizeInfo => { + options.driver.sendCommand('DOM.disable'); + options.driver.sendCommand('CSS.disable'); + + debugger; + return fontSizeInfo; + }); } } diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index 0ab45d6ffae1..9fda42b66828 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -25,7 +25,7 @@ module.exports = (function() { } global.Node = { - ELEMENT_NODE: 1 + ELEMENT_NODE: 1, }; global.CSSAgent = {}; @@ -33,7 +33,7 @@ module.exports = (function() { INJECTED: 'injected', USER_AGENT: 'user-agent', INSPECTOR: 'inspector', - REGULAR: 'regular' + REGULAR: 'regular', }; global.CSS = {}; @@ -328,8 +328,14 @@ module.exports = (function() { require('chrome-devtools-frontend/front_end/sdk/CSSStyleDeclaration.js'); WebInspector.CSSMetadata._generatedProperties = [ - {"name":"font-size","inherited":true}, - {"name":"color","inherited":true} + { + name: 'font-size', + inherited: true, + }, + { + name: 'color', + inherited: true, + }, ]; return WebInspector; From 9051b921af8e4d6a746261fda6e847fce822456d Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 3 Oct 2017 01:31:10 +0200 Subject: [PATCH 30/65] Work on inline styles, clean up jsdoc --- lighthouse-core/audits/seo/font-size.js | 88 +++++++++++++------ .../gather/gatherers/seo/font-size.js | 32 +++---- 2 files changed, 76 insertions(+), 44 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 34a2ccdf55d3..d527c17f5112 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -7,43 +7,82 @@ const Audit = require('../audit'); const ViewportAudit = require('../viewport'); +const CSSStyleDeclaration = require('../../lib/web-inspector').CSSStyleDeclaration; const MINIMAL_LEGIBLE_FONT_SIZE_PX = 16; const MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT = 75; +/** + * @param {!Array<{textLength:number}>} rules + * @returns number + */ function getTotalTextLength(rules) { return rules.reduce((sum, item) => sum + item.textLength, 0); } +/** + * @param {Array<{cssRule: WebInspector.CSSStyleDeclaration, fontSize: number, textLength: number, node: Node}>} fontSizeArtifact + * @returns {Array<{cssRule: WebInspector.CSSStyleDeclaration, fontSize: number, textLength: number, node: Node}>} + */ function getFailingRules(fontSizeArtifact) { const failingRules = new Map(); - fontSizeArtifact.forEach(item => { - if (item.fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { + fontSizeArtifact.forEach(({cssRule, fontSize, textLength, node}) => { + if (fontSize >= MINIMAL_LEGIBLE_FONT_SIZE_PX) { return; } - const cssRule = item.cssRule; - const ruleKey = - `${cssRule.styleSheetId}@${cssRule.range.startLine}:${cssRule.range.startColumn}`; - let failingRule = failingRules.get(ruleKey); + const artifactId = getFontArtifactId(cssRule, node); + const failingRule = failingRules.get(artifactId); if (!failingRule) { - failingRule = { - styleSheetId: cssRule.styleSheetId, - range: cssRule.range, - selectors: cssRule.parentRule.selectors, - fontSize: item.fontSize, - textLength: 0 - }; - failingRules.set(ruleKey, failingRule); + failingRules.set(artifactId, { + node, + cssRule, + fontSize, + textLength + }); + } else { + failingRule.textLength += textLength; } - - failingRule.textLength += item.textLength; }); return failingRules.valuesArray(); } +/** + * @param {WebInspector.CSSStyleDeclaration} rule + * @param {Node} node + * @returns string + */ +function ruleToSource(rule, node) { + if (rule.type === CSSStyleDeclaration.Type.Regular) { + return rule.selectors.map(item => item.text).join(', '); + } + + const attributes = node.attributes.map((item, idx) => { + if (idx % 2 === 0) { + return ` ${item}`; + } else { + return item ? `="${item}"` : ''; + } + }).join(''); + + return `<${node.localName}${attributes}>`; +} + +/** + * @param {WebInspector.CSSStyleDeclaration} rule + * @param {Node} node + * @return string + */ +function getFontArtifactId(rule, node) { + if (rule.type === CSSStyleDeclaration.Type.Regular) { + return `${rule.styleSheetId}@${rule.range.startLine}:${rule.range.startColumn}`; + } else { + return `node_${node.id}`; + } +} + class FontSize extends Audit { /** * @return {!AuditMeta} @@ -89,9 +128,9 @@ class FontSize extends Audit { const headings = [ { - key: 'selector', - itemType: 'text', - text: 'CSS selector', + key: 'source', + itemType: 'code', + text: 'Source', }, { key: 'percentage', @@ -106,13 +145,13 @@ class FontSize extends Audit { ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) - .map(rule => { - const percentageOfAffectedText = rule.textLength / totalTextLenght * 100; + .map(({cssRule, node, textLength, fontSize}) => { + const percentageOfAffectedText = textLength / totalTextLenght * 100; return { - selector: rule.selectors.map(item => item.text).join(', '), + source: ruleToSource(cssRule, node), percentage: `${percentageOfAffectedText.toFixed(2)}%`, - fontSize: `${rule.fontSize}px`, + fontSize: `${fontSize}px`, }; }); const details = Audit.makeTableDetails(headings, tableData); @@ -120,9 +159,6 @@ class FontSize extends Audit { return { rawValue: passed, - extendedInfo: { - value: failingRules - }, details, debugString: passed ? `${percentageOfPassingText.toFixed(2)}% of text is legible` : diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index a8dca8ceabc6..bdc4f36c7d24 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -11,8 +11,8 @@ const TEXT_NODE = 3; const FONT_SIZE_PROPERTY_NAME = 'font-size'; /** - * @param {!Node} node top document node - * @returns {Node} body node + * @param {!Node} node Top document node + * @returns {Node} */ function findBody(node) { const queue = [node]; @@ -68,12 +68,12 @@ function getAllNodesFromBody(driver) { } /** - * Returns effective CSS rule for the font-size property + * Returns effective CSS rule for given CSS property * - * @param {!string} CSS property name + * @param {!string} property CSS property name * @param {!Node} node - * @param {!Object} matched CSS rules - * @returns + * @param {!Object} matched CSS rule + * @returns {WebInspector.CSSStyleDeclaration} */ function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherited}) { const cssModel = { @@ -87,25 +87,21 @@ function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherit const nodeStyles = matchedStyles.nodeStyles(); const matchingRule = nodeStyles.find(style => { - const property = - style.allProperties.find(property => property.name === FONT_SIZE_PROPERTY_NAME); - return property && - matchedStyles.propertyState(property) !== CSSMatchedStyles.PropertyState.Overloaded; + const foundProperty = style.allProperties.find(item => item.name === property); + return foundProperty && + matchedStyles.propertyState(foundProperty) !== CSSMatchedStyles.PropertyState.Overloaded; }); - debugger; return matchingRule; } /** - * - * * @param {!Object} driver * @param {!Node} node - * @returns {!{fontSize: number, textLength: number, cssRule: !Object}} + * @returns {!{fontSize: number, textLength: number, cssRule: WebInspector.CSSStyleDeclaration}} */ function getFontSizeInformation(driver, node) { - const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.nodeId}); + const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.parentId}); const matchedRules = driver.sendCommand('CSS.getMatchedStylesForNode', {nodeId: node.parentId}); return Promise.all([computedStyles, matchedRules]) @@ -116,7 +112,8 @@ function getFontSizeInformation(driver, node) { return { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, - cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node, matchedRules) + node: node.parentNode, + cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node.parentNode, matchedRules) }; }); } @@ -133,7 +130,7 @@ class FontSize extends Gatherer { /** * @param {{driver: !Object}} options Run options - * @return {!Promise>} The font-size value of the document body + * @return {!Promise>} The font-size value of the document body */ afterPass(options) { const enableDOM = options.driver.sendCommand('DOM.enable'); @@ -149,7 +146,6 @@ class FontSize extends Gatherer { options.driver.sendCommand('DOM.disable'); options.driver.sendCommand('CSS.disable'); - debugger; return fontSizeInfo; }); } From 85966655641acf65a406bd37f56ec754ec74a703 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 3 Oct 2017 17:47:04 +0200 Subject: [PATCH 31/65] Handle inline styels, attribute styles and user agent styles. --- lighthouse-core/audits/seo/font-size.js | 57 +++++++++---------- .../gather/gatherers/seo/font-size.js | 19 ++++++- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index d527c17f5112..f77d9fbe13cd 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -49,25 +49,34 @@ function getFailingRules(fontSizeArtifact) { return failingRules.valuesArray(); } +function nodeToString(node) { + return node.localName; +} + /** * @param {WebInspector.CSSStyleDeclaration} rule * @param {Node} node * @returns string */ -function ruleToSource(rule, node) { - if (rule.type === CSSStyleDeclaration.Type.Regular) { - return rule.selectors.map(item => item.text).join(', '); +function getOriginDescription(rule, node) { + if (!rule) { + return 'User Agent Stylesheet'; } - const attributes = node.attributes.map((item, idx) => { - if (idx % 2 === 0) { - return ` ${item}`; - } else { - return item ? `="${item}"` : ''; - } - }).join(''); + if (rule.type === CSSStyleDeclaration.Type.Attributes) { + return `Attributes Style: ${nodeToString(node)}`; + } + + if (rule.type === CSSStyleDeclaration.Type.Inline) { + return `Inline Style: ${nodeToString(node)}`; + } + + if (rule.type === CSSStyleDeclaration.Type.Regular && rule.parentRule) { + const selector = rule.parentRule.selectors.map(item => item.text).join(', '); + return `CSS Selector: ${selector}`; + } - return `<${node.localName}${attributes}>`; + return 'unknown'; } /** @@ -76,10 +85,12 @@ function ruleToSource(rule, node) { * @return string */ function getFontArtifactId(rule, node) { - if (rule.type === CSSStyleDeclaration.Type.Regular) { + if (!rule) { + return 'user-agent'; + } else if (rule.type === CSSStyleDeclaration.Type.Regular) { return `${rule.styleSheetId}@${rule.range.startLine}:${rule.range.startColumn}`; } else { - return `node_${node.id}`; + return `node_${node.nodeId}`; } } @@ -127,21 +138,9 @@ class FontSize extends Audit { const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; const headings = [ - { - key: 'source', - itemType: 'code', - text: 'Source', - }, - { - key: 'percentage', - itemType: 'text', - text: '% of text', - }, - { - key: 'fontSize', - itemType: 'text', - text: 'Font Size', - } + {key: 'origin', itemType: 'code', text: 'Origin'}, + {key: 'percentage', itemType: 'text', text: '% of text'}, + {key: 'fontSize', itemType: 'text', text: 'Font Size'} ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) @@ -149,7 +148,7 @@ class FontSize extends Audit { const percentageOfAffectedText = textLength / totalTextLenght * 100; return { - source: ruleToSource(cssRule, node), + origin: getOriginDescription(cssRule, node), percentage: `${percentageOfAffectedText.toFixed(2)}%`, fontSize: `${fontSize}px`, }; diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index bdc4f36c7d24..3abbb2152707 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -75,15 +75,28 @@ function getAllNodesFromBody(driver) { * @param {!Object} matched CSS rule * @returns {WebInspector.CSSStyleDeclaration} */ -function getEffectiveRule(property, node, {inlineStyle, matchedCSSRules, inherited}) { +function getEffectiveRule(property, node, { + inlineStyle, + attributesStyle, + matchedCSSRules, + inherited +}) { const cssModel = { styleSheetHeaderForId: id => ({id}), }; const nodeType = node.nodeType; node.nodeType = () => nodeType; - const matchedStyles = - new CSSMatchedStyles(cssModel, node, inlineStyle, null, matchedCSSRules, null, inherited, null); + const matchedStyles = new CSSMatchedStyles( + cssModel, + node, + inlineStyle, + attributesStyle, + matchedCSSRules, + null, + inherited, + null + ); const nodeStyles = matchedStyles.nodeStyles(); const matchingRule = nodeStyles.find(style => { From 64374a0dfddfe81b182d56441b42caa01ea52b14 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 4 Oct 2017 16:33:43 +0200 Subject: [PATCH 32/65] Expose stylesheet URL, line number and column. --- lighthouse-core/audits/seo/font-size.js | 85 +++++++++++++++++++------ lighthouse-core/config/seo.js | 2 + 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index f77d9fbe13cd..131fbc5e1c62 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -5,6 +5,7 @@ */ 'use strict'; +const {URL} = require('url'); const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const CSSStyleDeclaration = require('../../lib/web-inspector').CSSStyleDeclaration; @@ -50,33 +51,75 @@ function getFailingRules(fontSizeArtifact) { } function nodeToString(node) { - return node.localName; + const attributesString = node.attributes.map((value, idx) => { + if(idx % 2 === 0) { + return ` ${value}`; + } + + return value ? `='${value}'` : ''; + }).join(''); + + return `<${node.localName}${attributesString}>`; } /** - * @param {WebInspector.CSSStyleDeclaration} rule + * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets * @param {Node} node + * @param {string} baseURL + * @param {WebInspector.CSSStyleDeclaration} styleDeclaration * @returns string */ -function getOriginDescription(rule, node) { - if (!rule) { - return 'User Agent Stylesheet'; +function getOrigin(stylesheets, node, baseURL, styleDeclaration) { + if (!styleDeclaration) { + return { + details: 'User Agent Stylesheet' + }; } - if (rule.type === CSSStyleDeclaration.Type.Attributes) { - return `Attributes Style: ${nodeToString(node)}`; + const range = styleDeclaration && styleDeclaration.range; + let file = baseURL; + let location = ''; + + if(range) { + location = `${range.startLine}:${range.startColumn}`; } - if (rule.type === CSSStyleDeclaration.Type.Inline) { - return `Inline Style: ${nodeToString(node)}`; + if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes) { + return { + details: `Attributes Style: ${nodeToString(node)}`, + file, + location + }; } - if (rule.type === CSSStyleDeclaration.Type.Regular && rule.parentRule) { - const selector = rule.parentRule.selectors.map(item => item.text).join(', '); - return `CSS Selector: ${selector}`; + if (styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { + return { + details: `Inline Style: ${nodeToString(node)}`, + file, + location + }; + } + + if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular && styleDeclaration.parentRule) { + const rule = styleDeclaration.parentRule; + const selector = rule.selectors.map(item => item.text).join(', '); + const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); + + if (stylesheetMeta) { + const url = new URL(stylesheetMeta.header.sourceURL, baseURL); + file = `${url.href}`; + } + + return { + details: `CSS Selector: ${selector}`, + file, + location + }; } - return 'unknown'; + return { + details: 'Unknown' + }; } /** @@ -107,7 +150,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize'] + requiredArtifacts: ['FontSize', 'Styles', 'URL'] }; } @@ -136,19 +179,25 @@ class FontSize extends Audit { const failingRules = getFailingRules(artifacts.FontSize); const failingTextLength = getTotalTextLength(failingRules); const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; + const pageUrl = artifacts.URL.finalUrl; const headings = [ - {key: 'origin', itemType: 'code', text: 'Origin'}, - {key: 'percentage', itemType: 'text', text: '% of text'}, + {key: 'file', itemType: 'url', text: 'File'}, + {key: 'location', itemType: 'url', text: 'Line and Column'}, + {key: 'details', itemType: 'code', text: 'Details'}, + {key: 'percentage', itemType: 'text', text: '% of Text'}, {key: 'fontSize', itemType: 'text', text: 'Font Size'} ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) .map(({cssRule, node, textLength, fontSize}) => { const percentageOfAffectedText = textLength / totalTextLenght * 100; + const origin = getOrigin(artifacts.Styles, node, pageUrl, cssRule); return { - origin: getOriginDescription(cssRule, node), + file: origin.file, + location: origin.location, + details: origin.details, percentage: `${percentageOfAffectedText.toFixed(2)}%`, fontSize: `${fontSize}px`, }; @@ -160,7 +209,7 @@ class FontSize extends Audit { rawValue: passed, details, debugString: passed ? - `${percentageOfPassingText.toFixed(2)}% of text is legible` : + `${percentageOfPassingText.toFixed(2)}% of text is legible.` : `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.` }; } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 09eafe8b0540..c7cd01ec3404 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -13,6 +13,8 @@ module.exports = { 'viewport', 'seo/meta-description', 'seo/font-size', + 'url', + 'styles', ], }], audits: [ From 312d030aa8606cfcfa2cb04ea5ba1d1c7a01205e Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Tue, 10 Oct 2017 02:25:13 +0200 Subject: [PATCH 33/65] Update output table according to the latst decisions. --- lighthouse-core/audits/seo/font-size.js | 74 ++++++------------- lighthouse-core/config/seo.js | 8 +- .../gather/gatherers/seo/font-size.js | 2 +- 3 files changed, 29 insertions(+), 55 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 131fbc5e1c62..e403e119f357 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -40,7 +40,7 @@ function getFailingRules(fontSizeArtifact) { node, cssRule, fontSize, - textLength + textLength, }); } else { failingRule.textLength += textLength; @@ -50,53 +50,30 @@ function getFailingRules(fontSizeArtifact) { return failingRules.valuesArray(); } -function nodeToString(node) { - const attributesString = node.attributes.map((value, idx) => { - if(idx % 2 === 0) { - return ` ${value}`; - } - - return value ? `='${value}'` : ''; - }).join(''); - - return `<${node.localName}${attributesString}>`; -} - /** * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets - * @param {Node} node * @param {string} baseURL * @param {WebInspector.CSSStyleDeclaration} styleDeclaration - * @returns string + * @returns {{source:!string, selector:string}} */ -function getOrigin(stylesheets, node, baseURL, styleDeclaration) { +function getOrigin(stylesheets, baseURL, styleDeclaration) { if (!styleDeclaration) { return { - details: 'User Agent Stylesheet' + source: 'User Agent Stylesheet', }; } const range = styleDeclaration && styleDeclaration.range; - let file = baseURL; - let location = ''; + let source = baseURL; if(range) { - location = `${range.startLine}:${range.startColumn}`; - } - - if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes) { - return { - details: `Attributes Style: ${nodeToString(node)}`, - file, - location - }; + source += `:${range.startLine}:${range.startColumn}`; } - if (styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { + if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || + styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { return { - details: `Inline Style: ${nodeToString(node)}`, - file, - location + source, }; } @@ -107,18 +84,17 @@ function getOrigin(stylesheets, node, baseURL, styleDeclaration) { if (stylesheetMeta) { const url = new URL(stylesheetMeta.header.sourceURL, baseURL); - file = `${url.href}`; + source = `${url.href}`; } return { - details: `CSS Selector: ${selector}`, - file, - location + selector, + source, }; } return { - details: 'Unknown' + source: 'Unknown', }; } @@ -150,7 +126,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize', 'Styles', 'URL'] + requiredArtifacts: ['FontSize', 'Styles', 'URL'], }; } @@ -182,23 +158,21 @@ class FontSize extends Audit { const pageUrl = artifacts.URL.finalUrl; const headings = [ - {key: 'file', itemType: 'url', text: 'File'}, - {key: 'location', itemType: 'url', text: 'Line and Column'}, - {key: 'details', itemType: 'code', text: 'Details'}, - {key: 'percentage', itemType: 'text', text: '% of Text'}, - {key: 'fontSize', itemType: 'text', text: 'Font Size'} + {key: 'source', itemType: 'code', text: 'Source'}, + {key: 'selector', itemType: 'code', text: 'Selector'}, + {key: 'coverage', itemType: 'text', text: 'Coverage'}, + {key: 'fontSize', itemType: 'text', text: 'Font Size'}, ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) - .map(({cssRule, node, textLength, fontSize}) => { + .map(({cssRule, textLength, fontSize}) => { const percentageOfAffectedText = textLength / totalTextLenght * 100; - const origin = getOrigin(artifacts.Styles, node, pageUrl, cssRule); + const origin = getOrigin(artifacts.Styles, pageUrl, cssRule); return { - file: origin.file, - location: origin.location, - details: origin.details, - percentage: `${percentageOfAffectedText.toFixed(2)}%`, + source: origin.source, + selector: origin.selector, + coverage: `${percentageOfAffectedText.toFixed(2)}%`, fontSize: `${fontSize}px`, }; }); @@ -210,7 +184,7 @@ class FontSize extends Audit { details, debugString: passed ? `${percentageOfPassingText.toFixed(2)}% of text is legible.` : - `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.` + `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, }; } } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index c7cd01ec3404..09232a18ca34 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -42,10 +42,10 @@ module.exports = { name: 'SEO', description: 'These ensure your app is able to be understood by search engine crawlers.', audits: [ - {id: 'viewport', weight: 1, group: 'seo-mobile'}, - {id: 'document-title', weight: 1, group: 'seo-content'}, - {id: 'meta-description', weight: 1, group: 'seo-content'}, - {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, + // {id: 'viewport', weight: 1, group: 'seo-mobile'}, + // {id: 'document-title', weight: 1, group: 'seo-content'}, + // {id: 'meta-description', weight: 1, group: 'seo-content'}, + // {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, {id: 'font-size', weight: 1, group: 'seo-mobile'}, ], }, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 3abbb2152707..8ae89d8c4db1 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -79,7 +79,7 @@ function getEffectiveRule(property, node, { inlineStyle, attributesStyle, matchedCSSRules, - inherited + inherited, }) { const cssModel = { styleSheetHeaderForId: id => ({id}), From 1f7ddbb05b63dedb6596b91ebfbaf64edf128cda Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 10:03:34 +0200 Subject: [PATCH 34/65] Changes to the result table. Clean up. --- lighthouse-core/audits/seo/font-size.js | 72 ++++++++++++------- .../gather/gatherers/seo/font-size.js | 9 ++- lighthouse-core/lib/web-inspector.js | 1 + 3 files changed, 51 insertions(+), 31 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index e403e119f357..9f0b50f60294 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -50,47 +50,67 @@ function getFailingRules(fontSizeArtifact) { return failingRules.valuesArray(); } +/** + * @param {Node} node + * @return {{type:string, snippet:string}} + */ +function nodeToTableNode(node) { + const attributesString = node.attributes.map((value, idx) => + (idx % 2 === 0) ? ` ${value}` : `="${value}"` + ).join(''); + + return { + type: 'node', + snippet: `<${node.localName}${attributesString}>`, + }; +} + /** * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets * @param {string} baseURL * @param {WebInspector.CSSStyleDeclaration} styleDeclaration + * @param {Node} node * @returns {{source:!string, selector:string}} */ -function getOrigin(stylesheets, baseURL, styleDeclaration) { - if (!styleDeclaration) { +function getOrigin(stylesheets, baseURL, styleDeclaration, node) { + if (styleDeclaration.parentRule && + styleDeclaration.parentRule.origin === CSSAgent.StyleSheetOrigin.USER_AGENT) { return { + selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '), source: 'User Agent Stylesheet', }; } - const range = styleDeclaration && styleDeclaration.range; - let source = baseURL; - - if(range) { - source += `:${range.startLine}:${range.startColumn}`; - } - if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { return { - source, + source: baseURL, + selector: nodeToTableNode(node), }; } if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular && styleDeclaration.parentRule) { const rule = styleDeclaration.parentRule; - const selector = rule.selectors.map(item => item.text).join(', '); const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); if (stylesheetMeta) { const url = new URL(stylesheetMeta.header.sourceURL, baseURL); - source = `${url.href}`; - } + const range = styleDeclaration.range; + const selector = rule.selectors.map(item => item.text).join(', '); + let source = `${url.href}`; - return { - selector, - source, - }; + if(range) { + const absoluteStartLine = range.startLine + stylesheetMeta.header.startLine + 1; + const absoluteStartColumn = range.startColumn + stylesheetMeta.header.startColumn + 1; + + source += `:${absoluteStartLine}:${absoluteStartColumn}`; + } + + return { + selector, + source, + }; + } } return { @@ -99,15 +119,15 @@ function getOrigin(stylesheets, baseURL, styleDeclaration) { } /** - * @param {WebInspector.CSSStyleDeclaration} rule + * @param {WebInspector.CSSStyleDeclaration} styleDeclaration * @param {Node} node * @return string */ -function getFontArtifactId(rule, node) { - if (!rule) { - return 'user-agent'; - } else if (rule.type === CSSStyleDeclaration.Type.Regular) { - return `${rule.styleSheetId}@${rule.range.startLine}:${rule.range.startColumn}`; +function getFontArtifactId(styleDeclaration, node) { + if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular) { + const startLine = styleDeclaration.range ? styleDeclaration.range.startLine : 0; + const startColumn = styleDeclaration.range ? styleDeclaration.range.startColumn : 0; + return `${styleDeclaration.styleSheetId}@${startLine}:${startColumn}`; } else { return `node_${node.nodeId}`; } @@ -158,16 +178,16 @@ class FontSize extends Audit { const pageUrl = artifacts.URL.finalUrl; const headings = [ - {key: 'source', itemType: 'code', text: 'Source'}, + {key: 'source', itemType: 'url', text: 'Source'}, {key: 'selector', itemType: 'code', text: 'Selector'}, {key: 'coverage', itemType: 'text', text: 'Coverage'}, {key: 'fontSize', itemType: 'text', text: 'Font Size'}, ]; const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) - .map(({cssRule, textLength, fontSize}) => { + .map(({cssRule, textLength, fontSize, node}) => { const percentageOfAffectedText = textLength / totalTextLenght * 100; - const origin = getOrigin(artifacts.Styles, pageUrl, cssRule); + const origin = getOrigin(artifacts.Styles, pageUrl, cssRule, node); return { source: origin.source, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 8ae89d8c4db1..789fbed2c0cb 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -7,7 +7,6 @@ const CSSMatchedStyles = require('../../../lib/web-inspector').CSSMatchedStyles; const Gatherer = require('../gatherer'); -const TEXT_NODE = 3; const FONT_SIZE_PROPERTY_NAME = 'font-size'; /** @@ -110,8 +109,8 @@ function getEffectiveRule(property, node, { /** * @param {!Object} driver - * @param {!Node} node - * @returns {!{fontSize: number, textLength: number, cssRule: WebInspector.CSSStyleDeclaration}} + * @param {!Node} node text node + * @returns {!{fontSize: number, textLength: number, node: Node, cssRule: WebInspector.CSSStyleDeclaration}} */ function getFontSizeInformation(driver, node) { const computedStyles = driver.sendCommand('CSS.getComputedStyleForNode', {nodeId: node.parentId}); @@ -126,7 +125,7 @@ function getFontSizeInformation(driver, node) { fontSize: parseInt(fontSizeProperty.value, 10), textLength: node.nodeValue.trim().length, node: node.parentNode, - cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node.parentNode, matchedRules) + cssRule: getEffectiveRule(FONT_SIZE_PROPERTY_NAME, node.parentNode, matchedRules), }; }); } @@ -136,7 +135,7 @@ function getFontSizeInformation(driver, node) { * @returns {boolean} */ function isNonEmptyTextNode(node) { - return node.nodeType === TEXT_NODE && node.nodeValue.trim().length > 0; + return node.nodeType === Node.TEXT_NODE && node.nodeValue.trim().length > 0; } class FontSize extends Gatherer { diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index 9fda42b66828..ff890d424ce2 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -26,6 +26,7 @@ module.exports = (function() { global.Node = { ELEMENT_NODE: 1, + TEXT_NODE: 3, }; global.CSSAgent = {}; From 1698dd7d564858a0e602ed802f394e0f04540f19 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 10:03:43 +0200 Subject: [PATCH 35/65] Gatherer test. --- .../gather/gatherers/seo/font-size-test.js | 75 +++++++++++++++++++ .../test/lib/web-inspector-test.js | 1 + 2 files changed, 76 insertions(+) create mode 100644 lighthouse-core/test/gather/gatherers/seo/font-size-test.js diff --git a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js new file mode 100644 index 000000000000..2250d689ca21 --- /dev/null +++ b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js @@ -0,0 +1,75 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/* eslint-env mocha */ + +const FontSizeGather = require('../../../../gather/gatherers/seo/font-size'); +const assert = require('assert'); +let fontSizeGather; +const body = { + nodeName: 'BODY', + children: [ + {nodeValue: ' text ', nodeType: Node.TEXT_NODE, parentId: 1}, + {nodeValue: ' text ', nodeType: Node.ELEMENT_NODE}, + {nodeValue: ' ', nodeType: Node.TEXT_NODE}, + {nodeValue: 'texttext', nodeType: Node.TEXT_NODE, parentId: 2}, + ], +}; +const dom = { + root: { + nodeName: 'HTML', + children: [body], + }, +}; + +describe('Font size gatherer', () => { + // Reset the Gatherer before each test. + beforeEach(() => { + fontSizeGather = new FontSizeGather(); + }); + + it('returns information about font size\'s used on page', () => { + return fontSizeGather.afterPass({ + driver: { + sendCommand(command, params) { + let result; + if (command === 'DOM.getDocument') { + result = dom; + } else if (command === 'CSS.getComputedStyleForNode') { + result = {computedStyle: [ + {name: 'font-size', value: params.nodeId === 1 ? 10 : 20}, + ]}; + } else if (command === 'CSS.getMatchedStylesForNode') { + result = { + inlineStyle: null, + attributesStyle: null, + matchedCSSRules: [], + inherited: [], + }; + } + + return Promise.resolve(result); + }, + }, + }).then(artifact => { + assert.deepEqual(artifact, [ + { + fontSize: 10, + textLength: 4, + cssRule: undefined, + node: body, + }, + { + fontSize: 20, + textLength: 8, + cssRule: undefined, + node: body, + }, + ]); + }); + }); +}); diff --git a/lighthouse-core/test/lib/web-inspector-test.js b/lighthouse-core/test/lib/web-inspector-test.js index 01cc059180fd..5308344bb6a1 100644 --- a/lighthouse-core/test/lib/web-inspector-test.js +++ b/lighthouse-core/test/lib/web-inspector-test.js @@ -19,6 +19,7 @@ describe('Web Inspector lib', function() { assert.ok(WebInspector.TimelineAggregator); assert.ok(WebInspector.NetworkManager); assert.ok(WebInspector.Color); + assert.ok(WebInspector.CSSMetadata); }); // Our implementation of using DevTools doesn't sandbox natives From 97e1096adfe7f50dbdcee8bccef4faf43198a694 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 22:54:28 +0200 Subject: [PATCH 36/65] Smoke tests. Fixing edgecase when size is inherited from attribute styles of a parent node. Fixing linting errors, improvic jsdoc. --- .../test/fixtures/seo/seo-tester.html | 14 +++++++++- .../test/smokehouse/seo/expectations.js | 8 ++++++ lighthouse-core/audits/seo/font-size.js | 27 ++++++++++--------- lighthouse-core/config/seo.js | 16 ++++++----- .../gather/gatherers/seo/font-size.js | 14 +++++----- .../gather/gatherers/seo/font-size-test.js | 8 +++--- 6 files changed, 56 insertions(+), 31 deletions(-) diff --git a/lighthouse-cli/test/fixtures/seo/seo-tester.html b/lighthouse-cli/test/fixtures/seo/seo-tester.html index 9b2d45754325..4ad7686b809f 100644 --- a/lighthouse-cli/test/fixtures/seo/seo-tester.html +++ b/lighthouse-cli/test/fixtures/seo/seo-tester.html @@ -11,8 +11,20 @@ + - crawlz me + +

123451234512345

+ + +

1

+ 2 + 34 +

5

diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 831cc6951362..cf8bd3593085 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -25,6 +25,10 @@ module.exports = [ 'http-status-code': { score: true, }, + 'font-size': { + rawValue: true, + debugString: '75% of text is legible.', + }, }, }, { @@ -49,6 +53,10 @@ module.exports = [ score: false, displayValue: '403', }, + 'font-size': { + rawValue: false, + debugString: 'Text is illegible because of a missing viewport config', + }, }, }, ]; diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 9f0b50f60294..0de23e6010e9 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -73,19 +73,22 @@ function nodeToTableNode(node) { * @returns {{source:!string, selector:string}} */ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { - if (styleDeclaration.parentRule && - styleDeclaration.parentRule.origin === CSSAgent.StyleSheetOrigin.USER_AGENT) { + if ( + !styleDeclaration || + styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || + styleDeclaration.type === CSSStyleDeclaration.Type.Inline + ) { return { - selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '), - source: 'User Agent Stylesheet', + source: baseURL, + selector: nodeToTableNode(node), }; } - if (styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || - styleDeclaration.type === CSSStyleDeclaration.Type.Inline) { + if (styleDeclaration.parentRule && + styleDeclaration.parentRule.origin === global.CSSAgent.StyleSheetOrigin.USER_AGENT) { return { - source: baseURL, - selector: nodeToTableNode(node), + selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '), + source: 'User Agent Stylesheet', }; } @@ -124,7 +127,7 @@ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { * @return string */ function getFontArtifactId(styleDeclaration, node) { - if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular) { + if (styleDeclaration && styleDeclaration.type === CSSStyleDeclaration.Type.Regular) { const startLine = styleDeclaration.range ? styleDeclaration.range.startLine : 0; const startColumn = styleDeclaration.range ? styleDeclaration.range.startColumn : 0; return `${styleDeclaration.styleSheetId}@${startLine}:${startColumn}`; @@ -198,13 +201,13 @@ class FontSize extends Audit { }); const details = Audit.makeTableDetails(headings, tableData); const passed = percentageOfPassingText >= MINIMAL_PERCENTAGE_OF_LEGIBLE_TEXT; + const debugString = passed ? + null : `${parseFloat((100 - percentageOfPassingText).toFixed(2))}% of text is too small.`; return { rawValue: passed, details, - debugString: passed ? - `${percentageOfPassingText.toFixed(2)}% of text is legible.` : - `${(100 - percentageOfPassingText).toFixed(2)}% of text is too small.`, + debugString, }; } } diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 09232a18ca34..099d9e6ed7d3 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -6,14 +6,16 @@ 'use strict'; module.exports = { - // extends: 'lighthouse:default', + extends: 'lighthouse:default', passes: [{ passName: 'defaultPass', gatherers: [ - 'viewport', 'seo/meta-description', + ], + }, { + passName: 'extraPass', + gatherers: [ 'seo/font-size', - 'url', 'styles', ], }], @@ -42,10 +44,10 @@ module.exports = { name: 'SEO', description: 'These ensure your app is able to be understood by search engine crawlers.', audits: [ - // {id: 'viewport', weight: 1, group: 'seo-mobile'}, - // {id: 'document-title', weight: 1, group: 'seo-content'}, - // {id: 'meta-description', weight: 1, group: 'seo-content'}, - // {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, + {id: 'viewport', weight: 1, group: 'seo-mobile'}, + {id: 'document-title', weight: 1, group: 'seo-content'}, + {id: 'meta-description', weight: 1, group: 'seo-content'}, + {id: 'http-status-code', weight: 1, group: 'seo-crawl'}, {id: 'font-size', weight: 1, group: 'seo-mobile'}, ], }, diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 789fbed2c0cb..d3b2486aaaf4 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -71,7 +71,7 @@ function getAllNodesFromBody(driver) { * * @param {!string} property CSS property name * @param {!Node} node - * @param {!Object} matched CSS rule + * @param {!Object} matched CSS rules * @returns {WebInspector.CSSStyleDeclaration} */ function getEffectiveRule(property, node, { @@ -135,14 +135,14 @@ function getFontSizeInformation(driver, node) { * @returns {boolean} */ function isNonEmptyTextNode(node) { - return node.nodeType === Node.TEXT_NODE && node.nodeValue.trim().length > 0; + return node.nodeType === global.Node.TEXT_NODE && node.nodeValue.trim().length > 0; } class FontSize extends Gatherer { /** * @param {{driver: !Object}} options Run options - * @return {!Promise>} The font-size value of the document body + * @return {!Promise>} font-size analysis */ afterPass(options) { const enableDOM = options.driver.sendCommand('DOM.enable'); @@ -155,10 +155,10 @@ class FontSize extends Gatherer { textNodes.map(node => getFontSizeInformation(options.driver, node)) )) .then(fontSizeInfo => { - options.driver.sendCommand('DOM.disable'); - options.driver.sendCommand('CSS.disable'); - - return fontSizeInfo; + return Promise.all([ + options.driver.sendCommand('DOM.disable'), + options.driver.sendCommand('CSS.disable'), + ]).then(_ => fontSizeInfo); }); } } diff --git a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js index 2250d689ca21..81ad822bd3b2 100644 --- a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js +++ b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js @@ -13,10 +13,10 @@ let fontSizeGather; const body = { nodeName: 'BODY', children: [ - {nodeValue: ' text ', nodeType: Node.TEXT_NODE, parentId: 1}, - {nodeValue: ' text ', nodeType: Node.ELEMENT_NODE}, - {nodeValue: ' ', nodeType: Node.TEXT_NODE}, - {nodeValue: 'texttext', nodeType: Node.TEXT_NODE, parentId: 2}, + {nodeValue: ' text ', nodeType: global.Node.TEXT_NODE, parentId: 1}, + {nodeValue: ' text ', nodeType: global.Node.ELEMENT_NODE}, + {nodeValue: ' ', nodeType: global.Node.TEXT_NODE}, + {nodeValue: 'texttext', nodeType: global.Node.TEXT_NODE, parentId: 2}, ], }; const dom = { From 12fecc78c6c32ecd007d73431b0e8906f5272451 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 11 Oct 2017 23:48:24 +0200 Subject: [PATCH 37/65] Add unit tests. --- lighthouse-core/audits/seo/font-size.js | 3 +- .../test/audits/seo/font-size-test.js | 111 ++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 lighthouse-core/test/audits/seo/font-size-test.js diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 0de23e6010e9..4414fb5aa9f0 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -149,7 +149,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize', 'Styles', 'URL'], + requiredArtifacts: ['FontSize', 'Styles', 'URL', 'Viewport'], }; } @@ -171,7 +171,6 @@ class FontSize extends Audit { if (totalTextLenght === 0) { return { rawValue: true, - debugString: 'Page contains no text', }; } diff --git a/lighthouse-core/test/audits/seo/font-size-test.js b/lighthouse-core/test/audits/seo/font-size-test.js new file mode 100644 index 000000000000..3c58ef20e906 --- /dev/null +++ b/lighthouse-core/test/audits/seo/font-size-test.js @@ -0,0 +1,111 @@ +/** + * @license Copyright 2017 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const FontSizeAudit = require('../../../audits/seo/font-size.js'); +const assert = require('assert'); +const CSSStyleDeclaration = require('../../../lib/web-inspector').CSSStyleDeclaration; + +const URL = 'https://example.com'; +const Styles = []; +const validViewport = 'width=device-width'; + +/* eslint-env mocha */ + +describe('SEO: Font size audit', () => { + it('fails when viewport is not set', () => { + const artifacts = { + URL, + Viewport: null, + Styles, + FontSize: [], + }; + + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, false); + assert.ok(auditResult.debugString.includes('missing viewport')); + }); + + it('fails when less than 75% of text is legible', () => { + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 1, fontSize: 15, node: {nodeId: 1, localName: 'p', attributes: []}}, + {textLength: 2, fontSize: 16, node: {nodeId: 2, localName: 'p', attributes: []}}, + ], + }; + + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, false); + assert.ok(auditResult.debugString.includes('33.33%')); + }); + + it('passes when there is no text', () => { + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 0}, + {textLength: 0}, + ], + }; + + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, true); + }); + + it('passes when more than 75% of text is legible', () => { + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 1, fontSize: 15, node: {nodeId: 1, localName: 'p', attributes: []}}, + {textLength: 2, fontSize: 16, node: {nodeId: 2, localName: 'p', attributes: []}}, + {textLength: 2, fontSize: 24, node: {nodeId: 3, localName: 'p', attributes: []}}, + ], + }; + const auditResult = FontSizeAudit.audit(artifacts); + assert.equal(auditResult.rawValue, true); + }); + + it('groups entries with same source, sorts them by coverage', () => { + const style1 = { + styleSheetId: 1, + type: CSSStyleDeclaration.Type.Regular, + range: { + startLine: 123, + startColumn: 10, + }, + }; + const style2 = { + styleSheetId: 1, + type: CSSStyleDeclaration.Type.Regular, + range: { + startLine: 0, + startColumn: 10, + }, + }; + const artifacts = { + URL, + Viewport: validViewport, + Styles, + FontSize: [ + {textLength: 3, fontSize: 15, node: {nodeId: 1}, cssRule: style1}, + {textLength: 2, fontSize: 14, node: {nodeId: 2}, cssRule: style2}, + {textLength: 2, fontSize: 14, node: {nodeId: 3}, cssRule: style2}, + ], + }; + const auditResult = FontSizeAudit.audit(artifacts); + + assert.equal(auditResult.rawValue, false); + assert.equal(auditResult.details.items.length, 2); + assert.equal(auditResult.details.items[0][2].text, '57.14%'); + }); +}); From 0929685e654773cce3ca1d46cb9003028f23e1f7 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 00:11:30 +0200 Subject: [PATCH 38/65] Remove info about color prop. --- lighthouse-core/lib/web-inspector.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lighthouse-core/lib/web-inspector.js b/lighthouse-core/lib/web-inspector.js index ff890d424ce2..2f40d1b0cc95 100644 --- a/lighthouse-core/lib/web-inspector.js +++ b/lighthouse-core/lib/web-inspector.js @@ -333,10 +333,6 @@ module.exports = (function() { name: 'font-size', inherited: true, }, - { - name: 'color', - inherited: true, - }, ]; return WebInspector; From a0a08f7adfe7a03630f7260f690e8c8d243899b0 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 00:43:38 +0200 Subject: [PATCH 39/65] Remove debugMessage from passing test expectations as it doesn't exist. --- lighthouse-cli/test/smokehouse/seo/expectations.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index cf8bd3593085..5a30a34f1283 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -27,7 +27,6 @@ module.exports = [ }, 'font-size': { rawValue: true, - debugString: '75% of text is legible.', }, }, }, From b3a99b94016b046d4b43fe9c7e16d4096517eb18 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 11:05:11 +0200 Subject: [PATCH 40/65] Replace URL with parseURL --- lighthouse-core/audits/seo/font-size.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 4414fb5aa9f0..cbaa93094cda 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -5,7 +5,7 @@ */ 'use strict'; -const {URL} = require('url'); +const parseURL = require('url').parse; const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const CSSStyleDeclaration = require('../../lib/web-inspector').CSSStyleDeclaration; @@ -97,7 +97,7 @@ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); if (stylesheetMeta) { - const url = new URL(stylesheetMeta.header.sourceURL, baseURL); + const url = parseURL(stylesheetMeta.header.sourceURL, baseURL); const range = styleDeclaration.range; const selector = rule.selectors.map(item => item.text).join(', '); let source = `${url.href}`; From d45a3135a255f295a5fcdef44ee72e04d3f637bd Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Thu, 12 Oct 2017 11:17:21 +0200 Subject: [PATCH 41/65] Make linter happy. --- lighthouse-core/audits/seo/font-size.js | 2 +- lighthouse-core/gather/gatherers/seo/font-size.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index cbaa93094cda..23629b15ce23 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -102,7 +102,7 @@ function getOrigin(stylesheets, baseURL, styleDeclaration, node) { const selector = rule.selectors.map(item => item.text).join(', '); let source = `${url.href}`; - if(range) { + if (range) { const absoluteStartLine = range.startLine + stylesheetMeta.header.startLine + 1; const absoluteStartColumn = range.startColumn + stylesheetMeta.header.startColumn + 1; diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index d3b2486aaaf4..4a452cb8843e 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -139,7 +139,6 @@ function isNonEmptyTextNode(node) { } class FontSize extends Gatherer { - /** * @param {{driver: !Object}} options Run options * @return {!Promise>} font-size analysis From e810e67ac49ce20b9da5e8170f59d9b2021ce932 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Mon, 16 Oct 2017 00:01:46 +0200 Subject: [PATCH 42/65] Check number of failing items in the smoke test. --- lighthouse-cli/test/smokehouse/seo/expectations.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 5a30a34f1283..bf13afd9b3b5 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -27,6 +27,11 @@ module.exports = [ }, 'font-size': { rawValue: true, + details: { + items: { + length: 5, + }, + }, }, }, }, From 1f4c6d2a986c0448c110c69ddd9547fbd018e00b Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Mon, 16 Oct 2017 00:07:51 +0200 Subject: [PATCH 43/65] getOrigin -> findStyleRuleSource s/Lenght/Length --- lighthouse-core/audits/seo/font-size.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 23629b15ce23..67a9e9f6c1b3 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -72,7 +72,7 @@ function nodeToTableNode(node) { * @param {Node} node * @returns {{source:!string, selector:string}} */ -function getOrigin(stylesheets, baseURL, styleDeclaration, node) { +function findStyleRuleSource(stylesheets, baseURL, styleDeclaration, node) { if ( !styleDeclaration || styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || @@ -166,9 +166,9 @@ class FontSize extends Audit { }; } - const totalTextLenght = getTotalTextLength(artifacts.FontSize); + const totalTextLength = getTotalTextLength(artifacts.FontSize); - if (totalTextLenght === 0) { + if (totalTextLength === 0) { return { rawValue: true, }; @@ -176,7 +176,7 @@ class FontSize extends Audit { const failingRules = getFailingRules(artifacts.FontSize); const failingTextLength = getTotalTextLength(failingRules); - const percentageOfPassingText = (totalTextLenght - failingTextLength) / totalTextLenght * 100; + const percentageOfPassingText = (totalTextLength - failingTextLength) / totalTextLength * 100; const pageUrl = artifacts.URL.finalUrl; const headings = [ @@ -188,8 +188,8 @@ class FontSize extends Audit { const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) .map(({cssRule, textLength, fontSize, node}) => { - const percentageOfAffectedText = textLength / totalTextLenght * 100; - const origin = getOrigin(artifacts.Styles, pageUrl, cssRule, node); + const percentageOfAffectedText = textLength / totalTextLength * 100; + const origin = findStyleRuleSource(artifacts.Styles, pageUrl, cssRule, node); return { source: origin.source, From 682b2c765df2875af33c48bea34ffeaa3c2295c1 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Mon, 16 Oct 2017 01:26:36 +0200 Subject: [PATCH 44/65] Collect stylesheet metadata in font-size gatherer. Get rid of separate run. --- lighthouse-core/audits/seo/font-size.js | 17 ++++++++--------- lighthouse-core/config/seo.js | 5 ----- .../gather/gatherers/seo/font-size.js | 10 ++++++++++ .../test/gather/gatherers/seo/font-size-test.js | 2 ++ 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 67a9e9f6c1b3..a0a239b636a8 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -66,13 +66,12 @@ function nodeToTableNode(node) { } /** - * @param {Array<{header:{styleSheetId:string, sourceURL:string}}>} stylesheets * @param {string} baseURL * @param {WebInspector.CSSStyleDeclaration} styleDeclaration * @param {Node} node * @returns {{source:!string, selector:string}} */ -function findStyleRuleSource(stylesheets, baseURL, styleDeclaration, node) { +function findStyleRuleSource(baseURL, styleDeclaration, node) { if ( !styleDeclaration || styleDeclaration.type === CSSStyleDeclaration.Type.Attributes || @@ -94,17 +93,17 @@ function findStyleRuleSource(stylesheets, baseURL, styleDeclaration, node) { if (styleDeclaration.type === CSSStyleDeclaration.Type.Regular && styleDeclaration.parentRule) { const rule = styleDeclaration.parentRule; - const stylesheetMeta = stylesheets.find(ss => ss.header.styleSheetId === rule.styleSheetId); + const stylesheet = styleDeclaration.stylesheet; - if (stylesheetMeta) { - const url = parseURL(stylesheetMeta.header.sourceURL, baseURL); + if (stylesheet) { + const url = parseURL(stylesheet.sourceURL, baseURL); const range = styleDeclaration.range; const selector = rule.selectors.map(item => item.text).join(', '); let source = `${url.href}`; if (range) { - const absoluteStartLine = range.startLine + stylesheetMeta.header.startLine + 1; - const absoluteStartColumn = range.startColumn + stylesheetMeta.header.startColumn + 1; + const absoluteStartLine = range.startLine + stylesheet.startLine + 1; + const absoluteStartColumn = range.startColumn + stylesheet.startColumn + 1; source += `:${absoluteStartLine}:${absoluteStartColumn}`; } @@ -149,7 +148,7 @@ class FontSize extends Audit { helpText: 'Font sizes less than 16px are too small to be legible and require mobile ' + 'visitors to “pinch to zoom” in order to read. ' + '[Learn more](https://developers.google.com/speed/docs/insights/UseLegibleFontSizes).', - requiredArtifacts: ['FontSize', 'Styles', 'URL', 'Viewport'], + requiredArtifacts: ['FontSize', 'URL', 'Viewport'], }; } @@ -189,7 +188,7 @@ class FontSize extends Audit { const tableData = failingRules.sort((a, b) => b.textLength - a.textLength) .map(({cssRule, textLength, fontSize, node}) => { const percentageOfAffectedText = textLength / totalTextLength * 100; - const origin = findStyleRuleSource(artifacts.Styles, pageUrl, cssRule, node); + const origin = findStyleRuleSource(pageUrl, cssRule, node); return { source: origin.source, diff --git a/lighthouse-core/config/seo.js b/lighthouse-core/config/seo.js index 099d9e6ed7d3..ad4cae7e158b 100644 --- a/lighthouse-core/config/seo.js +++ b/lighthouse-core/config/seo.js @@ -11,12 +11,7 @@ module.exports = { passName: 'defaultPass', gatherers: [ 'seo/meta-description', - ], - }, { - passName: 'extraPass', - gatherers: [ 'seo/font-size', - 'styles', ], }], audits: [ diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index 4a452cb8843e..a9cdc7f8ca78 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -144,6 +144,10 @@ class FontSize extends Gatherer { * @return {!Promise>} font-size analysis */ afterPass(options) { + const stylesheets = new Map(); + const onStylesheetAdd = sheet => stylesheets.set(sheet.header.styleSheetId, sheet.header); + options.driver.on('CSS.styleSheetAdded', onStylesheetAdd); + const enableDOM = options.driver.sendCommand('DOM.enable'); const enableCSS = options.driver.sendCommand('CSS.enable'); @@ -154,6 +158,12 @@ class FontSize extends Gatherer { textNodes.map(node => getFontSizeInformation(options.driver, node)) )) .then(fontSizeInfo => { + options.driver.off('CSS.styleSheetAdded', onStylesheetAdd); + + fontSizeInfo + .filter(info => info.cssRule && info.cssRule.styleSheetId) + .forEach(info => info.cssRule.stylesheet = stylesheets.get(info.cssRule.styleSheetId)); + return Promise.all([ options.driver.sendCommand('DOM.disable'), options.driver.sendCommand('CSS.disable'), diff --git a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js index 81ad822bd3b2..e4cc5f7722fb 100644 --- a/lighthouse-core/test/gather/gatherers/seo/font-size-test.js +++ b/lighthouse-core/test/gather/gatherers/seo/font-size-test.js @@ -35,6 +35,8 @@ describe('Font size gatherer', () => { it('returns information about font size\'s used on page', () => { return fontSizeGather.afterPass({ driver: { + on() {}, + off() {}, sendCommand(command, params) { let result; if (command === 'DOM.getDocument') { From a8b14f620428b68b0c58f53115ee8004719c5917 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Sun, 29 Oct 2017 23:39:16 +0100 Subject: [PATCH 45/65] Address some of the review comments --- lighthouse-core/audits/seo/font-size.js | 7 ++++--- lighthouse-core/config/default.js | 2 ++ lighthouse-core/gather/gatherers/seo/font-size.js | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index a0a239b636a8..947a87844de7 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -24,7 +24,7 @@ function getTotalTextLength(rules) { * @param {Array<{cssRule: WebInspector.CSSStyleDeclaration, fontSize: number, textLength: number, node: Node}>} fontSizeArtifact * @returns {Array<{cssRule: WebInspector.CSSStyleDeclaration, fontSize: number, textLength: number, node: Node}>} */ -function getFailingRules(fontSizeArtifact) { +function getUniqueFailingRules(fontSizeArtifact) { const failingRules = new Map(); fontSizeArtifact.forEach(({cssRule, fontSize, textLength, node}) => { @@ -102,6 +102,8 @@ function findStyleRuleSource(baseURL, styleDeclaration, node) { let source = `${url.href}`; if (range) { + // `stylesheet` can be either an external file (stylesheet.startLine will allways be 0), + // or a +

SEO

diff --git a/lighthouse-core/gather/gatherers/seo/font-size.js b/lighthouse-core/gather/gatherers/seo/font-size.js index ca779b61f527..8ee72ba435d0 100644 --- a/lighthouse-core/gather/gatherers/seo/font-size.js +++ b/lighthouse-core/gather/gatherers/seo/font-size.js @@ -156,7 +156,9 @@ class FontSize extends Gatherer { .then(nodes => { const textNodes = nodes.filter(isNonEmptyTextNode); totalTextLength = textNodes.reduce((sum, node) => sum += getNodeTextLength(node), 0); - const visitedNodes = textNodes.slice(0, MAX_NODES_VISITED); + const visitedNodes = textNodes + .sort((a, b) => getNodeTextLength(b) - getNodeTextLength(a)) + .slice(0, MAX_NODES_VISITED); visitedTextLength = visitedNodes.reduce((sum, node) => sum += getNodeTextLength(node), 0); return visitedNodes; From 93efc7758894aba3212b41068ad3c337802f21bc Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Wed, 29 Nov 2017 22:24:33 +0100 Subject: [PATCH 52/65] Typo --- lighthouse-cli/test/fixtures/seo/seo-tester.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-cli/test/fixtures/seo/seo-tester.html b/lighthouse-cli/test/fixtures/seo/seo-tester.html index 66a63500ed35..3b5959e4ab10 100644 --- a/lighthouse-cli/test/fixtures/seo/seo-tester.html +++ b/lighthouse-cli/test/fixtures/seo/seo-tester.html @@ -34,7 +34,7 @@

Anchor text

click this

Small text

- +

1

2 34 From ae2c8239ceb70f3c46a1de216528f36bc0040c39 Mon Sep 17 00:00:00 2001 From: Konrad Dzwinel Date: Fri, 1 Dec 2017 01:30:56 +0100 Subject: [PATCH 53/65] =?UTF-8?q?Typo=20=F0=9F=94=8E?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lighthouse-core/audits/seo/font-size.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 218bc011b871..09de8809a5b7 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -89,7 +89,7 @@ function findStyleRuleSource(baseURL, styleDeclaration, node) { let source = `${url.href}`; if (range) { - // `stylesheet` can be either an external file (stylesheet.startLine will allways be 0), + // `stylesheet` can be either an external file (stylesheet.startLine will always be 0), // or a