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

Lighthouse 3 Regression in Reporting Data #5402

Closed
joelhsmith opened this issue Jun 1, 2018 · 15 comments · Fixed by #5439
Closed

Lighthouse 3 Regression in Reporting Data #5402

joelhsmith opened this issue Jun 1, 2018 · 15 comments · Fixed by #5439

Comments

@joelhsmith
Copy link
Contributor

Accessibility Meta is missing in Lighthouse 3

Provide the steps to reproduce

Run Lighthouse 2.9.4 on any website using the cli.

For my example I am running a simple report on http://cats.com.

To simply the example, you can run this, but it is not necessary:

'use strict';

/**
 * Config file for running A11y audits.
 */
module.exports = {
  extends: 'lighthouse:default',
  settings: {
    onlyAudits: ['color-contrast'],
  },
};

If there is a contrast error, Lighthouse 2.9 provides information about the current failing element.
For my example I am showing the contrast ratio. Notice in failureSummary it calculates the current ratio and advises what contrast ratio needs met to pass.

"failureSummary": "Fix any of the following:\n Element has insufficient color contrast of 2.32 (foreground color: #ffffff, background color: #aaaaaa, font size: 9.8pt, font weight: normal). Expected contrast ratio of 4.5:1",

Example result:

  "extendedInfo": {
        "value": {
          "id": "color-contrast",
          "impact": "serious",
          "tags": [
            "cat.color",
            "wcag2aa",
            "wcag143"
          ],
          "description": "Ensures the contrast between foreground and background colors meets WCAG 2 AA contrast ratio thresholds",
          "help": "Elements must have sufficient color contrast",
          "helpUrl": "https://dequeuniversity.com/rules/axe/3.0/color-contrast?application=axeAPI",
          "nodes": [
            {
              "impact": "serious",
              "html": "<input type=\"submit\" value=\"Subscribe\" name=\"subscribe\" id=\"mc-embedded-subscribe\" class=\"button\">",
              "target": [
                "#mc-embedded-subscribe"
              ],
              "failureSummary": "Fix any of the following:\n  Element has insufficient color contrast of 2.32 (foreground color: #ffffff, background color: #aaaaaa, font size: 9.8pt, font weight: normal). Expected contrast ratio of 4.5:1",
              "path": "0,HTML,2,BODY,0,DIV,0,FORM,0,DIV,4,DIV,0,INPUT",
              "snippet": "<input type=\"submit\" value=\"Subscribe\" name=\"subscribe\" id=\"mc-embedded-subscribe\" class=\"button\">"
            }
          ]
        }
      },

Full Gist here

Yay! Lots of important meta.

But that meta has been removed in Lighthouse 3

Run Lighthouse 3.0.0-alpha.2 on any website. To simplify, you can run my Gist above.

  "details": {
        "type": "table",
        "headings": [
          {
            "key": "node",
            "itemType": "node",
            "text": "Failing Elements"
          }
        ],
        "items": [
          {
            "node": {
              "type": "node",
              "selector": "#mc-embedded-subscribe",
              "path": "0,HTML,2,BODY,0,DIV,0,FORM,0,DIV,4,DIV,0,INPUT",
              "snippet": "<input type=\"submit\" value=\"Subscribe\" name=\"subscribe\" id=\"mc-embedded-subscribe\" class=\"button\">"
            }
          }
        ]
      }

View Full Gist here

All gone.

This seems like a major regression. I was using that data to consume the json and create markdown files with all the data, not just what is shown on the default html Lighthouse report.

I know it says that extendedInfo has changed to details in the migration doc. But this is more than renaming something. This is removing major functionality.

@brendankenny
Copy link
Member

Agreed, there is stuff in there that should be in details now. Which properties are currently missing that you would find helpful in the new output?

@brendankenny
Copy link
Member

(other properties besides failureSummary, that is)

@joelhsmith
Copy link
Contributor Author

Ideally, I would like all of it. :-) But these are the six I miss most.

extendedInfo.value.id
extendedInfo.value.description
extendedInfo.value.help
extendedInfo.value.helpurl
extendedInfo.value.nodes.failureSummary
extendedInfo.value.nodes.html

