new_audit(font-size-audit): legible font sizes audit #3533

kdzwinel · 2017-10-11T22:12:04Z

Corresponding issue - #3174

Failing audit (grouped by source, sorted by coverage):

*edge case - when font-size is inherited from parent and parent uses attributes style (e.g. <font size=1>) we get no info about where the styles came from. Seems like a DT bug:

Successful audit:

…in a table.

…ing selectors even if test passed. Clean up.

…yles of a parent node. Fixing linting errors, improvic jsdoc.

patrickhulce

WOW nicely done! very impressive how much is accounted for here, left no stone unturned!

I'm mildly concerned about the implications of gathering all the information might have on when we can include the audit by default, but some of that might be a trade-off @rviscomi has already thought about?

Getting the granular level of attribution we have here might make it excluded from default config/HTTP archive/etc :/

patrickhulce · 2017-10-12T16:50:35Z

lighthouse-cli/test/smokehouse/seo/expectations.js

@@ -49,6 +52,10 @@ module.exports = [
        score: false,
        displayValue: '403',
      },
+      'font-size': {
+        rawValue: false,


perhaps we can assert how many elements fail too? details: { items: { length: x }}

Here we fail because viewport is not configured (see debugString below), so we don't even go into evaluating text size and details will be empty. Viewport is empty on this page because of the viewport audit that we also want to fail here.

yeah I figured that out last time, but who knows what I was thinking 18 days ago :)

patrickhulce · 2017-10-12T16:51:53Z

lighthouse-core/audits/seo/font-size.js

