Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[SEO Audits] Document uses legible font sizes #3174

Closed
rviscomi opened this issue Aug 29, 2017 · 33 comments
Closed

[SEO Audits] Document uses legible font sizes #3174

rviscomi opened this issue Aug 29, 2017 · 33 comments

Comments

@rviscomi
Copy link
Member

rviscomi commented Aug 29, 2017

Audit group: Mobile friendly
Description: Document uses legible font sizes
Failure description: Document does not use legible font sizes: <body> font-size {#} px (target: >= 16 px)
Help text: Font sizes less than 16px are too small to be legible and require mobile visitors to “pinch to zoom” in order to read. Read more about font size best practices in Use Legible Font Sizes.

Success condition: No font-size style detected on the body, or it exists and is set to at least 16px.

Notes

  • Alternate approaches could be to query the DOM for the first <p> (or similar) element and test if its font size is >= 16px. Or the audit could scan the entire DOM for text < 16px and the result could be the % of text that meets the size requirements, similar to the PSI source.
  • If the font-size style is omitted from the body, we can assume it inherits the default font-size of 16px from the UA stylesheet.
  • The detected font size is included in the failure description.
@rviscomi rviscomi changed the title [SEO Audit] Avoids small font sizes [SEO Audits] Avoids small font sizes Aug 29, 2017
@patrickhulce patrickhulce added this to the SEO health-check audits milestone Aug 29, 2017
@paulirish
Copy link
Member

query the DOM for the first

(or similar) element...

I think we may want to list ALL offending elements. PSI did have a really comprehensive approach on this one, so using basing this off their work sgtm.

@rviscomi
Copy link
Member Author

I'll have to double check their scoring criteria, but the PSI doc seems to point to 16px just being the baseline from which small/large variations are allowed. Eg: 12, 16, 20px. So should sparse 12px text be flagged as invalid? We could also do something like "> X% of text is < 16px" basing off of string length instead of DOM count.

@rviscomi rviscomi changed the title [SEO Audits] Avoids small font sizes [SEO Audits] Document avoids small font sizes Sep 6, 2017
@kdzwinel
Copy link
Collaborator

kdzwinel commented Sep 10, 2017

PSI doesn't show me a font size warning no matter what (e.g. this page with font-size: 1px). Maybe they no longer check that?

Mobile-Friendly test was a bit more helpful (I had to solve a lot of captchas though). It shows the font size warning when all text is <=8px, but it's enough for one paragraph to be >8px for the test to pass (e.g. this example).

However, that's only me poking a blackbox. Their audit may be also taking line-height, viewport settings, length of text etc. into consideration, and I haven't tested those.

I think that people may expect this lighthouse audit to work the same way as the corresponding mobile-friendly test audit does. It'd be good to know their exact criteria. Is it even possible to get that info?

@patrickhulce
Copy link
Collaborator

The PSI font-size check is far more complicated than their public document suggests and attempts to account for a host of crazy things. Sticking to a basic font-size property check and allowing a minimum threshold of small text sounds like a good starting point. We should probably also fail this check if mobile viewport isn't already set since even 16px font sizes will be way too small if the viewport is desktop-sized.

@rviscomi
Copy link
Member Author

We should probably also fail this check if mobile viewport isn't already set since even 16px font sizes will be way too small if the viewport is desktop-sized.

There's already a separate audit for the meta viewport, so should this one still fail? IMO failing this will obscure the real cause (viewport) and may lead people to dig into font-size which isn't necessarily broken. But it's true that without a proper viewport config, text will be illegible, so I can see it both ways.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Sep 11, 2017

There's already a separate audit for the meta viewport, so should this one still fail?

There's a precedent in the PWA audits to rely on a check in another audit (just by require('./the-other-audit')) and fail with a debugString that lets them know Text is illegible because of a missing viewport config or w/e which seems fine to do here as well if we explain the causes in the audit "Learn More" link.

@rviscomi
Copy link
Member Author

Descriptive debugString SGTM

@rviscomi rviscomi changed the title [SEO Audits] Document avoids small font sizes [SEO Audits] Document uses legible font sizes Sep 12, 2017
@kdzwinel
Copy link
Collaborator

@patrickhulce do you have something like this in mind?

const hasServiceWorker = SWAudit.audit(artifacts).rawValue;
if (!hasServiceWorker) {
result.failures.push('Site does not register a service worker');
}

@patrickhulce
Copy link
Collaborator

@kdzwinel yeah that's exactly what I was thinking about :) not the prettiest, but easy to reuse the logic you need for now

@kdzwinel
Copy link
Collaborator

I have a first version of this audit which:

  • fails if vieport isn't correctly set

screen shot 2017-09-13 at 23 58 57

  • checks if font-size on body >= 16px (original "Success condition")

screen shot 2017-09-14 at 00 00 10

Couple of questions:

  • I had to rework the failure description so that the value of the font-size found is at the end. It doesn't work well for the viewport case though (see the screenshot - "found: "). Any ideas how to work around it?
  • <body> font-size >= 16px fails for google.com (13px), amazon.com (15px), facebook.com (14px) - should we lower the threshold? Or maybe find all failing elements instead of focusing on <body> (as Paul suggested)?

@rviscomi
Copy link
Member Author

rviscomi commented Sep 14, 2017

I was thinking that the entire <body> font-size {#} px (target: >= 16 px) text could be the displayValue, with the detected font size embedded in it. That way it could be included only when it makes sense.

Interesting that those big sites fail the audit. I think the advice is still valid and we shouldn't necessarily lower the bar. I'm open to changing it up so it's not all about the <body>. My suggestion earlier was to have a limit like 90+% of text must be at least 16px. It'd also be nice if it was based on string length (what the user can actually see) rather than element count.

Rough outline of how it could work:

  • use a tree walker to get all text nodes
  • map text nodes to font sizes (getComputedStyle(text.parentElement)['font-size']) and text lengths
  • reduce to the % >= 16px
  • fail if % < 90

@rviscomi
Copy link
Member Author

I couldn't resist making a proof of concept.

const textNodes = [];
const walker = document.createTreeWalker(document.body, NodeFilter.SHOW_TEXT, null, false);
let node;
while(node = walker.nextNode()) textNodes.push(node);

const {valid, total} = textNodes.filter(textNode => {
    return textNode.parentElement
}).map(textNode => {
    return {
        length: textNode.textContent.trim().length,
        fontSize: parseInt(getComputedStyle(textNode.parentElement)["font-size"])
    };
}).reduce((soFar, {length, fontSize}) => {
    soFar.total += length;
    if (fontSize >= 16) soFar.valid += length;
    return soFar;
}, {
    total: 0,
    valid: 0
});

console.log('Percent valid (>=16px):', (valid/total).toFixed(2))

For example, https://mobile.nytimes.com is 85% valid. MDN is 77% valid. Gmail is actually <1% on mobile but 90% on desktop.......

Maybe based on those tests, the passing threshold could even be 75%.

@kdzwinel
Copy link
Collaborator

SGTM! Also, awesome POC - I didn't know about the treeWalker API.

How about making this audit non-binary and associating a score depending on the % of text < 16px?

@kdzwinel
Copy link
Collaborator

After some experiments I propose couple of improvements:

  • use getElementsInDocument helper from dom-helpers as it pierces through shadow DOM
  • trim value of the text nodes, as there is usually a lot of formatting whitespace between the tags in HTML
  • use existing table component (as shown below) to list all nodes with illegible text (two columns: selector/xpath and font size in px). Sort the results so that smallest text is at the top.

screen shot 2017-09-15 at 23 52 27

I've been also thinking about excluding text which is not visible - e.g. because in RWD a lot of text that is visible for desktop gets hidden on mobile via media queries. This might be tricky to get right though (besides display:none and visibility:hidden there is also text-indent, opacity, transforms etc. itd.). @rviscomi WDYT?

@rviscomi
Copy link
Member Author

Do you have a link to docs on getElementsInDocument? Curious to learn more about it but it's not appearing in search results.

A table sounds like a good addition. A couple of things to watch out for:

  • There may be thousands of text nodes and some kind of truncation would be needed.
  • Users might get the impression that the way to remediate the audit is to focus on each individual span and tweak its font size. The underlying issue is the style rule that affects hundreds of elements, so a table of the most influential font-size rules would go a long way.

Invisible text sounds like a good edge case. I'm inclined to note the caveat in the docs and iterate on it later. Have you looked into testing for visibility with the Intersection Observer API? Not sure if that'll be too slow at this scale.

@kdzwinel
Copy link
Collaborator

Do you have a link to docs on getElementsInDocument?

Sure, getElementsInDocument is a build-in lighthouse helper:

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/lib/dom-helpers.js#L20

There may be thousands of text nodes and some kind of truncation would be needed.

Good point, although as far as I can see, existing audits don't have any truncation/pagination (even though they show a lot of results):

screen shot 2017-09-17 at 16 15 00

It might be a good general feature for the table to add pagination.

table of the most influential font-size rules

Yeah, that'd be much more useful. I've tried getting that information using web APIs (getMatchedCSSRules / getComputedStyles), but it'd be hard/impossible to get that right. Thankfully, we have access to the DevTools Protocol and we can call getMatchedStylesForNode. This will probably require to dump the whole getElementsInDocument solution and do everything using DevTools Protocol. I'll work on that.

Have you looked into testing for visibility with the Intersection Observer API?

Intersection Observer will work for display:none and position:absolute, but won't work for transforms, text-indent and opacity (but maybe that's good enough?). It also limited to the current viewport (but we can always resize the viewport). Not sure about the performance either, but we can try it out in the second iteration 👍

@rviscomi
Copy link
Member Author

Oooh getMatchedStylesForNode looks promising. Looking forward to seeing it in action.

@patrickhulce do you have any advice for dealing with extra large tables/lists of debug info? Should we explore the pagination route or just truncate?

@kdzwinel
Copy link
Collaborator

FYI I've got version showing nodes on seo-font-size branch
screen shot 2017-09-18 at 10 22 59

Yesterday, I started experimenting with getMatchedStylesForNode and it won't be as easy as I thought it will. The thing is, getMatchedStylesForNode gives you all matching rules, but doesn't tell you which rule is the winning one. It's also a bit problematic to get stylesheet URL based on stylesheet ID - there is no method for that in DT protocol, you have to listen to all stylesheets being loaded while page is loading and store ID -> URL relation yourself. Anyway, DevTools are showing that info:
screen shot 2017-09-18 at 10 28 55
so it's definitely possible (I might even be able to reuse some of their code). It will just take more time than anticipated.

@kdzwinel
Copy link
Collaborator

kdzwinel commented Sep 26, 2017

FYI I made some progress (these are selectors from CSS rules on the left):
screen shot 2017-09-26 at 17 00 44

I have to handle inline styles and make these rules clickable so that they open appropriate files in the Sources pane.

@rviscomi
Copy link
Member Author

Nice!! Great work!

What order are the selectors in?

@kdzwinel
Copy link
Collaborator

ATM random but I can sort them either by font size or amount of text affected. WDYT?

@rviscomi
Copy link
Member Author

I like sorting by amount of text affected, most first. Could we also have a column for the % of text affected by that rule? That should make it clear how the list is sorted and quantify how big of an impact each rule has.

Would this table be available even when the audit passes, but there are still instances of small text?

@kdzwinel
Copy link
Collaborator

Updates:

  • "% of text affected" added,
  • table sorted by this value,
  • table shown even if audit passes.

Failure - gmail.com :
screen shot 2017-09-28 at 17 56 21

Success - brainly.com :
screen shot 2017-09-28 at 17 56 30

(details are not expanded by default)

@paulirish
Copy link
Member

I emailed some details and feedback about our implementation off-thread to konrad and rick. :)

