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

Settings: Focused input elements, values, modified state not read by screenreader #54836

Closed
cleidigh opened this issue Jul 23, 2018 · 20 comments
Closed
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug dropdown DropDown (SelectBox widget) native and custom issues settings-editor VS Code settings editor issues

Comments

@cleidigh
Copy link
Contributor

cleidigh commented Jul 23, 2018

The select box elements utilized within the new settings do not get their titles read
while the options appear to be read correctly. This behavior is different from
selected boxes contained within an action bar or individually rendered.

@roblourens

I am pretty sure this has to do with nested elements within a tree or the dynamic way the page is rendered. I'm trying to figure out how to test that.
I am researching changes made to ARIA 1.1 with many roles.

https://www.levelaccess.com/differences-aria-1-0-1-1-changes-rolecombobox/

@cleidigh cleidigh self-assigned this Jul 23, 2018
@cleidigh cleidigh added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues settings-editor VS Code settings editor issues dropdown DropDown (SelectBox widget) native and custom issues bug Issue identified by VS Code Team member as probable bug labels Jul 23, 2018
@cleidigh
Copy link
Contributor Author

cleidigh commented Jul 23, 2018

@roblourens
okay I think I stumbled on the right issue. If the parent select element has role=treeitem
the select title is read correctly, albeit as a tree item not a combo box. Not sure if this is an issue
or if there's a different way to do it. I have to do some more experiments.
Note: We may need to add an option or set function that defines the select box as being contained by
a tree since the roles are different if it's just part of an action bar.

Also I confirmed that this behavior is relevant to all the controls within the tree. This means none of the check boxes or other controls get read out either.

Given that there is a bunch of other content that might have aria issues , where do you want to capture that discussion? I am mainly thinking about the extended descriptions that expand when selected. Is that up for discussion?

@roblourens
Copy link
Member

role=treeitem

That sounds promising. Maybe these controls need to take a role in their options.

What I do about reading the descriptions, etc largely depends on the outcome of #54039. Maybe it goes into the aria-label for the setting item itself, or maybe we use aria-describedby or something else.

@cleidigh
Copy link
Contributor Author

I am going to add this as an option, otherwise we get nothing but we land on the select boxes.
If we can figure out a better way I can change it later but I think this is correct.

@cleidigh
Copy link
Contributor Author

cleidigh commented Jul 23, 2018

@roblourens
The labeling of the select box works, I thought it would be easy to do the same thing for the other controls.
I can get it to partially work but I don't think that's what it should be. The screenshot below shows the output for two different approaches. The first get the correct label but does not output the current value.
The second outputs just the value. does not make sense to me, would you like me to continue to work on this since I think it's relatively divorced from the layout ?

Reverse example order...

image

@roblourens
Copy link
Member

Yes you are welcome to work on it as much as you want!

@cleidigh
Copy link
Contributor Author

cleidigh commented Jul 25, 2018

@roblourens

ARIA labeling within a tree gets tricky and appears to have quite a few either quirks
or behaviors that appear to be inconsistent and not intuitive. I did a lot of research
and experimentation, some of which did not seem consistent.

I now have all controls reading the settings value when the input element gets focused.
It also reads the modified state.

Example:
level 1 Editor Font Size number selected 16 Modified

Who is the ARIA guru ? Perhaps my solution is not the best practice. I could not find
any documentation that clarified the situation. Mostly had to experiment and see results
using NVDA.

I have not addressed setting descriptions including embedded links. I figure it's better to wait
until the layout discussion is resolved.

Should I clean up and merge these changes in so at least we have some output? Probably will try to use
some helper functions to reduce code.

	private renderNumber(dataElement: SettingsTreeSettingElement, isSelected: boolean, template: ISettingTextItemTemplate, onChange: (value: number) => void): void {
		template.onChange = null;
		template.inputBox.value = dataElement.value;
		template.onChange = value => onChange(parseFn(value));
		template.inputBox.inputElement.tabIndex = isSelected ? 0 : -1;

		const parseFn = dataElement.valueType === 'integer' ? parseInt : parseFloat;

		// Setup and add ARIA attributes
		// Create id and label for control/input element - parent is control div
		const id = (dataElement.displayCategory + '_'+dataElement.displayLabel).replace(/ /g, '_');
		const label = dataElement.displayCategory + ' ' + dataElement.displayLabel + ' number selected ' + dataElement.value + ' ' + template.isConfiguredElement.textContent;

		// We use the parent control div for the aria-labelledby target
		// Does not appear you can use the direct label on the element itself within a tree
		template.inputBox.inputElement.parentElement.setAttribute('id', id);
		template.inputBox.inputElement.parentElement.setAttribute('aria-label',label);

		// Labels will not be read on descendent input elements of the parent treeitem
		// unless defined as role=treeitem and indirect aria-labelledby approach
		template.inputBox.inputElement.setAttribute('role','treeitem');
		template.inputBox.inputElement.setAttribute('aria-labelledby',id);
		template.inputBox.inputElement.setAttribute('aria-readonly', 'true');
	}

@cleidigh cleidigh changed the title Settings:SelectBox: Focused drop-down select element not read by screenreader Settings: Focused input elements, values, modified state not read by screenreader Jul 25, 2018
@roblourens
Copy link
Member

I think we are all just figuring it out as we go :)

I would like to have those changes, thanks. Why .setAttribute('aria-readonly', 'true');?

@cleidigh
Copy link
Contributor Author

cleidigh commented Jul 27, 2018

@roblourens

Ahh, you spotted the one thing I didn't really comment on !

Besides the issue of input controls seemingly requiring role=treeitem,
I basically found each of the input control types has some oddities
when trying to get labels to work under a tree. Notably I was able
to get input elements to read out their labels WITHOUT adding role=treeitem .
This only worked under chrome directly but, I think that may be related
to how our lists are implemented.

