Change Audit Descriptions based on Audit Results #2478

evenstensberg · 2017-06-09T14:50:17Z

this PR was started in #2418 and then moved here.

patrickhulce

thanks again! using fallbackDescription and audit logic looks a-ok to me

patrickhulce · 2017-06-09T18:18:59Z

lighthouse-core/audits/viewport.js

@@ -17,6 +17,8 @@ class Viewport extends Audit {
      category: 'Mobile Friendly',
      name: 'viewport',
      description: 'Has a `<meta name="viewport">` tag with `width` or `initial-scale`',
+      fallbackDescription: 'Does not have a `<meta name="viewport">` tag with `width` ' +
+      'or `initial-scale`',


nit: indentation

patrickhulce · 2017-06-09T18:20:09Z

lighthouse-core/audits/accessibility/video-description.js

@@ -21,6 +21,8 @@ class VideoDescription extends AxeAudit {
      category: 'Accessibility',
      name: 'video-description',
      description: '`<video>` elements contain a `<track>` element with `[kind="description"]`.',
+      fallbackDescription: '`<video>` elements do not contain a `<track>` element with ' +
+      '`[kind="description"]`.',


nit: indentation

patrickhulce · 2017-06-09T18:20:11Z

lighthouse-core/audits/accessibility/video-caption.js

@@ -21,6 +21,8 @@ class VideoCaption extends AxeAudit {
      category: 'Accessibility',
      name: 'video-caption',
      description: '`<video>` elements contain a `<track>` element with `[kind="captions"]`.',
+      fallbackDescription: '`<video>` elements do not contain a `<track>` element ' +
+      'with `[kind="captions"]`.',


nit: indentation

patrickhulce · 2017-06-09T18:20:17Z

lighthouse-core/audits/accessibility/th-has-data-cells.js

@@ -22,6 +22,8 @@ class THHasDataCells extends AxeAudit {
      name: 'th-has-data-cells',
      description: '`<th>` elements and elements with `[role="columnheader"/"rowheader"]` have ' +
          'data cells they describe.',
+      fallbackDescription: '`<th>` elements and elements with ' +
+      '`[role="columnheader"/"rowheader"]` do not have data cells they describe.',


nit: indentation

patrickhulce · 2017-06-09T18:20:19Z

lighthouse-core/audits/accessibility/td-headers-attr.js

@@ -23,6 +23,8 @@ class TDHeadersAttr extends AxeAudit {
      name: 'td-headers-attr',
      description: 'Cells in a `<table>` element that use the `[headers]` attribute only refer ' +
          'to other cells of that same table.',
+      fallbackDescription: 'Cells in a `<table>` element that use the `[headers]` ' +
+      'attribute refers to other cells of that same table.',


nit: indentation

patrickhulce · 2017-06-09T18:20:33Z

lighthouse-core/audits/accessibility/html-lang-valid.js

@@ -21,6 +21,8 @@ class HTMLLangValid extends AxeAudit {
      category: 'Accessibility',
      name: 'html-lang-valid',
      description: '`<html>` element has a valid value for its `[lang]` attribute.',
+      fallbackDescription: '`<html>` element does not have a valid value for ' +
+      'its `[lang]` attribute.',


nit: indentation

patrickhulce · 2017-06-09T18:20:41Z

lighthouse-core/audits/accessibility/definition-list.js

-          'or <template> elements.',
+          'or `<template>` elements.',
+      fallbackDescription: '`<dl>`\'s do not contain only properly-ordered `<dt>` and `<dd>` ' +
+      'groups, `<script>` ' +


nit: indentation, can collapse these two lines too

patrickhulce · 2017-06-09T18:20:54Z

lighthouse-core/audits/accessibility/color-contrast.js

@@ -22,6 +22,8 @@ class ColorContrast extends AxeAudit {
      category: 'Accessibility',
      name: 'color-contrast',
      description: 'Background and foreground colors have a sufficient contrast ratio.',
+      fallbackDescription: 'Background and foreground colors do not have a ' +
+      'sufficient contrast ratio.',


nit: indentation

patrickhulce · 2017-06-09T18:20:58Z

lighthouse-core/audits/accessibility/audio-caption.js

@@ -21,6 +21,8 @@ class AudioCaption extends AxeAudit {
      category: 'Accessibility',
      name: 'audio-caption',
      description: '`<audio>` elements contain a `<track>` element with `[kind="captions"]`.',
+      fallbackDescription: '`<audio>` elements are missing a `<track>` element with ' +
+      '`[kind="captions"]`.',


nit: indentation

patrickhulce · 2017-06-09T18:21:03Z

lighthouse-core/audits/accessibility/aria-required-children.js

-      description: '`[role]`s that require child `[role]`s contain them.',
+      description: 'Elements with `[role]` that require specific children `[role]`s, are present.',
+      fallbackDescription: 'Elements with `[role]` that require specific children `[role]`s, ' +
+      'are missing.',


nit: indentation

evenstensberg · 2017-06-09T18:30:39Z

yarn lint didn't complain in my tests, anything I need to run in order to get corrections?

patrickhulce · 2017-06-09T20:20:47Z

yarn lint didn't complain in my tests, anything I need to run in order to get corrections?

unfortunately eslint's indent support is quite limited, one day we'll just say run prettier and problem solved but for now it's a human endeavor :/

evenstensberg · 2017-06-10T13:57:23Z

All done now, tell me if you need an addition to the new feature on password pasting 👍

kaycebasques · 2017-06-12T16:35:42Z

Here's the "docs journey" that I want to protect:

User fails an audit.
User clicks a link to learn more about how to fix.
The name of the doc matches the name of the audit that they failed.

So, it seems that users will see the fallbackDescription when they fail an audit. Is that correct? If so, I need to make sure that all the docs match the new fallbackDescription texts

kaycebasques · 2017-06-12T16:41:18Z

lighthouse-core/audits/accessibility/list.js

@@ -22,6 +22,8 @@ class List extends AxeAudit {
      name: 'list',
      description: 'Lists contain only `<li>` elements and script supporting elements ' +
          '(`<script>` and `<template>`).',
+      fallbackDescription: 'Lists do not contain only `<li>` elements and script ' +
+          'supporting elements (`<script>` and `<template>`).',


Side note: In this iteration I see that we're just inversing the description. But I think sometimes that leads to clunky sentences. We can make the fallbacks more readable by rephrasing them:

E.g. this one could be changed to Lists contain invalid elements

I'm not asking you to do this now, just noting it

Yeah that's what I did initially. We be a bit more expressive though?

Right. I'd hate to lose the full description when someone fails. That's when they need the most help :) Most of the time, users will start off failing the audit. I think it will also be quite confusing if the two descriptions change a lot. People are going to be like "where did that audit go that I was just failing"?

Need a suggestion if I'm to change this 👍

No change needed (from me, at least). Just discussing

ebidel · 2017-06-12T18:46:36Z

@ev1stensberg can you rebase?

evenstensberg · 2017-06-12T19:00:07Z

Done

brendankenny

bikeshed: rename property as failureDescription? "fallback" isn't very specific

brendankenny · 2017-06-12T19:05:12Z

lighthouse-core/audits/audit.js

-
+    let auditDescription = audit.meta.description;
+    if (audit.meta.fallbackDescription) {
+      if (!result.rawValue || (typeof result.rawValue === 'number' && result.rawValue < 100)) {


Maybe I missed this in the last PR review, but shouldn't this be based on score?

Yup, and DOM size is invalid cause of that, one of the things that needed discussion. I change the code accordingly to @patrickhulce 's last comment on the other branch

Evidently I really wanted to use the labels that are used, but that happens later

Changed in new commit

yeah we should really be doing that step much earlier than in report-generator.js so things like this don't happen

patrickhulce · 2017-06-12T19:24:21Z

lighthouse-core/audits/audit.js

-
+    let auditDescription = audit.meta.description;
+    if (audit.meta.fallbackDescription) {
+      if (!result.rawValue || (typeof result.rawValue === 'number' && result.score < 100)) {


@ev1stensberg I believe brendan intended this entire expression to be based on score the local variable since that already does the correct use of result.rawValue if a score isn't set

Pushed a new commit, is that what you meant? Sorry, was a bit unsure when I review a second time!

evenstensberg · 2017-06-13T23:42:58Z

@brendankenny Done now, I agree!

paulirish · 2017-07-07T19:14:49Z

@ev1stensberg can you help us out with rebasing this?

once that's done we can merge.

evenstensberg · 2017-07-07T19:23:41Z

🇲🇰

googlebot · 2017-07-07T23:36:39Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

evenstensberg mentioned this pull request Jun 9, 2017

Provide two audit descriptions for use if they pass or fail #1611

Closed

patrickhulce reviewed Jun 9, 2017

View reviewed changes

patrickhulce requested a review from kaycebasques June 9, 2017 18:22

kaycebasques approved these changes Jun 12, 2017

View reviewed changes

brendankenny reviewed Jun 12, 2017

View reviewed changes

patrickhulce reviewed Jun 12, 2017

View reviewed changes

evenstensberg mentioned this pull request Jun 16, 2017

Confusion in Accessibility audit #2387

Closed

evenstensberg added 5 commits July 7, 2017 21:21

initial Commit

d782e84

proper indent

0844fc8

use score not rawValue

4a96e6c

base on score

1a5db42

use failureDescription

1d311bc

paulirish added 2 commits July 7, 2017 16:35

config test to assert for missing failureDescription.

730ef17

add missing failureDescription for two remaining audits

f780bba

paulirish approved these changes Jul 7, 2017

View reviewed changes

paulirish merged commit b0df777 into GoogleChrome:master Jul 7, 2017

This was referenced Jul 7, 2017

Wording issue --> "won't" should be "will" #2311

Closed

Invert the report form for the discouraged API use #1613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Audit Descriptions based on Audit Results #2478

Change Audit Descriptions based on Audit Results #2478

evenstensberg commented Jun 9, 2017 •

edited by paulirish

Loading

patrickhulce left a comment

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

patrickhulce Jun 9, 2017

evenstensberg commented Jun 9, 2017

patrickhulce commented Jun 9, 2017

evenstensberg commented Jun 10, 2017

kaycebasques commented Jun 12, 2017 •

edited

Loading

kaycebasques Jun 12, 2017

evenstensberg Jun 12, 2017

ebidel Jun 12, 2017

evenstensberg Jun 12, 2017

kaycebasques Jun 12, 2017 •

edited

Loading

ebidel commented Jun 12, 2017

evenstensberg commented Jun 12, 2017

brendankenny left a comment

brendankenny Jun 12, 2017

evenstensberg Jun 12, 2017

evenstensberg Jun 12, 2017

patrickhulce Jun 12, 2017

patrickhulce Jun 12, 2017

evenstensberg Jun 12, 2017

evenstensberg commented Jun 13, 2017

paulirish commented Jul 7, 2017

evenstensberg commented Jul 7, 2017

googlebot commented Jul 7, 2017

Change Audit Descriptions based on Audit Results #2478

Change Audit Descriptions based on Audit Results #2478

Conversation

evenstensberg commented Jun 9, 2017 • edited by paulirish 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

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

evenstensberg commented Jun 9, 2017

patrickhulce commented Jun 9, 2017

evenstensberg commented Jun 10, 2017

kaycebasques commented Jun 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

kaycebasques Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

ebidel commented Jun 12, 2017

evenstensberg commented Jun 12, 2017

brendankenny 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg commented Jun 13, 2017

paulirish commented Jul 7, 2017

evenstensberg commented Jul 7, 2017

googlebot commented Jul 7, 2017

evenstensberg commented Jun 9, 2017 •

edited by paulirish

Loading

kaycebasques commented Jun 12, 2017 •

edited

Loading

kaycebasques Jun 12, 2017 •

edited

Loading