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

search-widget(aria-live) status for results found #77031

Merged
merged 6 commits into from Jul 22, 2019

Conversation

@georgebatalinski
Copy link

commented Jul 10, 2019

  • We would like to notify the user about the results we found
    the screen reader will read out the number of results or no result
    aria-live is added at load, as it cannot be added adhoc

Proposal for Issue - #69741

George Batalinski
search-widget(aria-live) status for results found
- We would like to notify the user about the results we found
the screen reader will read out the number of results or no result
aria-live is added at load, as it cannot be added adhoc
src/vs/editor/contrib/find/findWidget.ts Outdated Show resolved Hide resolved
@@ -372,7 +372,9 @@ export class FindWidget extends Widget implements IOverlayWidget, IHorizontalSas
} else {
label = NLS_NO_RESULTS;
}
this._matchesCount.appendChild(document.createTextNode(label));
let myAlert = document.createElement('div');

This comment has been minimized.

Copy link
@isidorn

isidorn Jul 10, 2019

Contributor

Why do we need to introduce a div element? Can you please explain that.

This comment has been minimized.

Copy link
@isidorn

isidorn Jul 10, 2019

Contributor

Also if we decide this this is need myAlert should be a const not a let

This comment has been minimized.

Copy link
@georgebatalinski

georgebatalinski Jul 11, 2019

Author

Why do we need to introduce a div element? Can you please explain that.

  1. aria-live - seems to need a change, beyond just a text update. Without the DIV, the reading of aria-live is not consistent
  2. I investigated to review the accessibility tree - for clues as to why, but it does not seem that there is anyway to access the
    *. chrome://accessibility/
    **. accessibility tab - in the Element inspector is not available

I will try to install this tool - to review it for any clues
https://electronjs.org/docs/tutorial/accessibility#devtron

This comment has been minimized.

Copy link
@georgebatalinski

georgebatalinski Jul 14, 2019

Author

the extra div element - is needed, since normal text changes to the DOM are not enough

https://github.com/microsoft/vscode/blob/master/src/vs/base/browser/ui/aria/aria.ts#L58

@@ -807,6 +809,10 @@ export class FindWidget extends Widget implements IOverlayWidget, IHorizontalSas

this._matchesCount = document.createElement('div');
this._matchesCount.className = 'matchesCount';

this._matchesCount.setAttribute('role', 'alert');

This comment has been minimized.

Copy link
@isidorn

isidorn Jul 10, 2019

Contributor

role: alert and aria-live: assertive should be equivalelnt based on the W3 spec
Does it still work if we use only one of these?

This comment has been minimized.

Copy link
@georgebatalinski

georgebatalinski Jul 11, 2019

Author

correct, works with either

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@georgebatalinski thank you very much for you PR.
I have added comments in the PR which should be addressed. Did you test this works nicely?

@rebornix can you please take this over, since from tommorow I am on a 15 day vacation. I just did an initial review.

@isidorn isidorn added this to the July 2019 milestone Jul 10, 2019

@isidorn isidorn requested a review from rebornix Jul 10, 2019

George Batalinski
@georgebatalinski

This comment has been minimized.

Copy link
Author

commented Jul 11, 2019

@rebornix let me know your thoughts

@georgebatalinski

This comment has been minimized.

Copy link
Author

commented Jul 14, 2019

@isidorn @rebornix after doing some research - it seems we have a way to add an populate
monaco-alert container

Solutions:

  1. import { alert as alertFn } from 'vs/base/browser/ui/aria/aria';
    Downside:
    https://github.com/microsoft/vscode/blob/master/src/vs/base/browser/ui/aria/aria.ts#L71
    We would have to live with the way the updates are triggered - repeatedNtimes which is not necessary and confusing to user

  2. Otherwise, we keep the above solution and use the div to trigger the read

@rebornix

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@georgebatalinski thanks for your help on this. I played with the code change and it works as expected.

Regarding the two solutions you proposed, I found that option #2 actually works the same as aira. alert(LABEL, true). The reason for that is _updateMatchesCount is not executed when the search results count doesn't change, so we won't get the chance to pronounce the same text multiple times.

I'll suggest to simply use aria.alert(LABEL, true) to pronounce the match count when it changes.

@rebornix

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

In addition to that, to make the alert more descriptive, we can probably alert "1/2: Found 'world' at 9:20 when there are results, and "No results found for 'world' when there are no results. Then whenever the search string, result position and result count changes, users get the correct feedback, especially when there are no results.

Test cases I run manually:

Text Document

hello world
hello world

  1. no result
  • put cursor at line 3
  • cmd+f
  • type d
    • Expect: "No results found for d"
  1. result position change
  • put cursor at line 1
  • cmd+f
  • Expect: "1/2: Found 'world' at 1:7"
  1. result position change
  • put cursor at line 1
  • cmd+f
  • Expect: "1/2: Found 'world' at 1:7"
  • F3
  • Expect: "2/2: Found 'world' at 2:7"

Above information can be found by reading _state.searchString and _state._currentMatch.

@georgebatalinski let me know if you need me to make some updates directly ;)

@@ -807,6 +809,9 @@ export class FindWidget extends Widget implements IOverlayWidget, IHorizontalSas

this._matchesCount = document.createElement('div');
this._matchesCount.className = 'matchesCount';

this._matchesCount.setAttribute('aria-live', 'assertive');

This comment has been minimized.

Copy link
@rebornix

rebornix Jul 19, 2019

Member

If we use aria.alert, then this is no longer necessary, right?

This comment has been minimized.

Copy link
@georgebatalinski

George Batalinski added some commits Jul 21, 2019

George Batalinski
aria(add-context) add additional text context
- We use alert from aria base
George Batalinski
@georgebatalinski

This comment has been minimized.

Copy link
Author

commented Jul 22, 2019

@rebornix let me know if it works on your side as expected (It does on mine :)

@rebornix rebornix merged commit bc92427 into microsoft:master Jul 22, 2019

5 checks passed

VS Code Build #20190722.58 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
@rebornix

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

LGTM. thanks!

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@georgebatalinski @rebornix Great work! Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.