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

Moved functionallity of aria label to the iron-autogrow-textarea #484

Closed

Conversation

kalaspuffar
Copy link

@kalaspuffar kalaspuffar commented Feb 23, 2017

@@ -194,39 +194,6 @@
assert(!input.focused, 'input is blurred');
});
});

suite('a11y', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are deleting all the accessibility tests, please replace them with other tests that test that this is somehow still accessible

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the accessibility function has moved to PolymerElements/iron-autogrow-textarea so also have the test.

@notwaldorf
Copy link
Contributor

I am not entirely sure this is the correct approach. Can you explain in more detail why this doesn't work for paper-textarea but works for paper-input, which is accessible and follows the same approach?

Also, re: the iron-autogrow-textarea PR: if you want to label that one accessibly, you should use an iron-label or a label outside the element's shadow dom. If you add a label attribute to that element it implies it has some visual representation, like we do in paper-textarea, which is incorrect. So I don't think that element should have a label attribute.

@kalaspuffar
Copy link
Author

Hi @notwaldorf

The problem we have is due to the encapsulation of webcomponents shadow dom. Might be a problem that should be solved in the browsers perhaps.

Paper-input works and is accessable because the aria-describedby and aria-labeledby refers to elements in the same shadow dom. In this case the label referred is found and read by the screen reader.

But paper-textarea has more functionality and is split in multiple separate elements. So the aria-labeledby referes to an element in the parent element and outside of the current shadow dom context.

I've tried with NVDA, Apple Accessibility and ChromeVox screenreaders and in Chrome where the shadow dom is fully implemented the element is read as "textarea blank" without any label describing the element.

By adding in the accessibility function with a hidden label in the same shadow dom as the textarea the label is seen by the screenreader.

I've also tested in Safari with polyfills and then the screen readers sees the connection and reads the label. Then again Safari has not implemented the full spec of shadow dom and CSS and other context information bleeds from elements so that might be the reason.

I've taken som screenshots from before and after the change in the accessability tooling in chrome developer tools.

Before: https://i.imgur.com/6RUZoZu.png
After: https://i.imgur.com/yP5YmTk.png

Best regards
Daniel

@kalaspuffar
Copy link
Author

Hi @notwaldorf again.

I also tested to add the a11ySuite('label') test to paper-textarea and got the following failure in chrome:

Error: AX_ARIA_04 (ARIA state and property values must be valid) failed on the following element: #textarea

Firefox didn't report on this error but it might not have the full shadow dom implementation either.

Best regards
Daniel

@TomHennen
Copy link

Any further thoughts on this change?

We're using paper-textarea as well and screen-readers currently don't see the label.

Absent this change, are there any known work-arounds for this problem so that we can provide an accessible interface?

@notwaldorf
Copy link
Contributor

This PR doesn't work because without aria-describedby, the paper-textarea won't read the error message and the char counter. Those get populated here: https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavior.html#L423

/cc @robdodson

@kalaspuffar
Copy link
Author

Hi @notwaldorf

You are correct that this functionality will be disabled if we add this fix. And that would be a regression.

Sadly the current implementation don't work, if the browser implementation don't change. You can't target an element outside of your shadow dom. So even if you set that your counter should describe the textarea and the label above the iron-autogrow-textarea should label the textarea these will still not be read by assistive technology.

So this change suggest a fix that will make the label work but without creating something that will send the information from one element to another about what the message of the counter, as an example, to the iron-autogrow-textarea element then the text will not be read.

The add-on architecture seems really nice that you can just add more and they will just add their messages as needed but maybe it's a bit to flexible? or maybe you need another trigger to send aria information?

The best solution might be for the developers of assistive tech to handle this issue but that might require a standard change that allows certain interactions to happen over the boundaries of the shadow doms.

Please tell me if I've miss understood something or if there is a better solution to this problem. We use this fix in our production environment because we have about 3 description fields using the paper-textarea functionality and we have people producing with our tool that uses assistive technology so we need this fix.

And I can put in some time if you want me to rework this pull request I just need to know what kind of changes you want me to make and how you imagine this functionality to work.

/cc @robdodson

Best regards

Daniel

@robdodson
Copy link

I started working on a similar fix over in #549.
I think what might be good is to split this into two PRs. The first PR will focus on just getting labeling to work correctly for the textarea. In a follow up PR we can figure out how we want to plumb the describedby stuff through. I think it's possible to do this but want to get labeling working first.

@notwaldorf
Copy link
Contributor

Closing in favour of #549, like @robdodson mentioned. Let's try to get that PR merged.

@notwaldorf notwaldorf closed this Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants