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.

Sign up for GitHub

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

Jump to bottom

Possible regression in 'region' landmark assessment rule #2047

Closed
jayaddison opened this issue Feb 17, 2020 · 13 comments
Closed

Possible regression in 'region' landmark assessment rule #2047

jayaddison opened this issue Feb 17, 2020 · 13 comments

Comments

@jayaddison
Copy link
Contributor

Expectation:

As part of the test suite for Google Chrome's lighthouse project, an accessibility self-test is performed using axe-core on the HTML report which the application generates.

From manual inspection, the HTML assessed by the test does seem to adhere to the practices documented in the ARIA landmark authoring guidelines.

The test passes as expected when using axe-core 3.4.1.

Actual:

After upgrading to axe-core 3.5.1, the accessibility self-test fails (refs: comment, build logs).

Motivation:

This appears to be a false-positive and that's the main motivation for raising the issue. In addition it temporarily blocks lighthouse from upgrading to axe-core 3.5.1 (or would require a reduction in lighthouse's test coverage in order to upgrade).


axe-core version: 3.5.1

Browser and Assistive Technology versions

For Tooling issues:
- Node version: v10.15.2 (developer environment), v10.18.1 (AppVeyor environment), v10.19.0 (Travis-CI environment)
- Platform:  Linux (developer environment), Windows (AppVeyor environment), Linux (Travis-CI environment)
@jayaddison
Copy link
Contributor Author

Currently trying to reproduce this problem in a development environment, by editing the region-pass.html test fixture and using the grunt dev local webserver; hoping to provide some additional details/updates soon.

@jayaddison
Copy link
Contributor Author

There's a possibility that this is an incorrect issue report on my behalf.

After replacing the contents of the region-pass.html test fixture with the lighthouse test report output (and restoring the mocha test harness elements), the page is reported as a pass.

This makes me wonder if the true issue lies within the lighthouse test case and the way it runs axe-core against the page. In particular the fact that it generates the report into a body container (ref) could be relevant.

This test rendering approach hasn't changed recently in lighthouse, but does seem worth looking at further since the CSS selector change also relates to the body element.

@jayaddison
Copy link
Contributor Author

For reference - the HTML output used to replace the region-pass.html content during development testing is visible here.

@jayaddison
Copy link
Contributor Author

This issue is related to #1980

@straker
Copy link
Contributor

straker commented Feb 17, 2020

Interesting. We didn't really change how the check works but mostly how it reported the error. As you said, running on the HTML output by itself doesn't report a region rule violation, so I'm very curious what's going on.

I tried wrapping the output of the HTML report inside another <body> container, but that didn't seem to produce an error for the region rule either.

@straker
Copy link
Contributor

straker commented Feb 17, 2020 •
edited

What's strange to me is the output of the errors in CLI. The code should find the outermost element that isn't in a landmark, but the HTML sources for those errors is not reporting outermost elements. Instead of .lh-container you get individual children reported.

Edit: Ah, there's a <footer> element as a descendant and that's why.

@jayaddison
Copy link
Contributor Author

Thanks for taking a look. A little extra bit of info - when I revert the selector back from body * to html that seems to resolve the lighthouse test issue for me, and works fine in the axe-core codebase too as far as I can tell, but I didn't want to open a PR until figuring out exactly why that's the case.

Just catching up on the footer descendent item you mention.. not completely clear on that yet but taking a look.

@straker
Copy link
Contributor

straker commented Feb 17, 2020 •
edited

The <footer> element just prevents .lh-container from being reported as the source element for the error. Nothing too much to look into.

As for the selector, the only reason it was changed was to avoid looking at and head elements since they are never checked for regions. I don't have anything against changing it back to html, but I'd like to know why the lighthouse test is failing first.

Would it be possible to capture the output of the test HTML output before the assert is run on it? I'd like to see what exactly axe is running against (I know it should be what we've already looked at, but that should be passing).

@jayaddison
Copy link
Contributor Author

jayaddison commented Feb 17, 2020 •
edited

Here's my initial attempt at including the HTML output - I used console.log(renderer._dom._document.documentElement.outerHTML); to produce this, and piped it to file, both of which are non-ideal, but hopefully safe.

One caveat is that I think there's some dynamic content introduced into the page by lighthouse's report viewer component (which I don't yet understand). For example, I've seen a noscript tag appear after the body element (and before main) which isn't present in the output I've attached.

Edit: zipfile removed in favour of gist

@straker
Copy link
Contributor

straker commented Feb 17, 2020

Could you past the output into a codeblock tag or upload to codepen/pastebin? Sorry, I try not to open zip files

@jayaddison
Copy link
Contributor Author

Yep, no problem - that's a sensible policy. I've uploaded it to https://gist.github.com/jayaddison/1a465b0ad0b3300439151fc2e1e5ab13 .

Your note made me double-check myself and so there is technically one difference from the original zip contents; I've pasted in the Google Chrome / Apache 2.0 licensing comment and doctype tag preceding the opening HTML tag.

@straker
Copy link
Contributor

straker commented Feb 17, 2020 •
edited

Alright, I think I see where the problem may be. The test is appending the output of renderReport to the <body> element. Render report in turn takes the JSON output of the lighthouse results and creates the document from it. However, the document only contains the two divs .lh-topbar and .lh-container (with child contents) and not the <main> wrapper of the report HTML. This causes the test to only contain a structure like below which contains no landmarks.

<body>
  <div class="lh-topbar">...</div>
  <div class="lh-container"> 
    <div class="lh-sticky-header">...</div>
    <div></div>
    <div class="lh-report">...</div>
  </div>
</body>

As to why this is a new failure I'm not sure. My only guess is since the html selector causes the test to pass, there's something wrapping the contents of renderer._dom._document.body that is a landmark.

However, I believe this is a true failure based on the output alone of renderReport.

@jayaddison
Copy link
Contributor Author

Thanks again @straker - yep, after making some small adjustments (particularly this one to ensure that content rendered for the lighthouse self-test is contained by a main element), the region rule seems to assess the output correctly.

In other words, this does seem like a true positive and axe-core seems correct. My only thought regarding why this appeared is that perhaps the html selector in the lighthouse test was not working as expected in the past?

Either way, this issue seems resolved so I'll go ahead and close it.

@jayaddison jayaddison mentioned this issue Feb 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@straker @jayaddison