@kdzwinel
Copy link
Collaborator

kdzwinel commented Oct 4, 2017

Update:

  • inline styles working,
  • attributes working,
  • and I'm also exposing location URL + line:column info.

version3

Unfortunately, ATM there is no way to link to resources (#3457) so I've tried exposing everything in a details table. As you can see, we have a bit of information overload now (screenshot was done in a full window, in DT everything will be even more cramped). I propose we drop line + column since:

  • IMO it's least useful and
  • ATM it's only accurate in case of an external stylesheet (for inline styles and embedded stylesheets it shows relative, instead of absolute, values, so it'd require some more work to get right).

@rviscomi
Copy link
Member Author

rviscomi commented Oct 5, 2017

After playing with devtools some more, I've refined my idea of the optimal user experience. Rather than linking to the Sources panel to highlight the line of code containing the applicable style, I think the link should go to the Elements panel. This is consistent with how the "Computed" tab works.

image

Clicking the circled arrow next to "12px" jumps to the "Styles" tab and has a brief yellow highlight behind the font-size style.

image

I like this experience better for the audit results for a couple of reasons: the familiarity of inspecting styles (inline, stylesheet, etc) on the Elements > Styles panel, and taking the user directly to the place where they could change the font-size to immediately see how it affects the page. This area also includes a link to the resource/line/column in the Sources panel to give an idea of where the change needs to be made to the source code.

