Skip to content

Commit

Permalink
fix: Reduce noise in ChromiumComponentsShouldUseWebScanner rule (#931)
Browse files Browse the repository at this point in the history 
#### Details

microsoft/accessibility-insights-windows#1625
points out that the `ChromiumComponentsShouldUseWebScanner` rule is
flagging items that are outside the context of a web page, which goes
against the rule's primary purpose. This change limits the
`ChromiumComponentsShouldUseWebScanner` rule to elements within a HTML
document (including the document itself).

While working on this, we discovered a bug in WebView2, which is tracked
at MicrosoftEdge/WebView2Feedback#3530.

##### Motivation

Address
microsoft/accessibility-insights-windows#1625

##### Before & after

###### Text results using Edge

As expected, only the `ChromiumComponentsShouldUseWebScanner` rule is
impacted
Rule | Count Before | Count After
--- | --- | ---
An element of the given ControlType must not support the invoke pattern
| 1 | 1
An on-screen element must not have a null BoundingRectangle property | 7
| 7
Chromium components should be scanned with a web-based scanner | 211 |
135
The Name must not include the same text as the LocalizedControlType | 5
| 5
The Name property must not contain only whitespace | 3 | 3
The Name property must not include the element's control type | 1 | 1
The Name property of a focusable element must not be null | 10 | 10

###### Screenshots using Edge

These screenshots are limited to just the
`ChromiumComponentsShouldUseWebScanner` rule:

State | Screenshot
--- | ---
Before change |
![image](https://github.com/microsoft/axe-windows/assets/45672944/13040b65-3ac7-475e-b6ef-6ca2d2a98907)
After change |
![image](https://github.com/microsoft/axe-windows/assets/45672944/07b7faac-d0fe-46c7-a6b4-7991f9f88838)


##### Context

This includes an opportunistic optimization to some of the code in
`Relationships`, because the unit test code looked odd with arbitrary
multipliers to compensate for hoe `Relationships` is coded. This could
be a separate PR if that's preferred.

<!-- 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:
microsoft/accessibility-insights-windows#1625
  • Loading branch information
@DaveTryon
DaveTryon authored Jun 2, 2023
1 parent 4f621b9 commit 2db94f5
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 14 deletions.
4 changes: 2 additions & 2 deletions src/AutomationTests/AutomationIntegrationTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ public class AutomationIntegrationTests
const int WindowsFormsControlSamplerKnownErrorCount = 6;
const int WpfControlSamplerKnownErrorCount = 7;
const int WindowsFormsMultiWindowSamplerAppAllErrorCount = 12;
// Note: This will be reduced to 1 when we add logic to ignore all but the top level chrome element
const int WebViewSampleKnownErrorCount = 21;
// Note: This should change to 159 after https://github.com/MicrosoftEdge/WebView2Feedback/issues/3530 is fixed and integrated
const int WebViewSampleKnownErrorCount = 6;

readonly string _wildlifeManagerAppPath = Path.GetFullPath("../../../../../tools/WildlifeManager/WildlifeManager.exe");
readonly string _win32ControlSamplerAppPath = Path.GetFullPath("../../../../../tools/Win32ControlSampler/Win32ControlSampler.exe");
Expand Down
4 changes: 3 additions & 1 deletion src/Rules/Library/ChromiumComponentsShouldUseWebScanner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
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;

namespace Axe.Windows.Rules.Library
{
Expand All @@ -25,7 +27,7 @@ public override bool PassesTest(IA11yElement e)

protected override Condition CreateCondition()
{
return Chrome;
return Chrome & (Document | AnyAncestor(Document));
}
} // class
} // namespace
24 changes: 14 additions & 10 deletions src/Rules/PropertyConditions/Relationships.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Axe.Windows.Core.Misc;
using Axe.Windows.Rules.Resources;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Axe.Windows.Rules.PropertyConditions
Expand Down Expand Up @@ -38,17 +39,19 @@ private static Condition CreateSecondChildCondition()
private static bool MatchOrderInSiblings(IA11yElement e, int index)
{
if (e == null) throw new ArgumentNullException(nameof(e));
if (e.Parent == null || e.Parent.Children == null) return false;
if (index < 1 || index > e.Parent.Children.Count()) return false;
IEnumerable<IA11yElement> siblings = e.Parent?.Children;
if (siblings == null) return false;
if (index < 1 || index > siblings.Count()) return false;

return e.Parent.Children.ElementAt(index - 1).RuntimeId == e.RuntimeId;
return siblings.ElementAt(index - 1).RuntimeId == e.RuntimeId;
}

private static bool HasSiblingsOfSameType(IA11yElement e)
{
if (e?.Parent == null) return false;
IA11yElement parent = e?.Parent;
if (parent == null) return false;

var count = (from c in e.Parent.Children
var count = (from c in parent.Children
where c.ControlTypeId == e.ControlTypeId
select c).Count();

Expand All @@ -66,9 +69,10 @@ private static bool MatchParent(Condition c, IA11yElement e)
{
if (c == null) throw new ArgumentNullException(nameof(c));
if (e == null) throw new ArgumentNullException(nameof(e));
if (e.Parent == null) return false;
IA11yElement parent = e.Parent;
if (parent == null) return false;

return c.Matches(e.Parent);
return c.Matches(parent);
}

public static ValueCondition<int> SiblingCount(Condition c)
Expand All @@ -86,11 +90,11 @@ private static int SiblingCount(Condition c, IA11yElement e)
{
if (c == null) throw new ArgumentNullException(nameof(c));
if (e == null) throw new ArgumentNullException(nameof(e));
if (e.Parent == null) return -1;
if (e.Parent.Children == null) return -1;
IEnumerable<IA11yElement> children = e.Parent?.Children;
if (children == null) return -1;

int count = 0;
foreach (var child in e.Parent.Children)
foreach (var child in children)
{
if (c.Matches(child))
++count;
Expand Down
69 changes: 68 additions & 1 deletion src/RulesTest/Library/ChromiumComponentsShouldUseWebScannerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ public class ChromiumComponentsShouldUseWebScannerTests
{
private static readonly Rules.IRule Rule = new Rules.Library.ChromiumComponentsShouldUseWebScanner();
private Mock<IA11yElement> _elementMock;
private Mock<IA11yElement> _parentMock;

[TestInitialize]
public void BeforeEach()
{
_elementMock = new Mock<IA11yElement>(MockBehavior.Strict);
_parentMock = new Mock<IA11yElement>(MockBehavior.Strict);
}

[TestMethod]
Expand All @@ -37,16 +39,81 @@ public void Condition_FrameworkIsNotChrome_ReturnsFalse()
}

_elementMock.Verify(m => m.Framework, Times.Exactly(nonChromeValues.Count()));
_elementMock.Verify(m => m.Framework, Times.Exactly(nonChromeValues.Count()));
}

[TestMethod]
public void Condition_FrameworkIsChrome_ControlTypeIsNotDocument_NoParent_ReturnsFalse()
{
IEnumerable<int> nonDocumentTypes = ControlType.All.Except(new int[] { ControlType.Document });
_elementMock.Setup(m => m.Framework).Returns(FrameworkId.Chrome).Verifiable();
_elementMock.Setup(m => m.Parent).Returns<IA11yElement>(null).Verifiable();

foreach (int nonDocumentType in nonDocumentTypes)
{
_elementMock.Setup(m => m.ControlTypeId)
.Returns(nonDocumentType)
.Verifiable();

Assert.IsFalse(Rule.Condition.Matches(_elementMock.Object));
}

_elementMock.Verify(m => m.Framework, Times.Exactly(nonDocumentTypes.Count()));
_elementMock.Verify(m => m.ControlTypeId, Times.Exactly(nonDocumentTypes.Count()));
_elementMock.Verify(m => m.Parent, Times.Exactly(nonDocumentTypes.Count()));
}

[TestMethod]
public void Condition_FrameworkIsChrome_ParentControlTypeIsNotDocument_ReturnsFalse()
{
IEnumerable<int> nonDocumentTypes = ControlType.All.Except(new int[] { ControlType.Document });
_elementMock.Setup(m => m.ControlTypeId).Returns(ControlType.Window).Verifiable(); // Arbitrary type
_elementMock.Setup(m => m.Framework).Returns(FrameworkId.Chrome).Verifiable();
_elementMock.Setup(m => m.Parent).Returns(_parentMock.Object).Verifiable();
_parentMock.Setup(m => m.Parent).Returns<IA11yElement>(null).Verifiable();

foreach (int nonDocumentType in nonDocumentTypes)
{
_parentMock.Setup(m => m.ControlTypeId)
.Returns(nonDocumentType)
.Verifiable();

Assert.IsFalse(Rule.Condition.Matches(_elementMock.Object));
}

_elementMock.Verify(m => m.Framework, Times.Exactly(nonDocumentTypes.Count()));
_elementMock.Verify(m => m.ControlTypeId, Times.Exactly(nonDocumentTypes.Count()));
_elementMock.Verify(m => m.Parent, Times.Exactly(nonDocumentTypes.Count()));
_parentMock.Verify(m => m.ControlTypeId, Times.Exactly(nonDocumentTypes.Count()));
_parentMock.Verify(m => m.Parent, Times.Exactly(nonDocumentTypes.Count()));
}

[TestMethod]
public void Condition_FrameworkIsChrome_ParentControlTypeIsDocument_ReturnsTrue()
{
_elementMock.Setup(m => m.Framework).Returns(FrameworkId.Chrome).Verifiable();
_elementMock.Setup(m => m.ControlTypeId).Returns(ControlType.Window).Verifiable(); // Arbitrary value
_elementMock.Setup(m => m.Parent).Returns(_parentMock.Object).Verifiable();
_parentMock.Setup(m => m.ControlTypeId).Returns(ControlType.Document).Verifiable();

Assert.IsTrue(Rule.Condition.Matches(_elementMock.Object));

_elementMock.Verify(m => m.Framework, Times.Once());
_elementMock.Verify(m => m.ControlTypeId, Times.Once());
_elementMock.Verify(m => m.Parent, Times.Once());
_parentMock.Verify(m => m.ControlTypeId, Times.Once());
}

[TestMethod]
public void Condition_FrameworkIsChrome_ReturnsTrue()
public void Condition_FrameworkIsChrome_ControlTypeIsDocument_ReturnsTrue()
{
_elementMock.Setup(m => m.Framework).Returns(FrameworkId.Chrome).Verifiable();
_elementMock.Setup(m => m.ControlTypeId).Returns(ControlType.Document).Verifiable();

Assert.IsTrue(Rule.Condition.Matches(_elementMock.Object));

_elementMock.Verify(m => m.Framework, Times.Once());
_elementMock.Verify(m => m.ControlTypeId, Times.Once());
}

[TestMethod]
Expand Down

0 comments on commit 2db94f5

Please sign in to comment.