microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.93k stars 28.77k forks source link

orca talking excessively when triggering problem view. #145109

Open jvesouza opened 2 years ago

jvesouza commented 2 years ago

Issue Type: Bug

CC: @isidorn CC: @joanmarie

Steps to reproduce:

  1. Make sure that orca is running
  2. Open a empty .js file
  3. Insert the following code: var a = 1; var b = 2; var c = 3;
  4. Move the cursor to begin of the file
  5. Press ctrl+shift+m

Expected results: orca should announce the following: No problems have been detect in the workspace. Actual results: Orca announces that No problems have been detect in the workspace and then reads the contents of the file.

I can't say for sure, but it seems that the problem appeared after VSCode switched to electron version 17. This problem does not appear when I use VSCode version 1.65.

VS Code version: Code - Insiders 1.66.0-insider (3e5c7e2c570a729e664253baceaf443b69e82da6, 2022-03-14T05:15:27.407Z) OS version: Linux x64 5.16.14-arch1-1 Restricted Mode: No

System Info |Item|Value| |---|---| |CPUs|Intel(R) Core(TM) i5-5200U CPU @ 2.20GHz (4 x 2494)| |GPU Status|2d_canvas: enabled
canvas_oop_rasterization: disabled_off
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
oop_rasterization: disabled_off
opengl: enabled_on
rasterization: disabled_software
raw_draw: disabled_off_ok
skia_renderer: enabled_on
video_decode: disabled_software
video_encode: disabled_software
vulkan: disabled_off
webgl: enabled
webgl2: enabled| |Load (avg)|0, 1, 1| |Memory (System)|15.54GB (1.86GB free)| |Process Argv|-n . --crash-reporter-id bcecf5e0-97d1-4830-b28c-0943c4adab1c| |Screen Reader|yes| |VM|0%| |DESKTOP_SESSION|gnome-xorg| |XDG_CURRENT_DESKTOP|GNOME| |XDG_SESSION_DESKTOP|gnome-xorg| |XDG_SESSION_TYPE|x11|
Extensions (39) Extension|Author (truncated)|Version ---|---|--- vscode-eslint|dba|2.2.2 EditorConfig|Edi|0.16.4 prettier-vscode|esb|9.3.0 shell-format|fox|7.2.2 rest-client|hum|0.24.6 file-downloader|min|1.0.11 mindaro|min|1.0.120220125 vscode-docker|ms-|1.20.0 csharp|ms-|1.24.1 vscode-aks-tools|ms-|0.0.13 vscode-kubernetes-tools|ms-|1.3.7 python|ms-|2022.1.1883053198-dev vscode-pylance|ms-|2022.2.4-pre.1 remote-containers|ms-|0.226.0 remote-ssh|ms-|0.76.1 remote-ssh-edit|ms-|0.76.1 azure-account|ms-|0.10.1 powershell|ms-|2021.12.0 vsliveshare|ms-|1.0.5418 vsliveshare-audio|ms-|0.1.91 oracledevtools|Ora|21.4.0 vscode-boot-dev-pack|Piv|0.1.0 vscode-concourse|Piv|1.32.0 vscode-manifest-yaml|Piv|1.32.0 vscode-spring-boot|Piv|1.32.0 fabric8-analytics|red|0.3.5 java|red|1.4.0 vscode-commons|red|0.0.6 vscode-xml|red|0.19.1 vscode-yaml|red|1.5.1 sonarlint-vscode|Son|3.3.3 vscodeintellicode|Vis|1.2.17 vscode-java-debug|vsc|0.38.0 vscode-java-dependency|vsc|0.19.0 vscode-java-pack|vsc|0.22.0 vscode-java-test|vsc|0.34.2 vscode-maven|vsc|0.35.1 vscode-spring-boot-dashboard|vsc|0.3.0 vscode-spring-initializr|vsc|0.9.0
A/B Experiments ``` vsliv695:30137379 vsins829:30139715 vsliv368:30146709 vsreu685:30147344 python383:30185418 vspor879:30202332 vspor708:30202333 vspor363:30204092 pythonvspyl392:30422396 pythontb:30258533 pythonvspyt551cf:30291413 pythonptprofiler:30281269 vshan820:30294714 pythondataviewer:30285072 vscod805cf:30301675 pythonvspyt200:30323110 bridge0708:30335490 bridge0723:30353136 vsaa593:30376534 pythonvs932:30404738 wslgetstarted:30449409 vsclayoutctrc:30405799 dsvsc009:30440021 pythonvsnew555:30438690 vscscmwlcmc:30428972 vscgsvid1:30433757 cppdebug:30451566 dockersurvey3cf:30438169 pynewfile477:30451556 ```
jvesouza commented 2 years ago

After I updated vscode there seems to have been a small change in behavior. Now the contents of the file are read before the no-problem announcement.

The commit of the version I'm using is 79d9bb8754331225372e892393619d4072474a19

isidorn commented 2 years ago

@jvesouza thanks for filling this issue. I can not reproduce, this works fine for me and the content of the editor does not get read. Though I am using an older version of orca since I am having issues updating to latest on my Ubuntu.

@joanmarie might you give this a try when you find time?

jvesouza commented 2 years ago

I tested initially using version 42.0 of orca but I just tested with version 41.2 and the result here was the same.