Until the ability to link to other panels is fixed, the next best experience IMO would be to provide the resource/line/column information. So there would be 4 columns:

  • Source
    • eg foo.css:10:5
  • Selector
    • eg .f6 or element.style if inline
  • Coverage
    • eg 35.48%
  • Font Size
    • eg 10px

Not sure if the html file would be referenced in the source column only for style blocks or inline styles. If it's not available for whatever reason, it's ok to leave blank.

@rviscomi
Copy link
Member Author

rviscomi commented Oct 5, 2017

To throw another wrench into this audit, one detail that was discussed briefly during the meeting earlier was that legibility is not just dependent on font size but also font family and language. For example, Thai has dense characters so a larger font than usual may be necessary for legibility. There are also some fonts that are only designed for a minimum size, and below that text is hard to read.

I don't think we can algorithmically account for everything, but maybe there are some heuristics we can take away from it. For example, if we detect that the page is a particular language like Thai or Vietnamese, we may want to consider increasing the minimum font size of the audit. Whatever we do, it's important to make sure that the documentation is clear about how the audit works and any differences between pages. That said, the best thing may just be consistency.

We will have a better idea of things when we release the calibration survey, so this is just food for thought now.

@kdzwinel
Copy link
Collaborator

@rviscomi I got the table to look as you suggested:
screen shot 2017-10-10 at 23 25 47