I mostly just use the Accessibility section, so I don't know if there are other Categories where meta might have been removed.

Thanks @brendankenny for the fast response! Y'all are awesome.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jun 1, 2018

We're trying to trim down the size of the JSON where possible and a decent amount of that information is duplicated with the audit information already provided (id, title, description, html snippet, selector, etc), or static based on the axe docs.

It seems like just adding impact and failureSummary from the previous metadata might give us close to full coverage?

cc @robdodson do you know if impact changes between nodes of the same rule or will they all be the same impact (matching that of the overall rule)?

@brendankenny
Copy link
Member

brendankenny commented Jun 1, 2018

We're trying to trim down the size of the JSON where possible

yeah, it seems like extendedInfo.value.id is communicated by the audit id (I believe they always match due to how the lookup into the axe results are done) and extendedInfo.value.description, help, and helpurl are found in the audit title and description. The format is quite different than before, and I know that can be a pain, but that's why we were trying to stick it with the 3.0 major breaking changes.

I'm curious about html vs snippet. Are they ever different?

@joelhsmith
Copy link
Contributor Author

joelhsmith commented Jun 4, 2018

Yes, thanks for asking. I can help explain.

snippet is just a weak, less informative version of html. html has the real info people debug errors with that are missing in snippet.

The difference in link-name:

LH2 extendedInfo.value.nodes.snippet is just:

<a href="/foobar">"

LH2 extendedInfo.value.nodes.html is:

 <a href="/foobar"><span class="blue">Learn more about FooBar</span></a>

If we try to help someone debug their accessibility, we need the name in the link to help identify which link has poor contrast.

In the following audits also need html instead of snippet:

  1. color-contrast,
  2. link-name,
  3. most aria- audits,
  4. button-name,
  5. label,
  6. image-alt
  7. and a few more.

So, if we want to cut fat, snippet is the first one we should would get rid of. However, I think there might be a few audits only have snippet. The rule I suggest is, if snippet and html exist always use html.

@joelhsmith
Copy link
Contributor Author

joelhsmith commented Jun 5, 2018

One last request...

LH2 extendedInfo.value.tags would also be useful.

tags labels which WCAG requirement was violated. WCAG 2.1 is coming out soon. It has a few new accessibility rules. Most U.S. Federal, State, and local law only mandates WCAG 2.0. Having the tags meta will allow people to at least programmatically filter out criteria that they might not have the mandate to require by law.

We would be eternally grateful if we can have these four fields back before LH3 is released :-).

extendedInfo.value.impact
extendedInfo.value.tags
extendedInfo.value.nodes.html
extendedInfo.value.nodes.failureSummary

I think that will provide full coverage. Does that sound possible?

@robdodson
Copy link
Contributor

cc @robdodson do you know if impact changes between nodes of the same rule or will they all be the same impact (matching that of the overall rule)?

They'll all be the same impact, which comes from the rule itself.

@ptrin
Copy link

ptrin commented Jun 5, 2018

I'd also like the meta to stay so that this lighthouse to markdown tool can continue development.

@patrickhulce
Copy link
Collaborator

@ptrin does the list @joelhsmith is asking for cover what you need?

extendedInfo.value.impact

extendedInfo.value.tags

Seems reasonable I think we can do this with little impact to LHR size 👍

extendedInfo.value.nodes.failureSummary

Ideally we'd be able to construct this with the rest of the information provided, but it seems like in some cases it's providing totally new information (like the actual color contrast ratio for example). I'm cool with this too 👍

extendedInfo.value.nodes.html

I'm a little more hesitant on this one since we provide a selector, DevTools node path, and the opening tag for identification purposes. The innerHtml as you point out can be quite useful, but it can also be much larger. Would improving snippet to contain the first X characters of textContent be enough here?

@ptrin
Copy link

ptrin commented Jun 5, 2018

does the list @joelhsmith is asking for cover what you need?

Yes, I'm referring to his reporting tool.

@joelhsmith
Copy link
Contributor Author

In defence of extendedInfo.value.nodes.html :-)