+ * @return {{type:string, snippet:string}}
+ */
+function nodeToTableNode(node) {
+  const attributesString = node.attributes.map((value, idx) =>


how does this look in practice, are the attributes way too long? is there a subset we might be interested in?

good point, list of attributes may be long in some cases (e.g. schema.org metadata). However, limiting number of attributes shown to e.g. class and id, may be IMO confusing to the user as our representation of the node won't match the original one. This is especially problematic as ATM users have to find these nodes manually in their code/DOM (we don't link to the Element's panel from the report). If we want to limit number of attributes shown, we should at least expose node's xpath (just like a11y tests do) to make sure it is findable:

hm yeah I agree, I'm on board with the path to element on hover either way

patrickhulce · 2017-10-12T16:52:46Z

lighthouse-core/audits/seo/font-size.js

+ * @param {Node} node
+ * @returns {{source:!string, selector:string}}
+ */
+function getOrigin(stylesheets, baseURL, styleDeclaration, node) {


origin is a pretty overloaded term on the web, how about "findStyleRuleSource" or something?

patrickhulce · 2017-10-12T16:53:26Z

lighthouse-core/audits/seo/font-size.js

+      };
+    }
+
+    const totalTextLenght = getTotalTextLength(artifacts.FontSize);


s/Lenght/Length

patrickhulce · 2017-10-12T17:02:32Z

lighthouse-core/config/seo.js

+    passName: 'extraPass',
+    gatherers: [
+      'seo/font-size',
+      'styles',


unfortunate that this relies on the styles gatherer :/ do you foresee a way the audit could optionally rely on the styles gatherer output? i.e. still report the violations but maybe with less helpful identifiers? I'm not sure we're going to get to a place in DevTools where we want to add the extra pass by default and it'd be a shame to miss out on the whole audit

Only thing I need the styles gatherer for is stylesheetId -> stylesheet URL info. We can just say 'external stylesheet' if we don't have that mapping info, but I wonder - what's wrong with the 'styles' gatherer? Why does it need a separate run? Maybe I can collect stylesheetId -> stylesheet URL information in my gatherer separately and avoid requiring a separate run?

styles gatherer parses all the stylesheets on the page with gonzales which can be extremely slow in some cases (old runs of theverge spent ~20s in styles gatherer)

this seems like a useful split though for cases where you don't need the actual parsed content of stylesheets 👍

I've tried creating a styles-metadata gatherer that duplicates styles gatherer minus the parsing. However, I couldn't get styleSheetIds to match. font-size gatherer returned different IDs than styles-metadata gatherer for the exact same stylesheets. As far as I can see, this happens when you have two separate CSS.enable->CSS.disable runs. When I've put CSS.styleSheetAdded into the font-size gatherer (same CSS.enable->CSS.disable run) everything started to work.

Why this happens? No idea, possibly a Chrome bug. Why it worked when I used styles gatherer? This gatherer starts collecting stylesheet info in beforePass and finishes in afterPass, I can't do it in the default run because LH will complain that: "CSS domain enabled when starting trace".

I'm not a fan of keeping CSS.styleSheetAdded in the font-size gatherer, but don't see any other option at this point. Please let me know if you have any other ideas.

patrickhulce · 2017-10-12T17:09:48Z

lighthouse-core/gather/gatherers/seo/font-size.js

+      .then(() => getAllNodesFromBody(options.driver))
+      .then(nodes => nodes.filter(isNonEmptyTextNode))
+      .then(textNodes => Promise.all(
+        textNodes.map(node => getFontSizeInformation(options.driver, node))


this seems like it could have 1000s of inflight commands, 2 for each node in the body right?

any thoughts on limiting the protocol round-trips to try and compute as much as possible on browser side or do we absolutely need the node IDs for attribution :/

2 for each node in the body

For each non empty text node in the body, yes.

Yeah, I was also worried about that part, but interestingly it performs very well. I understand your concern though. If we would like to keep the number of sent commands down I do have some ideas:

get all computed styles in one shot with DOMSnapshot.getSnapshot (we should be able to connect info from that method with info from DOM.getDocument via backendNodeId )

inject a script that will collect font-size information in the browser context (but it will be tricky to connect that info with Node objects from DOM.getDocument)

only call getMatchedStylesForNode for nodes that have font-size below the threshold (but this would mean making the gatherer less generic)

filter nodes a bit more to only consider visible ones (figuring out what's visible might be tricky though)

Let me know what you think!

but interestingly it performs very well.

alright then works for me as is :) maybe throw a comment in there saying it works out fine because X so future people don't bother trying to fix?

I have renewed concern about the impact of this on pages with lots of DOM nodes :) I ran it on a few pages with a lot of elements I pulled from HTTPArchive and the gatherer took ~3-7s.

axe and event listeners audits are generally just as bad on the same sites, and it's probably a small fraction of sites where it's this bad, but we should try to be shrinking the list of super slow gatherers in the default set if possible.

What kind of impact do the 2nd and 3rd strategies you've proposed have on the runtime? If they're easy to explore, even in a hacky way it'd be nice to make an informed decision here :) A good example case for testing: https://www.flynashville.com/Pages/default.aspx

I did some testing (using url that you provided):

current solution

getting nodes: 631.190ms
getting font size info: 6445.445ms
whole gatherer: 7212.822ms

w/o CSS.getComputedStyleForNode

getting nodes: 717.872ms
getting font size info: 6005.998ms
whole gatherer: 6726.438ms

w/o CSS.getMatchedStylesForNode

getting nodes: 621.086ms
getting font size info: 215.401ms
whole gatherer: 839.306ms

CSS.getMatchedStylesForNode only for nodes with font-size < 16px

getting nodes: 576.719ms
getting font size info: 4676.480ms
whole gatherer: 5280.742ms

It looks like CSS.getMatchedStylesForNode is responsible for bad performance.

Ideas that we had were around getting nodes and computed styles for them. However, this doesn't seem to be a bottleneck. We will need some ideas for dealing with CSS.getMatchedStylesForNode. Calling it only for nodes below the threshold doesn't do the trick.

Wish there was something like https://chromedevtools.github.io/devtools-protocol/tot/CSS/#method-setEffectivePropertyValueForNode but for getting effective rules for given property. Or ability to make more specific CSS.getMatchedStylesForNode calls (e.g. tell that we only care about one property and only about effective rule).

As discussed with @patrickhulce right now - we will only call getMatchedStylesForNode for top X nodes with longest text. If website has more than X failing nodes user will get information at the end of the table that this is a partial result. I'll try to figure out what X value is to keep this audit under a second.

patrickhulce · 2017-10-12T17:10:48Z

lighthouse-core/gather/gatherers/seo/font-size.js

+ * @param {!Object} driver
+ * @returns {!Array<!Node>}
+ */
+function getAllNodesFromBody(driver) {


do you think you could modify

lighthouse/lighthouse-core/gather/driver.js

Lines 783 to 795 in 63beebe

/**

* Returns the flattened list of all DOM nodes within the document.

* @param {boolean=} pierce Whether to pierce through shadow trees and iframes.

* True by default.

* @return {!Promise<!Array<!Element>>} The found elements, or [], resolved in a promise

*/

getElementsInDocument(pierce = true) {

return this.sendCommand('DOM.getFlattenedDocument', {depth: -1, pierce})

.then(result => {

const elements = result.nodes.filter(node => node.nodeType === 1);

return elements.map(node => new Element({nodeId: node.nodeId}, this));

});

}

for your use case or is it much easier to work directly with the protocol as you're doing here?

it'd be nice to avoid collecting too many ad-hoc methods of retrieving DOM elements

bump for thoughts on this :)

getElementsInDocument filters text nodes out and puts everything into Elements (throwing away a lot of metadata returned by the DOM. getFlattenedDocument) so it was really hard for me to reuse it. Instead, I have extracted the this.sendCommand('DOM.getFlattenedDocument', {depth: -1, pierce}) part as a new driver method (getNodesInDocument) that getElementsInDocument depends on.

Ah ok, that's cool :)

s/Lenght/Length

…e run.

…for dynamically injected styles

paulirish

Totally epic PR. Love the results of this one.

two tiny nits. we're good to go after that.

paulirish · 2017-12-12T21:36:56Z

lighthouse-core/audits/seo/font-size.js

+      description: 'Document uses legible font sizes.',
+      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. ' +


it's a little funny to see this audit end up in passing even when there are failures (example). I know this is the same as other audits, but i'm thinking we could start explaining our passing cutoff in the helpText here.

Strive to have >75% of page text >=16px;

@vinamratasingal @kaycebasques hows that sound?

paulirish · 2017-12-12T21:40:08Z

lighthouse-core/audits/seo/font-size.js

+        (failingTextLength - analyzedFailingTextLength) / visitedTextLength * 100;
+
+      tableData.push({
+        source: 'Addtl illegible text',


abbreviation bikeshed!!

https://writingexplained.org/english-abbreviations/additional kinda recommends Add'l. That was the one I was expecting. let's do that.

kdzwinel · 2017-12-13T21:20:18Z

@paulirish both nits addressed. AppVeyor failure is not connected with this PR.

patrickhulce · 2017-12-14T18:53:48Z

woohoo!! great job @kdzwinel! 🎉 💯

kaycebasques · 2017-12-14T18:58:57Z

Cool, remind me to call out Konrad's contribution in the relevant "What's New In DevTools", when this (eventually) makes its way into the Audits panel

jitendravyas · 2018-01-09T21:41:48Z

Why is this placed in SEO audit section?

rviscomi · 2018-01-09T21:47:13Z

@jitendravyas having legible text is a big part of responsive web design, which plays a significant role in SEO.

Mobile pages that provide a poor searcher experience can be demoted in rankings or displayed with a warning in mobile search results.

https://developers.google.com/search/mobile-sites/mobile-seo/

jitendravyas · 2018-01-09T21:57:06Z

I know that tiny font sizes are not good but didn't know that Search engine crawler can detect the font size given in CSS file and it can affect ranking too. I used to think that Crawler check Content (HTML) only like Lynx browser see it.

Font size is actually an accessibility issue, I think it can be placed in Accessibility section.

But anyway, it's a good edition, though it will hard to keep 16px as a minimum size for any text, mainly in web apps.

rviscomi · 2018-01-09T22:03:47Z

Yeah, there are already some SEO audits that are borrowed from the accessibility section; there's a lot of overlap between the two. It's ok (and encouraged) to use the audits in whichever section is applicable because they should affect the aggregate score of the relevant audit categories.

I agree that 16px may be too ambitious. Analyzing the data in HTTP Archive, we're only seeing a ~20% pass rate for this audit. So we're looking into making necessary adjustments.

jitendravyas · 2018-04-08T05:51:24Z

Today I checked on lighthouse website and found that minimum font size has been changed from 16px to 12px. Where can I find the reason behind this change?
https://developers.google.com/web/tools/lighthouse/audits/font-sizes

rviscomi · 2018-04-08T06:09:00Z

Your previous comment summed it up nicely:

But anyway, it's a good edition, though it will hard to keep 16px as a minimum size for any text, mainly in web apps.

16px was too high a bar and 80% of pages tested were failing the audit. The dashboard on HTTP Archive shows that the audit is performing much better now with a more realistic bar:

jitendravyas · 2018-04-10T02:21:15Z

ok. I would like to know why 12px was decided why not 14?

Aim to have a font size of at least 12px on at least 60% of the text on your pages

Was it decided based on WCAG 2.0 or any other research or recommendation or just because 80% of pages tested were failing the audit?

rviscomi · 2018-04-10T04:48:01Z

12px/60% was chosen because we knew the audit would fail at a much more tolerable rate of ~25%. Of course, bigger text is better for legibility, but it's a subjective measurement. I'd be very interested to see more research into this space and we can adjust the audit calibration accordingly.

kdzwinel changed the title ~~feature(font-size-audit): Adding legible font sizes audit~~ new-audit(font-size-audit): legible font sizes audit Oct 11, 2017

kdzwinel added 17 commits October 12, 2017 00:31

Font Size audit - WIP

4cc44c1

Calculate percentage of ilegible text on page. Show failing elements …

ad797fc

…in a table.

Getting font-size information togheter with associated CSS rules - WIP

b2aa5c8

Matched rules - WIP

64360dc

Extracting style info - WIP

3f2effc

Expose effective rule

fdf6473

Show what percentage of text each rule affected. Show table with fail…

35599d3

…ing selectors even if test passed. Clean up.

Work on inline styles, clean up jsdoc

483a5c2

Handle inline styels, attribute styles and user agent styles.

9a10a10

Expose stylesheet URL, line number and column.

4b0863c

Update output table according to the latst decisions.

005d308

Changes to the result table. Clean up.

a4ec14e

Gatherer test.

7298ae6

Smoke tests. Fixing edgecase when size is inherited from attribute st…

acb9aa8

…yles of a parent node. Fixing linting errors, improvic jsdoc.

Add unit tests.

a61106e

Remove info about color prop.

eaf454b

Remove debugMessage from passing test expectations as it doesn't exist.

6d1b518

kdzwinel force-pushed the seo-font-size-rdp branch from ccc68cc to 6d1b518 Compare October 11, 2017 22:52

paulirish changed the title ~~new-audit(font-size-audit): legible font sizes audit~~ new_audit(font-size-audit): legible font sizes audit Oct 11, 2017

kdzwinel added 2 commits October 12, 2017 11:05

Replace URL with parseURL

e07a3f8

Make linter happy.

242024d

patrickhulce suggested changes Oct 12, 2017

View reviewed changes

kdzwinel added 3 commits October 16, 2017 00:01

Check number of failing items in the smoke test.

3c66033

getOrigin -> findStyleRuleSource

99d89ba

s/Lenght/Length

Collect stylesheet metadata in font-size gatherer. Get rid of separat…

018e2bf

…e run.

paulirish modified the milestones: Sprint Uno: Oct 2-13, Sprint Dos: Oct 16-27 Oct 16, 2017

Font Size audit - WIP

4e1f171

kdzwinel added 2 commits December 7, 2017 16:59

Update copy, remove audits from seo config, show 'dynamic' as source …

ede0864

…for dynamically injected styles

Fix test

a588863

kdzwinel force-pushed the seo-font-size-rdp branch from e298c13 to a588863 Compare December 7, 2017 16:14

kdzwinel added 2 commits December 7, 2017 21:34

Fix tests

06443e5

Add simple selector to nodes in the details table.

88091d7

kdzwinel mentioned this pull request Dec 8, 2017

Improve node selectors shown in the details table #4015

Closed

vinamratasingal-zz modified the milestones: Sprint Cinco: November 28 - Dec 9, Sprint Seis: December 11 - 22 Dec 11, 2017

paulirish approved these changes Dec 12, 2017

View reviewed changes

kdzwinel added 3 commits December 12, 2017 23:36

Add'l 🚲🏠

5a71a58

Fix tests.

3223cb5

Additional info in the helpText

0b0cb9a

kdzwinel mentioned this pull request Dec 14, 2017

core(config): show SEO audits in the UI #4057

Merged

patrickhulce merged commit 9ef858a into GoogleChrome:master Dec 14, 2017

dependencies bot mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

paulirish removed the waiting4reviewer label Mar 6, 2018

patrickhulce mentioned this pull request Oct 30, 2018

core(font-size): speed up gatherer #6391

Closed

lemcardenas mentioned this pull request Aug 24, 2020

OOM in ImageElements gatherer #11289

Closed

connorjclark mentioned this pull request Jan 11, 2021

Cannot read property 'trim' of undefined #11606

Closed

	/**
	* Returns the flattened list of all DOM nodes within the document.
	* @param {boolean=} pierce Whether to pierce through shadow trees and iframes.
	* True by default.
	* @return {!Promise<!Array<!Element>>} The found elements, or [], resolved in a promise
	*/
	getElementsInDocument(pierce = true) {
	return this.sendCommand('DOM.getFlattenedDocument', {depth: -1, pierce})
	.then(result => {
	const elements = result.nodes.filter(node => node.nodeType === 1);
	return elements.map(node => new Element({nodeId: node.nodeId}, this));
	});
	}

new_audit(font-size-audit): legible font sizes audit #3533

new_audit(font-size-audit): legible font sizes audit #3533

Conversation

kdzwinel commented Oct 11, 2017 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

current solution

w/o CSS.getComputedStyleForNode

w/o CSS.getMatchedStylesForNode

CSS.getMatchedStylesForNode only for nodes with font-size < 16px

Choose a reason for hiding this comment

patrickhulce Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel commented Dec 13, 2017 • edited Loading

patrickhulce commented Dec 14, 2017

kaycebasques commented Dec 14, 2017

jitendravyas commented Jan 9, 2018

rviscomi commented Jan 9, 2018

jitendravyas commented Jan 9, 2018 • edited Loading

rviscomi commented Jan 9, 2018

jitendravyas commented Apr 8, 2018 • edited Loading

rviscomi commented Apr 8, 2018

jitendravyas commented Apr 10, 2018

rviscomi commented Apr 10, 2018

kdzwinel commented Oct 11, 2017 •

edited

Loading

kdzwinel Oct 12, 2017 •

edited

Loading

kdzwinel Oct 15, 2017 •

edited

Loading

kdzwinel Oct 12, 2017 •

edited

Loading

kdzwinel Oct 31, 2017 •

edited

Loading

patrickhulce Oct 12, 2017 •

edited

Loading

kdzwinel commented Dec 13, 2017 •

edited

Loading

jitendravyas commented Jan 9, 2018 •

edited

Loading

jitendravyas commented Apr 8, 2018 •

edited

Loading