Skip to content

Commit

Permalink
fix: Reduce chromium noise (#951)
Browse files Browse the repository at this point in the history
#### Details

As called out in #836, the `ChromiumContentShouldUseWebScanner` has
caused a lot of confusion. This PR aims to reduce that confusion. It
does the following:

- The `ChromiumContentShouldUseWebScanner` rule will now trigger _only_
on Chromium-generated `Document` elements
- Other rules are disabled on elements that are Chromium-generated
`Document` elements or any of their descendants
- Future rules default to being disabled on elements that are
Chromium-generated `Documents` or any of their descendants. The default
behavior can easily be overridden
- Where needed for unit tests with strict `mock`s, provide test
constructors that allow unit tests to isolate the class under test from
the Chromium suppression

##### Motivation

Address #836, make the `ChromiumContentShouldUseWebScanner` rule less
intrusive

##### Attachments

[BeforeAndAfter.zip](https://github.com/microsoft/axe-windows/files/12101240/BeforeAndAfter.zip)
contains 2 `a11ytest` files
- `Before.a11ytest` contains scan results from Edge before this change
- `After.a11ytest` contains scan results from Edge after this change

Here are how the numbers line up

Message | Count before | Count after | Count of Chromium elements
--- | --- | --- | ---
An element must not have the same Name and LocalizedControlType as its
parent | 7 | 0 | 7
An element of the given ControlType must not support the Invoke pattern
| 1 | 1 | 0
An element of the given ControlType must support the Text pattern | 1 |
0 | 1
An on-screen element must not have a null BoundingRectangle property | 5
| 5 | 0
Chromium components should be scanned with a web-based scanner | 63 | 2
| 63
The Name must not include the same text as the LocalizedControlType | 2
| 2 | 0
The Name property must not include the element's control type | 2 | 2 |
0

Things to note on this table:
- There are 2 Chromium documents on the page--one for the HTML content
and one for the search button. This is an implementation detail of the
search button
- Only the 2 Chromium documents are counted in "Count after". The 61
omitted elements are children of one of the two Chromium documents
- For all other columns. "Count before" - "Count after" = "Count of
Chromium elements". This is as expected

If you open `After.a11ytest` and inspect the children of the Chromium
documents, the UIA properties are reported but no tests were run. That's
exactly what this change was intended to do.

##### Screenshots

Here are visual representations of the test results with all test
results displaying. Notice the difference in the HTML document space:

File | Snapshot (click to enlarge) | Test Results (click to enlarge)
--- | --- | ---
Before |
![image](https://github.com/microsoft/axe-windows/assets/45672944/3e70661f-6981-451d-b1fe-3314decb276c)
|
![image](https://github.com/microsoft/axe-windows/assets/45672944/eb03f67e-2e2f-467d-8dd5-666b44aa2a1b)
After |
![image](https://github.com/microsoft/axe-windows/assets/45672944/aa08a6ca-4d81-4ee2-b300-fe7de70a4ef6)
|
![image](https://github.com/microsoft/axe-windows/assets/45672944/9519e8b9-9b90-48b6-ab9e-46c19b18c4c4)

##### Context

<!-- Are there any parts that you've intentionally left out-of-scope for
a later PR to handle? -->

<!-- Were there any alternative approaches you considered? What
tradeoffs did you consider? -->

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: #836
  • Loading branch information
DaveTryon committed Jul 26, 2023
1 parent d68e4d7 commit b078401
Show file tree
Hide file tree
Showing 25 changed files with 150 additions and 79 deletions.
8 changes: 3 additions & 5 deletions src/Rules/Library/ChromiumComponentsShouldUseWebScanner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@
using Axe.Windows.Core.Bases;
using Axe.Windows.Core.Enums;
using Axe.Windows.Rules.Resources;
using static Axe.Windows.Rules.PropertyConditions.ControlType;
using static Axe.Windows.Rules.PropertyConditions.Framework;
using static Axe.Windows.Rules.PropertyConditions.Relationships;
using static Axe.Windows.Rules.PropertyConditions.ElementGroups;

namespace Axe.Windows.Rules.Library
{
[RuleInfo(ID = RuleId.ChromiumComponentsShouldUseWebScanner)]
class ChromiumComponentsShouldUseWebScanner : Rule
{
public ChromiumComponentsShouldUseWebScanner()
public ChromiumComponentsShouldUseWebScanner() : base(excludedCondition: null)
{
Info.Description = Descriptions.ChromiumComponentsShouldUseWebScanner;
Info.HowToFix = HowToFix.ChromiumComponentsShouldUseWebScanner;
Expand All @@ -27,7 +25,7 @@ public override bool PassesTest(IA11yElement e)

protected override Condition CreateCondition()
{
return Chrome & (Document | AnyAncestor(Document));
return IsChromiumDocument;
}
} // class
} // namespace
9 changes: 7 additions & 2 deletions src/Rules/Library/ClickablePointOffScreen.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All rights reserved.
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using Axe.Windows.Core.Bases;
Expand All @@ -14,7 +14,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.ClickablePointOffScreen)]
class ClickablePointOffScreen : Rule
{
public ClickablePointOffScreen()
// Testable constructor
internal ClickablePointOffScreen(Condition excludedCondition) : base(excludedCondition)
{
Info.Description = Descriptions.ClickablePointOffScreen;
Info.HowToFix = HowToFix.ClickablePointOffScreen;
Expand All @@ -23,6 +24,10 @@ public ClickablePointOffScreen()
Info.ErrorCode = EvaluationCode.Warning;
}

public ClickablePointOffScreen() : this(DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
9 changes: 7 additions & 2 deletions src/Rules/Library/ClickablePointOnScreen.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All rights reserved.
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using Axe.Windows.Core.Bases;
Expand All @@ -15,7 +15,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.ClickablePointOnScreen)]
class ClickablePointOnScreen : Rule
{
public ClickablePointOnScreen()
// Testable constructor
public ClickablePointOnScreen(Condition excludedCondition) : base(excludedCondition)
{
Info.Description = Descriptions.ClickablePointOnScreen;
Info.HowToFix = HowToFix.ClickablePointOnScreen;
Expand All @@ -24,6 +25,10 @@ public ClickablePointOnScreen()
Info.ErrorCode = EvaluationCode.Error;
}

public ClickablePointOnScreen() : this(DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
7 changes: 6 additions & 1 deletion src/Rules/Library/ClickablePointOnScreenWPF.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.ClickablePointOnScreenWPF)]
class ClickablePointOnScreenWPF : Rule
{
public ClickablePointOnScreenWPF()
// Testable constructor
public ClickablePointOnScreenWPF(Condition excludedCondition) : base(excludedCondition)
{
Info.Description = Descriptions.ClickablePointOnScreen;
Info.HowToFix = HowToFix.ClickablePointOnScreen;
Expand All @@ -25,6 +26,10 @@ public ClickablePointOnScreenWPF()
Info.FrameworkIssueLink = "https://go.microsoft.com/fwlink/?linkid=2214600";
}

public ClickablePointOnScreenWPF() : this(DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
7 changes: 6 additions & 1 deletion src/Rules/Library/EdgeBrowserHasBeenDeprecated.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.EdgeBrowserHasBeenDeprecated)]
class EdgeBrowserHasBeenDeprecated : Rule
{
public EdgeBrowserHasBeenDeprecated()
// Testable constructor
internal EdgeBrowserHasBeenDeprecated(Condition excludedCondition) : base (excludedCondition)
{
Info.Description = Descriptions.EdgeBrowserHasBeenDeprecated;
Info.HowToFix = HowToFix.EdgeBrowserHasBeenDeprecated;
Expand All @@ -20,6 +21,10 @@ public EdgeBrowserHasBeenDeprecated()
Info.FrameworkIssueLink = "https://go.microsoft.com/fwlink/?linkid=2214421";
}

public EdgeBrowserHasBeenDeprecated() : this (excludedCondition: DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
return false;
Expand Down
7 changes: 6 additions & 1 deletion src/Rules/Library/FrameworkDoesNotSupportUIAutomation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ class FrameworkDoesNotSupportUIAutomation : Rule
@"^\s*SunAwt.*$",
};

public FrameworkDoesNotSupportUIAutomation()
// Testable constructor
internal FrameworkDoesNotSupportUIAutomation(Condition excludedCondition) : base (excludedCondition)
{
Info.Description = Descriptions.FrameworkDoesNotSupportUIAutomation;
Info.HowToFix = HowToFix.FrameworkDoesNotSupportUIAutomation;
Expand All @@ -27,6 +28,10 @@ public FrameworkDoesNotSupportUIAutomation()
Info.FrameworkIssueLink = "https://go.microsoft.com/fwlink/?linkid=2214160";
}

public FrameworkDoesNotSupportUIAutomation() : this(excludedCondition: DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
string className = e.ClassName;
Expand Down
9 changes: 7 additions & 2 deletions src/Rules/Library/IsControlElementTrueRequired.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All rights reserved.
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using Axe.Windows.Core.Bases;
Expand All @@ -14,7 +14,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.IsControlElementTrueRequired)]
class IsControlElementTrueRequired : Rule
{
public IsControlElementTrueRequired()
// Testable constructor
public IsControlElementTrueRequired(Condition excludedCondition) : base(excludedCondition)
{
Info.Description = Descriptions.IsControlElementTrueRequired;
Info.HowToFix = HowToFix.IsControlElementTrueRequired;
Expand All @@ -23,6 +24,10 @@ public IsControlElementTrueRequired()
Info.ErrorCode = EvaluationCode.Error;
}

public IsControlElementTrueRequired() : this(DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
7 changes: 6 additions & 1 deletion src/Rules/Library/IsControlElementTrueRequiredButtonWPF.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.IsControlElementTrueRequiredButtonWPF)]
class IsControlElementTrueRequiredButtonWPF : Rule
{
public IsControlElementTrueRequiredButtonWPF()
// Testable constructor
public IsControlElementTrueRequiredButtonWPF(Condition excludedCondition): base(excludedCondition)
{
Info.Description = Descriptions.IsControlElementTrueRequired;
Info.HowToFix = HowToFix.IsControlElementTrueRequired;
Expand All @@ -24,6 +25,10 @@ public IsControlElementTrueRequiredButtonWPF()
Info.FrameworkIssueLink = "https://go.microsoft.com/fwlink/?linkid=2214420";
}

public IsControlElementTrueRequiredButtonWPF() : this(DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.IsControlElementTrueRequiredTextInEditXAML)]
class IsControlElementTrueRequiredTextInEditXAML : Rule
{
public IsControlElementTrueRequiredTextInEditXAML()
// Testable constructor
public IsControlElementTrueRequiredTextInEditXAML(Condition excludedCondition) : base(excludedCondition)
{
Info.Description = Descriptions.IsControlElementTrueRequired;
Info.HowToFix = HowToFix.IsControlElementTrueRequired;
Expand All @@ -24,6 +25,10 @@ public IsControlElementTrueRequiredTextInEditXAML()
Info.FrameworkIssueLink = "https://go.microsoft.com/fwlink/?linkid=2214418";
}

public IsControlElementTrueRequiredTextInEditXAML() : this(DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
9 changes: 7 additions & 2 deletions src/Rules/Library/ProgressBarRangeValue.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All rights reserved.
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using Axe.Windows.Core.Bases;
Expand All @@ -14,7 +14,8 @@ namespace Axe.Windows.Rules.Library
[RuleInfo(ID = RuleId.ProgressBarRangeValue)]
class ProgressBarRangeValue : Rule
{
public ProgressBarRangeValue()
// Testable constructor
internal ProgressBarRangeValue(Condition excludedCondition) : base(excludedCondition)
{
Info.Description = Descriptions.ProgressBarRangeValue;
Info.HowToFix = HowToFix.ProgressBarRangeValue;
Expand All @@ -23,6 +24,10 @@ public ProgressBarRangeValue()
Info.ErrorCode = EvaluationCode.Error;
}

public ProgressBarRangeValue() : this(excludedCondition: DefaultExcludedCondition)
{
}

public override bool PassesTest(IA11yElement e)
{
if (e == null) throw new ArgumentNullException(nameof(e));
Expand Down
6 changes: 4 additions & 2 deletions src/Rules/PropertyConditions/ElementGroups.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Axe.Windows.Core.Bases;
using Axe.Windows.Core.Misc;
using Axe.Windows.Core.Types;
using Axe.Windows.Rules.Resources;
using static Axe.Windows.Rules.PropertyConditions.BoolProperties;
using static Axe.Windows.Rules.PropertyConditions.ControlType;
using static Axe.Windows.Rules.PropertyConditions.Framework;
Expand All @@ -13,7 +14,7 @@
namespace Axe.Windows.Rules.PropertyConditions
{
/// <summary>
/// A collection of conditions representing elements grouped for miscellaneous rules.
/// A collection of conditions representing elements grouped for miscellaneous rules.
/// These are conditions which tend to be reused by multiple rules.
/// </summary>
static class ElementGroups
Expand Down Expand Up @@ -45,7 +46,8 @@ static class ElementGroups
public static Condition WPFButton = WPF & Button;
public static Condition XAMLTextInEdit = XAML & Text & Parent(Edit);
public static Condition WinFormsEdit = Edit & WinForms;

public static Condition IsChromiumDocument = Chrome & Document;
public static Condition IsChromiumContent = (IsChromiumDocument | AnyAncestor(IsChromiumDocument))[ConditionDescriptions.IsChromiumContent];
public static Condition AllowSameNameAndControlType = CreateAllowSameNameAndControlTypeCondition();

private static Condition CreateMinMaxCloseButtonCondition()
Expand Down
9 changes: 9 additions & 0 deletions src/Rules/Resources/ConditionDescriptions.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/Rules/Resources/ConditionDescriptions.resx
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,8 @@
<value>LocalizedLandmarkType</value>
<comment>Do not translate: the name of a UI Automation property.</comment>
</data>
<data name="IsChromiumContent" xml:space="preserve">
<value>IsChromiumContent</value>
<comment>{Locked}: a token used by the Axe.Windows domain-specific language for accessibility descriptions.</comment>
</data>
</root>
15 changes: 13 additions & 2 deletions src/Rules/Rule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Axe.Windows.Core.Exceptions;
using Axe.Windows.Core.Misc;
using Axe.Windows.Rules.Resources;
using static Axe.Windows.Rules.PropertyConditions.ElementGroups;

namespace Axe.Windows.Rules
{
Expand All @@ -19,17 +20,27 @@ abstract class Rule : IRule
{
public RuleInfo Info { get; private set; }
public Condition Condition { get; }
protected static Condition DefaultExcludedCondition => IsChromiumContent;

protected Rule()
protected Rule(Condition excludedCondition)
{
// keep these two calls in the following order or the RuleInfo.Condition string won't get populated
// keep these calls in the following order or the RuleInfo.Condition string won't get populated
#pragma warning disable CA2214
Condition = CreateCondition();
#pragma warning restore CA2214

if (excludedCondition != null)
{
Condition = Condition - excludedCondition;
}

InitRuleInfo();
}

protected Rule() : this (excludedCondition: DefaultExcludedCondition)
{
}

private void InitRuleInfo()
{
var info = GetRuleInfoFromAttributes();
Expand Down

0 comments on commit b078401

Please sign in to comment.