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

ghost · 2019-07-10T00:11:01Z

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

- 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

isidorn · 2019-07-10T07:57:41Z

src/vs/editor/contrib/find/findWidget.ts

 			label = NLS_NO_RESULTS;
 		}
-		this._matchesCount.appendChild(document.createTextNode(label));
+		let myAlert = document.createElement('div');


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

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

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

aria-live - seems to need a change, beyond just a text update. Without the DIV, the reading of aria-live is not consistent

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

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

isidorn · 2019-07-10T07:59:39Z

src/vs/editor/contrib/find/findWidget.ts

 		this._matchesCount = document.createElement('div');
 		this._matchesCount.className = 'matchesCount';
+
+		this._matchesCount.setAttribute('role', 'alert');


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?

correct, works with either

isidorn · 2019-07-10T08:01:07Z

@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.

ghost · 2019-07-11T20:00:34Z

@rebornix let me know your thoughts

ghost · 2019-07-14T18:05:38Z

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

Solutions:

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
Otherwise, we keep the above solution and use the div to trigger the read

rebornix · 2019-07-19T21:25:50Z

@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 · 2019-07-19T21:56:10Z

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

no result

put cursor at line 3
cmd+f
type d
- Expect: "No results found for d"

result position change

put cursor at line 1
cmd+f
Expect: "1/2: Found 'world' at 1:7"

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 ;)

rebornix · 2019-07-19T21:59:16Z

src/vs/editor/contrib/find/findWidget.ts

 		this._matchesCount = document.createElement('div');
 		this._matchesCount.className = 'matchesCount';
+
+		this._matchesCount.setAttribute('aria-live', 'assertive');


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

- We use alert from aria base

ghost · 2019-07-22T13:52:08Z

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

…ki/77031

rebornix · 2019-07-22T20:06:29Z

LGTM. thanks!

isidorn · 2019-07-29T09:41:55Z

@georgebatalinski @rebornix Great work! Thanks a lot!

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

2e6c318

- 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

ghost mentioned this pull request Jul 10, 2019

Accessibility improvements for finding in files #69741

Closed

bpasero assigned isidorn Jul 10, 2019

isidorn requested changes Jul 10, 2019

View reviewed changes

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

isidorn assigned rebornix Jul 10, 2019

isidorn requested a review from rebornix July 10, 2019 08:01

fix(pr-review) clean up code

c6a663b

rebornix reviewed Jul 19, 2019

View reviewed changes

George Batalinski added 2 commits July 21, 2019 18:28

aria(add-context) add additional text context

c06ae4f

- We use alert from aria base

clean up

5dd61b3

rebornix added 2 commits July 22, 2019 11:30

Merge remote-tracking branch 'upstream/master' into pr/georgebatalins…

ec35d3d

…ki/77031

Handle edge cases

28a403e

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

github-actions bot locked and limited conversation to collaborators Mar 29, 2020

Conversation

ghost commented Jul 10, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidorn commented Jul 10, 2019

Uh oh!

ghost commented Jul 11, 2019

Uh oh!

ghost commented Jul 14, 2019

Uh oh!

rebornix commented Jul 19, 2019

Uh oh!

rebornix commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 22, 2019

Uh oh!

rebornix commented Jul 22, 2019

Uh oh!

isidorn commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rebornix commented Jul 19, 2019 •

edited

Loading