If you look at the label for the number input box example, I actually
simulate some of the expected labeling behavior.

  • role=treeitem, override some of the expected behavior of the controls
    I actually add 'number' into the label as this normal role does not get read.

  • I also artificially add the number value into the label , 'selected' as well

  • Finally, by default input controls are 'editable' , normally a reader
    would read something like:

    Setting Item number 100 selected editable
    selected 100

    Adding readonly disables the reading of both 'editable' and the readout of what
    is the selected value, I think the editable is implicit and redundant for our case
    but we can always revisit that.

    The main reason I added this is for a workaround on the 'selected' output.
    Look at the example output , the first is with read-only the second as default editable
    the 'selected File' is what is read, but this is actually the first four characters
    of the label, not '1000' I think this is a bug since the label is not the value.
    My workaround manually adds the selected and value to the label.

    If this is more than you wanted to know, sorry , but it works !!
    If you want to know some more of the details I'd be happy to recount an answer.

I will get these changes in. After that, let me know if there's anything else for the milestone
concerning settings.

With read-only:

Settings tree view
blank
level 2 Files Auto Save, Setting selected
Files Auto Save Delay, Setting selected level 2
level 1 Files Auto Save Delay number selected 1000

Without read-only (only change)

Settings tree view
level 2 Files Auto Save Delay, Setting selected
level 1 Files Auto Save Delay number selected 1000 editable
selected File

@roblourens
Copy link
Member

roblourens commented Jul 27, 2018

I'm worried about setting aria-readonly to trick the screenreader, when the control is editable. Reading "editable" does seem redundant, but if that's what users expect for an editable control, I think we should keep that. But I'm fine with the other changes. Thanks for the help!

@cleidigh
Copy link
Contributor Author

okay but per above there are two issues 'editable' and the output for 'selected' which is broken.

I can add the 'editable' artificially in the label to avoid the second issue. That would make the screen reader output alert the user, but the internal accessibility tree would consider the element not 'editable'

I think that should be okay I can't think of an operational problem with the above.

you okay with that approach?

@cleidigh
Copy link
Contributor Author

of course on updating to current master something is changed ... have to figure out another quirk first
the read-only seems to stop working WTF :-(

@roblourens
Copy link
Member

I read your answer again, I guess I don't follow what 'selected' means or why it's related to whether the control is editable.

but the internal accessibility tree would consider the element not 'editable'

This is what I'm nervous about, I don't know whether adding it to the label manually will produce the same experience, or if it will confuse some tools.

@cleidigh
Copy link
Contributor Author

I totally agree about dubious workarounds, that's why I was frustrated not finding a definitive , documented solution to this.

to reiterate the issue more clearly:

If a input has aria-readonly=false OR there is no added attribute, the control is 'editable' and that word is read after the label.

The reader also reads the input selection which in my example should be 'selected 1000'
instead the output is 'selected File'
the screen reader erroneously takes the number of characters selected of the true value 1000 = 4 characters but of the label not the value

The problem is if we use aria-readonly= false we get that error, with some recent change now the read-only seems to be ignored and we get the editable and erroneous selected text reader output.

I will keep experimenting, but isn't there an accessibility team we can ask?

@roblourens
Copy link
Member

We just switched back to Electron 2. I wonder if that's responsible for the new change you see.

I'll ask around but I don't know of any experts nearby.

@cleidigh
Copy link
Contributor Author

I think you might be correct I was aware of the electron 2 just not exactly when it happened
this does appear to be and interaction between electron/Chrome and NVDA
I think I can also confidently say there is an error between those two. I just did a bunch more experiments and it is clear the read-only attribute has changed behavior and is broken.

Currently I have come up with two choices:

Read '1000 selected' with no precursor label e.g. File Auto Save

SecondChoice
Read '1000 selected' with a suffix label for the actual setting
1000 selected File Auto Save

we could add units and current setting and modified BUT ONLY as a suffix
this annoys me as it seems like something pretty basic but input controls with trees don't seem to be normal

If you have a preference for one of the above two let me know I figure I converge in one of the two
until we have a proper answer, neither of my two possibilities above should violate any norms
they are just not perfect output

@roblourens
Copy link
Member

Yeah, that is annoying. I guess the second is preferable.

@cleidigh
Copy link
Contributor Author

cleidigh commented Jul 27, 2018

well now that I knew what I'm looking for I found #5544
that relates to the tree item issue, FYI @bpasero @zersiax
working on a similar issue above, I don't think we have any input elements within a tree currently
that would match this problem with labeling so this is yet another use case

I will implement option two above until we have a better approach.
also will post an issue with an NVDA

@roblourens
Copy link
Member

roblourens commented Jul 27, 2018

Oh wow, that issue predates me. Good find.

I am using the tree control here, but it does not currently really function as a "tree". I could transition away from the tree in the future, but if simply hacking the role on this tree's elements would make this all easier to deal with, I'm ok with that. I'm assuming that wouldn't cause issues for screenreader users since it doesn't function as a real tree anyway.

@cleidigh
Copy link
Contributor Author

I also just found something related within the NVDA issues, not exactly our problem but related

I think you would have a lot of rework if not using a tree , the role issue gets us something
and I don't think that's a hack rather a requirement. what I just don't understand is why
this is really an issue at all, I need to get a better handle on the debugging tools within the reader
these might be able to help.

@cleidigh
Copy link
Contributor Author

I don't think input elements are really supported inside a tree. ARIA grid specifically do, but some of the output appears different. I'm going to try a nested grid.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug dropdown DropDown (SelectBox widget) native and custom issues settings-editor VS Code settings editor issues
Projects
None yet
Development

No branches or pull requests

2 participants