fix: Reduce chromium noise #951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DaveTryon
commented
Jul 20, 2023
| @@ -27,7 +26,7 @@ public override bool PassesTest(IA11yElement e) | |||
|
|
|||
| protected override Condition CreateCondition() | |||
| { | |||
| return Chrome & (Document | AnyAncestor(Document)); | |||
| return Chrome & Document; | |||
There was a problem hiding this comment.
This is the change that reduces the scope of ChromiumComponentsShouldUseWebScanner from all Chromium elements to just Chromium Documents
DaveTryon
commented
Jul 20, 2023
| @@ -19,17 +20,28 @@ abstract class Rule : IRule | |||
| { | |||
| public RuleInfo Info { get; private set; } | |||
| public Condition Condition { get; } | |||
| protected static Condition DefaultExcludedCondition => IsChromiumContent; | |||
There was a problem hiding this comment.
Exposing this to derived classes gives us a single place to modify the default, then the production constructors of the derived classes just pick up the new value without requiring any additional code change
DaveTryon
commented
Jul 20, 2023
|
|
||
| InitRuleInfo(); | ||
| } | ||
|
|
||
| protected Rule() : this (excludedCondition: DefaultExcludedCondition) |
There was a problem hiding this comment.
By default, exclude DefaultExcludedCondition
DaveTryon
commented
Jul 20, 2023
| _elementMock.VerifyAll(); | ||
| } | ||
|
|
||
| public void Matches_FrameworkIsChrome_IsNotDocument_ParentIsChromeDocument_ReturnsTrue() |
There was a problem hiding this comment.
This test overlaps with the one after it, so could potentially be removed
DaveTryon
commented
Jul 20, 2023
|
|
||
| namespace Axe.Windows.Rules.Library | ||
| { | ||
| [RuleInfo(ID = RuleId.ChromiumComponentsShouldUseWebScanner)] | ||
| class ChromiumComponentsShouldUseWebScanner : Rule | ||
| { | ||
| public ChromiumComponentsShouldUseWebScanner() |
There was a problem hiding this comment.
I could build an argument for renaming this to ChromiumDocumentsShouldUseWebScanner, but that would make the PR harder to review. Happy to put this into a subsequent PR if needed.
DaveTryon
commented
Jul 20, 2023
| @@ -27,7 +25,7 @@ public override bool PassesTest(IA11yElement e) | |||
|
|
|||
| protected override Condition CreateCondition() | |||
| { | |||
| return Chrome & (Document | AnyAncestor(Document)); | |||
| return IsChromiumDocument; | |||
There was a problem hiding this comment.
This is the change that limits ChromiumComponentsShouldUseWebScanner to just scan Chromium Documents
codeofdusk
approved these changes
Jul 20, 2023
There was a problem hiding this comment.
Verified that:
- In a WebView2 application, the "chromium content should use web scanner" rule was reported to fail. No spurious Chromium-related errors were reported.
- In an Electron application, the "chromium content should use web scanner" rule was reported to fail. No spurious Chromium-related errors were reported.
- In a non-web scenario, no change is observed.
RobGallo
requested changes
Jul 26, 2023
Codecov Report
@@ Coverage Diff @@
## main #951 +/- ##
==========================================
+ Coverage 73.70% 74.82% +1.12%
==========================================
Files 398 423 +25
Lines 12046 13093 +1047
==========================================
+ Hits 8878 9797 +919
- Misses 3168 3296 +128
|
RobGallo
approved these changes
Jul 26, 2023
DaveTryon
added a commit
that referenced
this pull request
Sep 1, 2023
#### Details In #951, we intentionally excluded all elements inside a Chromium document from scan results. This makes sense for most developers, since the conversion from HTML to UIA is convoluted and outside the control of the HTML developer. However, the Edge team relies on this functionality to help improve their HTML-to-UIA conversion code. This PR defaults to excluding the elements inside a Chromium document, but adds a way to request that scan results include elements within a Chromium document. This option is exposed in 3 places: 1. In the `Automation` layer 2. In the CLI 3. In the `SelectAction` class (for future use from Accessibility Insights for Windows) ##### Motivation Requested by the Edge team ##### Results [Comparison.zip](https://github.com/microsoft/axe-windows/files/12480720/Comparison.zip) contains `Default.a11ytest` and `WithChromiumContent.a11ytest`, which were captured by running the CLI on the same instance of Edge. The rule comparison is as follows: Message | Default Count | WithChromiumContent count --- | --- | --- An on-screen element must not have a null BoundingRectangle property | 5 | 5 Chromium components should be scanned with an web-based scanner | 2 | 2 The Name must not include the same text as the LocalizedControlType | 2 | 2 The Name property must not include the element's control type | 2 | 2 An element must not have the same Name and LocalizedControlType as its parent | 0 | 4 An element of the given ControlType must support the Text pattern | 0 | 1 The elements that are unique to the WithChromiumContent file are all within the Chromium document ##### Screenshots Case | Snapshot (click to enlarge) | Results (click to enlarge) --- | --- | --- Default |  | WithChromiumContent |  |  Delta |  |  ##### Context <!-- Are there any parts that you've intentionally left out-of-scope for a later PR to handle? --> - The `Actions` project didn't previously reference the `Rules` project directly, so the plumbing may be non-ideal. If we were to use the existing dependencies, `Actions` would call into `Desktop`, which would call into `RuleSelection`, which would call into `Rules`. We can add the 2 missing layers if we think there's value there. - The flag to enable Chromium content is basically global. This will work as expected in the CLI, in AI-Win, and in AccChecker. It will fail if someone creates a scenario where concurrent scans want to have different Chromium behavior. I've taken a YAGNI approach for now, and we can always make it more complex at a future time, should that prove necessary. <!-- 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 --> - [n/a] Addresses an existing issue:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
As called out in #836, the
ChromiumContentShouldUseWebScannerhas caused a lot of confusion. This PR aims to reduce that confusion. It does the following:ChromiumContentShouldUseWebScannerrule will now trigger only on Chromium-generatedDocumentelementsDocumentelements or any of their descendantsDocumentsor any of their descendants. The default behavior can easily be overriddenmocks, provide test constructors that allow unit tests to isolate the class under test from the Chromium suppressionMotivation
Address #836, make the
ChromiumContentShouldUseWebScannerrule less intrusiveAttachments
BeforeAndAfter.zip contains 2
a11ytestfilesBefore.a11ytestcontains scan results from Edge before this changeAfter.a11ytestcontains scan results from Edge after this changeHere are how the numbers line up
Things to note on this table:
If you open
After.a11ytestand 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:
Context
Pull request checklist