Skip to content
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

[Feature Request] Make scans against Chromium-based UIs more actionable #836

Closed
dbjorge opened this issue Nov 11, 2022 · 9 comments
Closed

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Nov 11, 2022

Is your feature request related to a problem? Please describe.

This issue is based on discussion with @RobGallo and @DaveTryon after several customer interactions where users attempting to scan apps that are Chromium-based get confused.

The most common case is a user trying to scan a browser (Edge, Chrome, etc) with their page loaded, but we've also seen cases where a user has their own Windows app that embeds Chromium (eg, via Electron or WebView2).

There are two related problems that happen when a user does this:

  • Most ATs special case Chromium instances such that it's a lot more useful for practical accessibility purposes to scan web content with a web scanner like Accessibility Insights for Web, rather than with axe-windows/Accessibility Insights for Windows. However, axe-windows results don't make any mention of this, even though we could easily distinguish these cases programmatically (by looking for a FrameworkId UIA property of "Chrome").
  • Even with an absolutely minimal WebView2 usage serving an empty page with no issues, there are a few low-severity issues that axe-windows flags that are essentially framework issues (~10 BoundingRectangleNotNull violations). Users can't do anything about these, but we don't currently treat them as framework issues.

Describe the solution you'd like

Rob and I think we'd ideally handle this on the axe-windows side similarly to how we treat old edge - create a rule which specifically detects an embedded chromium instance (via FrameworkId "Chome") and present a specific error that directly instructs users to scan the contents of the browser with a web-based scanner. Such a rule should probably somehow suppress any further errors in descendants of the root "Chrome" element - this will require some new form of handling, we don't currently have any other rules that do this type of subtree suppression.

It's a little questionable to call this an "Error" since we don't really want to treat it as an error to use a WebView2 at all, but we don't really have a better means to present actionable feedback to a user about needing an extra step for scanning such a component. I think that a single actionable error would clearly be a lot better than the current case of "dozens or hundreds of inactionable errors", though.

Describe alternatives you've considered

  • The real gold standard in terms of customer ease-of-use might be to actually perform an axe-core scan inside the detected chromium instance somehow. But we think this would probably be technically infeasible to do automatically unless the Chromium instance were accepting CDP debugger commands, and it would be a huge chunk of work to present it reasonably.
  • It would be good to investigate/file bugs against the BoundingRectangleNotNull violations that are consistently present regardless of page content. We did some initial investigation and think this is probably an issue on the Chromium side rather than the WebView2/MSAA_Proxy side, but didn't nail down an exact location. If we wanted to follow up on this, good contacts to reach out to for more context would be the folks marked as the Chromium windows accessibility platform OWNERS. However, we think pushing on these are pretty low priority; the issues themselves are such low severity that we think it would be hard to get traction on them, so if we end up making the proposed rule above that would mask them anyway, it doesn't feel worth the effort to try to drive them to resolution.

Additional context

n/a

@ghost ghost added the status: new This issue is new and requires triage by DRI. label Nov 11, 2022
@ghost ghost assigned dbjorge Nov 11, 2022
@dbjorge dbjorge added status: ready for triage This issue is ready to be triaged by the Accessibility Insights team. feature request labels Nov 11, 2022
@ghost ghost assigned asksep Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

This issue has been marked as ready for team triage; we will triage it in our weekly review and update the issue. Thank you for contributing to Accessibility Insights!

@ghost ghost removed the status: new This issue is new and requires triage by DRI. label Nov 11, 2022
@ferBonnin
Copy link
Contributor

Triaged with the team.
We will show a Framework rule error and then eliminate or not scan the Chromium component

sfoslund added a commit that referenced this issue Jan 27, 2023
…871)

#### Details

This PR is 1/2 of addressing
#836. It includes the
following commits/changes, which can be reviewed separately:

-
27295d6
adds a new sample project to the tools repo that uses a minimal
webview2.
-
b007694
adds the `ChromiumComponentsShouldUseWebScanner` rule and a new e2e test
leveraging the new web view sample added in this PR.

##### Motivation

Addresses issue #836

##### Context

This PR only adds the new rule logic but does not do the work to
suppress any further errors in descendants of the root "Chrome" element
(hence why there are 35 expected errors in the end to end test instead
of just 1). A future PR will add the suppression logic separately.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue:
[#0000](#836)
@DaveTryon DaveTryon added status: needs investigation This issue requires investigation (PM, Design, or SWE) by the Accessibility Insights team. and removed status: ready for triage This issue is ready to be triaged by the Accessibility Insights team. labels Jan 30, 2023
@microsoft-github-policy-service
Copy link
Contributor

This issue requires additional investigation by the Accessibility Insights team. When the issue is ready to be triaged again, we will update the issue with the investigation result and add "status: ready for triage". Thank you for contributing to Accessibility Insights!

@DaveTryon
Copy link
Contributor

After discussion in Windows triage, this will require some fundamental changes. We feel this needs to be treated as a full feature and not as a kluge fix.

@microsoft-github-policy-service
Copy link
Contributor

This issue requires additional investigation by the Accessibility Insights team. When the issue is ready to be triaged again, we will update the issue with the investigation result and add "status: ready for triage". Thank you for contributing to Accessibility Insights!

@Gso-1

This comment was marked as spam.

@Gso-1

This comment was marked as spam.

@DaveTryon DaveTryon moved this to Old backlog in Accessibility Insights Jun 28, 2023
@DaveTryon DaveTryon moved this from Old backlog to Tabled in Accessibility Insights Jul 17, 2023
DaveTryon added a commit that referenced this issue Jul 26, 2023
#### 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
@DaveTryon DaveTryon removed the status: needs investigation This issue requires investigation (PM, Design, or SWE) by the Accessibility Insights team. label Jul 26, 2023
@DaveTryon DaveTryon moved this from Tabled to Needs release in Accessibility Insights Jul 26, 2023
@DaveTryon
Copy link
Contributor

The Chromium rule now triggers exactly once for each Chromium Document. All rules are suppressed for elements within the Chromium Document

@codeofdusk
Copy link
Contributor

Released as part of 2.2.0. Closing as complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants