-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Add advanced setting to allow UIA in chromium based browsers #11079
Conversation
On my machine, NVDA completely freezes with this PR in use:
|
What version of chrome was this? I think it is likely that Microsoft works on the UIA implementation separately from the chromium project and that it's only merged into chromium when a major milestone has been reached. Therefore, we couldn't be sure that the UIA implementation in Edge 81 is the same as in Chrome 81, etc. |
Running Chrome 83.0.4103.34. |
I've just installed Chrome Canary (version 84) and retested. I opened Chrome, NVDA hung and I closed it. I get the following in the log:
|
I also installed Chrome Canary. The point is that, when using alpha and force UIA so it will at least work on the address bar, the address bar itself isn't very accessible. Therefore I don't think these UIA implementations are in sync. |
NV access is interested in this pr, specifically to support Edge on Windows 10 S with the NVDA store app (which cannot inject in-process). Edge 81 does not have the UIA freezing bugs last time I tested. I think we should only enable UIA for msEdge. Not all of Chromium, if this is possible. |
If the improved UIA implementation makes it to Chrome, wouldn't it significantly improve performance to enable it there as well? |
We are still a long way off from UIA surpassing
IAccessible2+virtualBuffers on the web in performance and quality. This
may be a goal we are heading towards, but right now I feel we should
only do it where it is absolutely necessary.
Also, This pr should honor both the advanced setting, and also if NvDA
can't inject (appModule.helperLocalBindingHandle is 0) similar to how
Microsoft Word works.
|
Hi, provided that Google follows UIA best practices in Chromium/Chrome combo. Thanks.
|
I'm happy to change this into something that only works in Edge, but it might help to know how Chrome and Edge UIA implementations relate to each other. @michaelDCurran, would this be something Microsoft and/or Google could inform us about? Note that in Chrome, the UIA support is still considered to be experimental and still needs a command line parameter to work. If they are the same implementations though, I don't see a reason to block UIA from working in Chrome if someone explicitly passes the experimental uiautomation cmd parameter. |
I don't believe that Google has any plans to turn on UIA by default any
time soon. Therefore, I think I agree it is okay to allow UIA to be used
for all Chromium implementations if it is available and either the
advanced setting in NVDA is on or NVDA cannot inject in-process.
But we would not want to publish the idea of UIA with Chromium too
heavily here. UIA right now is not yet at all a suitable replacement
where IAccessible2/virtualBuffers is possible.
This may very well change in the near future, but even if it did, it
would only be useful in the latest builds of Windows 10.
But, for Edgium in Windows 10 S, it is necessary that NVDA can use UIA
in that scenario.
|
Agreed. So either the documentation should contain a huge disclaimer, or we should advertise the option as Edge specific, yet being chromium specific under the hood. I'd go for the former. We can reword the option in such a way that it mentions Edge and supported Chromium based browsers, for example. |
Hi, maybe the label would say something to the effect of “Use UIA in Chromium-based Microsoft Edge (experimental)”? Thanks.
|
@michaelDCurran I have updated the initial description to reflect the current state of the pr. While I started this work, I"m afraid I neither have the time nor the skills to work further on this. You really know UIA much better than I do. May be @josephsl might be interested to take a look as well? Note that the new UIA.web module contains code from the edge module, all moved around, so that makes reviewing this code a bit easier. |
Hi, I think it might be best to rename UIA.web to UIA.chromium for sake of consistency with IAccessible.chromium so that EdgeHTML code can be kept for backward compatibility and to support PWA’s. Thanks.
|
…e them in format fields, as headings were being reported twice.
…completely before the start of the parent textRange. Stops a freeze on some checkboxes in Edgium and is generally safer.
…ctly in browseMode in Edgium.
…s from being used.
This comment has been minimized.
This comment has been minimized.
@michaelDCurran Thanks for taking this further! |
In order for existing logic around when NVDA should and should not report the accessible name for a control, Chromium / Edge needs to expose aria-label in the UIA element's AriaProperties property. Edge classic did this previously, but Chromium's UIA implementation does not. |
…, and that the names of controls (that do not use their name as content) are always reported.
@codeofdusk Would you mind testing this branch again, and let me know if you see any freezes. Rememeber to turn on Use UIA in Edge and other Chromium-based brwosers in NvDA's advanced panel. |
This comment has been minimized.
This comment has been minimized.
@michaelDCurran Could you double check the PR description to make sure it is still accurate please? |
This comment has been minimized.
This comment has been minimized.
Running Chrome with the --enable-experimental-ui-automation flag now uses UIA as expected (confirmed in Python console) and documents are browsable. Opening this page in IA2 Chrome causes a hang of several seconds. The same page in UIA Chrome loads almost instantly, but attempting to move by heading causes the browser to crash: NVDA traceback
|
I've been looking at this, but I'm not sure how to review it effectively. Without tests or further documentation about why the changes are made and the circumstances in which they are required I'd only be rubber stamping it. I'd like to see a plan for making this area of the code easier to understand. How can we move away from large recursive functions with many pathways? |
Hi, one thing to keep in mind is what Narrator is doing these days – accessing Chromium’s IAccessible2 implementation through UIA, and a discussion around this time last year about making NVDA work with Edge 79 and later through IAccessible and a renewed effort to use UIA for Chromium support (required on Windows 10 in S mode). Thanks.
|
One option is to rebase these commits to clean up the history. So that each commit message can explain the need for the change. Move classes / functions around first (without further modification), then further commits to fix up the things that need fixing with full explanations of why it needs to change in the commit message (eg the scenario where there is an issue). This would be with the goal to merge this rather than squash merge. |
Closing this PR as it is replaced by #12025 |
Link to issue number:
Related to #10555
Summary of the issue:
UIA is now forcefully disabled in chrome based browsers. While this makes perfect sense, it could be beneficial for developers and/or development purpose to circumvent this without hacking.
Description of how this pull request fixes the issue:
Known issues (Todo)
Testing performed:
Change log entry:
t.b.d.