(BTW I also discovered that we need to handle <small> - see last element in the table above)

However, the issue is that for inline styles and <font size=1> it doesn't seem to be possible to get corresponding lines/columns (probably because we operate on a DOM, not HTML). Without that, we leave developers with almost nothing (see rows 3 and 4 above). IMO we should either:

  • show DOM nodes

screen shot 2017-10-10 at 23 25 55

  • or group all these inline+attributes rows together and call them e.g. "element.style" (like DevTools do)

@rviscomi
Copy link
Member Author

rviscomi commented Oct 10, 2017

I'm in favor of your first option, although we should keep / (or /index.html or whatever) as the source even if we don't have line/column and show the nodes under the selector column. I also like the idea of keeping them on separate rows to have finer granularity.

@bennidhamma
Copy link

The original link in the help text "Use Legible Font Sizes" points to the URL

https://developers.google.com/speed/docs/insights/UseLegibleFontSizes

but now it points to this url:

https://developers.google.com/web/fundamentals/design-and-ux/responsive/#optimize_text_for_reading

Material Design's guidelines state that 14sp (which AFAICT maps to 14px) is the recommended size for paragraph text:

https://learnui.design/blog/android-material-design-font-size-guidelines.html

This feels inconsistent and we're not sure which guideline to follow. We don't want to get scored for search result position, but we also agree with the Material Design guidelines that 14sp/px is actually a more optimal size for paragraph text.

Would you consider changing this audit rule to 14px instead of 16px? Or perhaps provide an explanation or any insight and help clear up our confusion?

thanks!

@bennidhamma
Copy link

Sorry, I didn't realize the material link was a third party analysis. This link: https://material.io/design/typography/the-type-system.html#type-scale shows a few of the sizes in the type scale at 14, specifically Body2 and Subtitle2 and BUTTON.

Still confused, this audit seems to contradict the recommendations of the Material Design type scale...

@rviscomi
Copy link
Member Author

This audit has since been adjusted to recommend at least 12px font 60% of the time. Still not in sync with the MD guideline you cited but more permissive.

@bennidhamma
Copy link

bennidhamma commented Jun 21, 2018 via email

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

No branches or pull requests

6 participants