feat(chrome-scans): Add mechanism for excluding elements from scans #873
Conversation
Codecov Report
@@ Coverage Diff @@
## main #873 +/- ##
==========================================
+ Coverage 73.70% 74.91% +1.21%
==========================================
Files 398 422 +24
Lines 12046 13108 +1062
==========================================
+ Hits 8878 9820 +942
- Misses 3168 3288 +120
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/RuleSelection/RuleRunner.cs
Outdated
| throw; | ||
| } | ||
| //#pragma warning disable CA1031 // Do not catch general exception types | ||
| // catch (Exception ex) |
There was a problem hiding this comment.
Debugging code (did you intend to check this in)?
There was a problem hiding this comment.
I was patterning off the existing run logic (see https://github.com/microsoft/axe-windows/blob/main/src/RuleSelection/RuleRunner.cs#L31), but happy to remove this if we don't think this is necessary.
src/DesktopTests/UIAutomation/TreeWalkers/TreeWalkerForTestTests.cs
Outdated
Show resolved
Hide resolved
src/DesktopTests/UIAutomation/TreeWalkers/TreeWalkerForTestTests.cs
Outdated
Show resolved
Hide resolved
| //#pragma warning restore CA1031 // Do not catch general exception types | ||
| } | ||
|
|
||
| private static bool ExcludeFromRunUnsafe(A11yElement e, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Can you comment on why this method is called "Unsafe" ? I'm not quite following the logic of the naming
There was a problem hiding this comment.
I was patterning off the existing run set up: https://github.com/microsoft/axe-windows/blob/main/src/RuleSelection/RuleRunner.cs#L39
| @@ -27,5 +29,11 @@ protected override Condition CreateCondition() | |||
| { | |||
| return Chrome; | |||
| } | |||
|
|
|||
| public override bool IncludeInResults(IA11yElement element) | |||
There was a problem hiding this comment.
Taking a big step back, could we include this check directly in ChronmiumComponentsShouldUseWebScanner.CreateCondition ? I think that would avoid the whole concept of included and excluded rules, leading to simpler code.
Something like this, perhaps:
protected override Condition CreateCondition()
{
return Chrome
& (NoParentExists | ~Parent(Chrome));
}
There was a problem hiding this comment.
We could definitely do that, but I originally went with this approach because if we just updated the condition then we will continue to scan the chrome elements for all the other rules, which results in a lot of failures that aren't really reliable. One alternative is we could do the normal scan with the updated condition logic you're suggesting and then find some way to exclude all the failures from all the elements that fail the chrome rule after the fact, but that would mean running tests on many elements just to exclude them later, which felt wasteful to me.
There was a problem hiding this comment.
I'm fairly sure that we can do this more easily, just by updating the condition for the rule that we're trying to exclude. Please see this comment for more info
|
Per windows triage: Closing this for now as we're still undecided as to how best to approach this change. We plan to treat this as a full feature going forward. |
Details
This PR is 2/2 of addressing #836. #871 added the
ChromiumComponentsShouldUseWebScannerrule and this PR adds logic to exclude elements that fail this rule from further scanning. It also includes logic to ensure that only the top level chrome element reports aChromiumComponentsShouldUseWebScannerrule violation.Motivation
Addresses issue #836
Pull request checklist