I feel like the beautiful thing about Lighthouse is that it proves an easy birds eye view of their issues. I think providing the HTML context of the offending link or contrast error in question supports that birds eye view functionality, making it more accessible (no pun intended) to the layman.

I take your point, there is technically a way to get to the node.html element via the selector. For an advanced developer pasting in the selector into the DevTools “Find by string, selector, or XPath” fields is ok. We know what we are doing. But when we make custom reports for people they won’t know what to do with that selector string or that the functionality even exists.

But being able to print the context for a less competent developer would help them. Here are a few example where extendedInfo.value.nodes.htm is needed to provide context:

a11y-color-contrast audit

from https://www.w3.org/WAI/demos/bad/before/home.html

extendedInfo.value.nodes.snippet:
<b>

extendedInfo.value.nodes.html:
<b>More City Parks</b>

from https://theonion.com

extendedInfo.value.nodes.snippet:
<a href=\"//politics.theonion.com\" onclick=\"window.ga('send', 'event', 'Sub navigation', 'Vertical/Tag Click', 'onionpolitics');\">

extendedInfo.value.nodes.html:
<a href=\"//politics.theonion.com\" onclick=\"window.ga('send', 'event', 'Sub navigation', 'Vertical/Tag Click', 'onionpolitics');\">Politics</a>

Note the name of the link "politics" comes through html

In support of providing a birds eye view, I kinda think .html instead of .snippet solves the core issue of the need for context.

Anecdotally, for color-contrast and link-name, I tested about 50 website’s homepages this morning, and I did not run across a scenario where using html was much longer than Snippet. The possibility of .html getting long and adding to the files size for the color-contrast and link-name audits might be an edge case.

To play devil’s advocate against myself: Perhaps, providing .innerText could suffice for a11y-color-contrast or link-name. But the extendedInfo.value.nodes.html would provide both, without the need for another field. And those two fields are the least likely to add bloat.

ARIA:

I concede that the aria-* audits can make a longer string, such as aria-required-children, but in defense of those kb, the need to see the context of children for a rule like aria-allowed-attr is everything.

The aria-allowed-attr audit result won’t make sense without seeing it children. Because Each ARIA role supports a specific subset of aria-* attributes. Mismatching these invalidates the aria-* attributes.

aria-allowed-attr audit

from https://Petsmart.com

extendedInfo.value.nodes.snippet:
<li aria-hidden=\"true\" role=\"presentation\" aria-selected=\"false\" aria-controls=\"navigation02\" id=\"slick-slide02\" class=\"\">

extendedInfo.value.nodes.html:
<li aria-hidden=\"true\" role=\"presentation\" aria-selected=\"false\" aria-controls=\"navigation02\" id=\"slick-slide02\" class=\"\"><button type=\"button\" data-role=\"none\" role=\"button\" aria-required=\"false\" tabindex=\"0\">3</button></li>

Note the true source of the error is actually caused by the inner HTML not the snippet, depending on your point of view.

extendedInfo.value.nodes.html and extendedInfo.value.nodes.snippet do make duplicate results sometimes. And I understand we don’t want to cause bloat from duplicative code. But when applicable, using .html instead of .snippet would solve both the context problem as well bloat. It would also eliminate the need to add a second field, because pulling out the name from .html is a simple stripTags procedure for people who are consuming the JSON to output in friendly ways. Snippet will never have information that is not provided by .html already.

Other idea, can we create a flag to generate verbose version of the report with all that .extendedInfo.value.nodes.html?

@joelhsmith
Copy link
Contributor Author

Oh, @patrickhulce I would be remiss not to thank you for impact, tags, and failureSummary additions.

@patrickhulce
Copy link
Collaborator

Alright @joelhsmith good points, you've sold me on returning .html :)

Would it be a problem for you if the .html content just got moved to be the .snippet key? We have special handling for the .snippet property to be displayed as code right now and it'd be nice to be consistent.

@joelhsmith
Copy link
Contributor Author

Perfect! As i mentioned before there are a couple audits that only have .snippet but do not have .html. So ya, if .html exists in the audit, replace .snippet with .html's content. That would be great.

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

Successfully merging a pull request may close this issue.

5 participants