-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
WIP: core(axe): new audits empty-table-header, frame-focusable-content #12536
Conversation
@@ -2016,33 +2114,32 @@ | |||
], | |||
"incomplete": [ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno why color-contrast
went bye-bye
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno why
color-contrast
went bye-bye
hmm, didn't this change once before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i guess it did and we decided to manually keep it around?
failureTitle: '`role=text` elements should not have focusable descendants', | ||
// TODO: need web.dev article. | ||
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | ||
description: 'Browsers may ignore `role=text` if it contains focusable descendants.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: 'Browsers may ignore `role=text` if it contains focusable descendants.', | |
description: 'Browsers may ignore elements with `role=text` if they contain focusable descendants.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure on this one. browsers ignore the element _as far as role=text
is concerned, which is why I put the focus on that. The element still exists, is visible, etc. so it isn't entirely ignored :)
@@ -225,6 +225,21 @@ <h2>Do better web tester page</h2> | |||
<button class="small-button">Do something</button> | |||
<button class="small-button">Do something else</button> | |||
|
|||
<!-- FAIL - frame-focusable-content --> | |||
<iframe tabindex="-1" src="focusable-iframe.html"></iframe> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use srcdoc instead to avoid the 1-line extra file? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first try, didn't work! Not supported I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first try, didn't work! Not supported I suppose.
interesting, are srcdoc iframes a unique origin and so not available in the parent via js (so axe wouldn't be able to see inside)?
<!-- FAIL - aria-text --> | ||
<div role="text"> | ||
Still an <a href="#">interactive link</a> because of the author error. | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to a11y_tester
with the other accessibility smoke tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are here to populate the sample LHR. Do we care about coverage there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are here to populate the sample LHR. Do we care about coverage there?
ah, got it. Ideally yes, so maybe just duplicate in there? It's very helpful to have yarn smoke a11y
cover (almost?) everything in the accessibility category
// TODO .lh-category-wrapper:nth-child(2) > .lh-category > .lh-clump--manual.lh-clump.lh-audit-group > summary | ||
'nested-interactive': {enabled: false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the selector of the failure? This is going to be lost down here. How hard is the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, it was Friday evening, this PR got big, and I just wanted to expedite feedback (since I knew there'd be quite a bit).
don't plan on pushing this PR w/o looking into this more. looks fun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://dequeuniversity.com/rules/axe/4.2/nested-interactive
so it seems that this rule fails b/c summary
containing a link :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this rule is not ready for LH–should we hide it, keep it to zero weight, or delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this rule is not ready for LH–should we hide it, keep it to zero weight, or delete?
I vote we delete it and aria-text
but leave a TODO for nested-interactive
so we update it next time if ready
lighthouse-core/test/report/html/renderer/report-renderer-test.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Hulce <[email protected]>
<!-- FAIL - aria-text --> | ||
<div role="text"> | ||
Still an <a href="#">interactive link</a> because of the author error. | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are here to populate the sample LHR. Do we care about coverage there?
ah, got it. Ideally yes, so maybe just duplicate in there? It's very helpful to have yarn smoke a11y
cover (almost?) everything in the accessibility category
@@ -225,6 +225,21 @@ <h2>Do better web tester page</h2> | |||
<button class="small-button">Do something</button> | |||
<button class="small-button">Do something else</button> | |||
|
|||
<!-- FAIL - frame-focusable-content --> | |||
<iframe tabindex="-1" src="focusable-iframe.html"></iframe> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first try, didn't work! Not supported I suppose.
interesting, are srcdoc iframes a unique origin and so not available in the parent via js (so axe wouldn't be able to see inside)?
'use strict'; | ||
|
||
/** | ||
* @fileoverview Ensures `role=text` is only used on elements with no focusable descendants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rob seemed like he was lukewarm about some of the possible aria-text
gotchas and it's a best-practice
, not wcag2a
or wcag2aa
. Should we drop for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rob said:
aria-text is kind of news to me and looks like it has a potential gotcha (like if you have a link inside of the heading) so...hm...idk
i said:
apparently aria-label is the role to use if element has focusable elements, not aria-text. altho TIL role=text isnt standard yet? https://incl.ca/roletext-presently-kinda-not-thing-sorta/#:~:text=here-,role%3D%22text%22%20isn,While,-that
or are you referring to something else?
role=text
not being standard seems like a convincing reason to drop/hide/0-weight it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's what I was trying to summarize without using many braincells, apparently :)
lighthouse-core/audits/accessibility/frame-focusable-content.js
Outdated
Show resolved
Hide resolved
|
||
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); | ||
|
||
class NestedInteractive extends AxeAudit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if nested-interactive
is problematic, I say we just put 'nested-interactive': {enabled: false},
in accessibility.js
and put a TODO linked to your dequelabs/axe-core#2958. Looking at the history of nested-interactive
, the discussion seems to be all around <button>
and e.g. links inside elements that are role="checkbox"
, but then it became all presentational elements. We thought we were doing well in the report with our <details>
, so I think in this case we at least need to figure out how we fix ourselves before we demand other people fix their own sites :)
lighthouse-core/audits/accessibility/frame-focusable-content.js
Outdated
Show resolved
Hide resolved
lighthouse-core/audits/accessibility/frame-focusable-content.js
Outdated
Show resolved
Hide resolved
@@ -2016,33 +2114,32 @@ | |||
], | |||
"incomplete": [ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno why
color-contrast
went bye-bye
hmm, didn't this change once before?
Co-authored-by: Brendan Kenny <[email protected]>
@connorjclark @patrickhulce I think the main last thing here is deciding if we want to keep
|
Given time before 8.0, I'd say drop for now. We can always bring it back in a point release later.
Does this mean even when the table header has text, it gets marked as incomplete? Or are you saying that in every case where it should be failing it shows up as incomplete instead? (Or somewhere in between) If it's just that the failing case is replaced with incomplete, I think that's fine. It's a bit weird to not be afailure but maybe it's not as big a deal 🤷 If it's any of the other two options. I'd also say drop. |
#10072 is a bit weird. The idea was that for axe rules that can't fail but get marked So in all it means
all that's fine, I guess? It's not part of |
lol so they flipped passing and notApplicable on this rule. I'd vote file a bug with axe and implement later if it's fixed. |
When we get back to this we should add an audit for select-name (fixes #12639). Seems like an oversight that this got missed. Maybe we should add an assertion that every axe rule that we're running has an audit? |
👋 hey @connorjclark @patrickhulce - I noticed this while looking back through some GitHub history. After refreshing my memory and reading this thread, case three above ( I remember Footnotes:
|
Really sorry if my comment came across as criticizing. The thing I was trying to say (but reading back, it's a bit too implicitly stated) is that the change was necessary to handle incomplete results, but that the notApplicable part left a subset of results from a single axe rule in a weird state because axe has one way of marking results as possible but not (currently) verifiable problems and lighthouse has its own way of doing the same, but they're not quite compatible. So sometimes you end up in this technically correct but hard to diagnose state for that one audit (two when we land I didn't raise any alarms specifically because this only affects Your thoughts on resolving the third state sound good to me, and maybe there's more we can do to help future updaters of axe in Lighthouse with knowing when to mark an audit |
And there have been several people in the accessibility gatherer and |
Thanks @brendankenny - nope, no problem in your messaging at all - I was worried by the idea of #10072 having had a wide negative impact, but am glad to hear and see that the number of And yep, I recall that kind of venn-diagram-matching problem re: the slight differences between the way the two libraries model the world. A lot of my unease, I think, was that I'd lost whatever context I had since then, and that perhaps I hadn't understood the problem properly (still possible!). I'll probably gladly opt-out of any further changes for now, a bit less time nowadays. Thank you though :) |
Many bug fixes. A few new rules.
Need web.dev pages ... OR (hear me out...) we just link to axe docs...