joanmarie commented 2 years ago

It looks like an unfortunate series of events. Heuristics are why we can't have nice things. :wink:

  1. Ctrl+Shift+M is pressed.
  2. Focus is given to the problem view which Orca presents.
  3. Chromium/Electron then fires a text-insertion event on the file contents as if those contents were newly written/added to the document. And Orca tries to guess why. It sees that the insertion is in editable content, and that a keyboard command was just given, so maybe it was inserted in response to that command and errs on the side of reading the text than being silent.

I might be able to fix this on the basis that the editable field doesn't claim to be focused. But I don't know if that will break anything. I've just branched for Orca v42, so I can safely risk breaking master. :grin: That said....

@isidorn Any idea why the text field associated with the file is (re)inserting text as if it were newly written? Not doing that would be a safer fix I think.

EDIT: It looks like the text field's accessible object is getting destroyed and recreated. That might be a side effect of some changes which were made -- and subsequently revised -- regarding what happens when an accessible object gets reparented. So hopefully this will just go away on its own. That said, I think I'll still make the tweak to Orca master and see how that works out.

joanmarie commented 2 years ago

Ok, Orca master no longer re-presents the entire file, but it's sure chatty about that problem view. Lemme see if I can make that suck less.

joanmarie commented 2 years ago

New question for you @isidorn: The 'Problems (Ctrl+Shift_M) - Total 0 Problems' object has the ARIA tab role. It's firing an expanded event. Any chance that's coming from a change in VSCode rather than something odd Chromium is doing?

isidorn commented 2 years ago

@joanmarie thanks for the great investigation and the fix. Once Ctrl+Shift+M is pressed the focus is moved to the element with "No problems have been detected", that object has no tab role or any aria-expanded stuff in it's parent chain. The object you are referring to is the tab in the panel view - part that reads PROBLEMS. That one should not get focus. It should always have aria-expanded=true set on it and role=tab - those things do not change on the vscode side. The only way they could change is in the initial render. So do you see the change coming if you press Ctrl+Shift+M and the bottom panel is already visible? Or you only see it when it is rendered for the first time?

I believe this is the place where we set the aria-expanded https://github.com/microsoft/vscode/blob/main/src/vs/workbench/browser/parts/compositeBarActions.ts#L698

I just tried out putting a breakpoint here and this state indeed seems to flicker on the first render. @sbatten Might you know more here? Can we try to keep the checked state more stable, so we do not set too many expanded events?

joanmarie commented 2 years ago

So do you see the change coming if you press Ctrl+Shift+M and the bottom panel is already visible? Or you only see it when it is rendered for the first time?

If the bottom panel is already visible and the "problems" tab is already selected, I do not see an expanded-changed event.

If, however, the bottom panel is already visible but some other tab (e.g. "output" is selected), I do see an expanded-changed event.

Possibly-related aside: The second scenario makes me wonder if this change is being done deliberately as a way to say "this is the currently-active tab." Which would conceptually make sense, but is not quite what the ARIA spec suggests:

For a multi-selectable tablist, authors SHOULD ensure that the tab for each visible tabpanel has the aria-expanded attribute set to true, and that the tabs associated with the remaining hidden tabpanel elements have their aria-expanded attributes set to false.

But this isn't a multi-selectable tablist. The spec then says:

Authors SHOULD ensure that a selected tab has its aria-selected attribute set to true, that inactive tab elements have their aria-selected attribute set to false, and that the currently selected tab provides a visual indication that it is selected.

Granted those are SHOULD and not MUST, and aria-expanded is officially supported on tab without any normative qualification about when it is supported. :woman_shrugging:

Having said all that, I believe I see a way to filter out the expanded noise on the Orca side of things -- a change which makes sense independent of what you might modify in VSCode. runs off to do that

joanmarie commented 2 years ago

Update: I made the change in Orca so it's less chatty in response to the expanded change. But I still think we can make things better via VSCode changes.

Observations:

Thoughts?

isidorn commented 2 years ago

@joanmarie yeah you got this right, that was the idea for aria-expanded here.

I think your suggestions make sense. For

  1. https://github.com/microsoft/vscode/blob/main/src/vs/base/browser/ui/actionbar/actionbar.ts#L213 the actionBar would need to make the role configurable, such that actionBar clients can specify different roles.
  2. We would just remove the ariaLabel here https://github.com/microsoft/vscode/blob/main/src/vs/workbench/browser/parts/compositeBar.ts#L229

I think both are low hanging and make sense. @sbatten what do you think?

jvesouza commented 2 years ago

@joanmarie Thank you very much for the corrections, it was much better.

One thing that is happening here is that when I press ctrl+shift+m ​​to exit the problems view, the entire file is read.

There is still a similar problem which can be reproduced as follows:

  1. Using VScode create two files, a.js and b.js.
  2. Insert the following content into each file: var a = 1;
  3. Select the a.js file, place the cursor in the name of the var a and rename the variable pressing the f2 key. Orca will announce that the variable has been renamed as expected.
  4. Press ctrl+tab to move to the b.js file.

In my environment, in addition to reading the name of the file to which the focus was moved, orca also reads the previous message that informs that the variable a has been renamed.

sbatten commented 1 year ago

The actionbar/compositebar changes have been done so I think I am no longer needed on this issue.