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

Fix accessibility #1280

Closed
wants to merge 1 commit into from
Closed

Conversation

stevenMouret
Copy link
Contributor

  • Fix aria-describedby when using data-parsley-error-message
  • Manage aria-hidden attribute on error wrapper

…r-message - Add aria-hidden attribute on error wrapper
var removeDescribedby = this._ui.$errorClassHandler
.removeAttr('aria-describedby');

return removeErrorMessage && removeDescribedby;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This return statement makes no sense to me. Simplest change would be to first removeAttr('aria-describedby') then do as before, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bonjour Marc andré,

I am not sure to understand. In the case there is no input assistance, it is necessary to remove aria-describedby. If we do not remove aria-describedby the technical assistance will try to read an empty ul.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please re-read carefully what I wrote. Nowhere do I propose not removing the aria-describedby.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello Marc André,

Sorry this pull request is open for some time now.
Maybe it is possible to do more simple, but I am not a JS expert. The chunk of code you are highlighting just allow to remove the aria-describedby and the error message. If it is possible to make more simple do not hesitate to refactoring my code. If you need more explanation about the this accessibility changes I can explain more.

@marcandre
Copy link
Collaborator

Great, thanks for the PR. I commented on a return. Also, it would be great if this was part of one of the tests. Are you up to tweak this?

@marcandre
Copy link
Collaborator

I've pushed a modified commit to master, but I'm running into issues on Travis.

I've wasted an hour of my life on incompatibilities that seem to me completely ridiculous but it's still not working.

@stevenMouret
Copy link
Contributor Author

I've pushed a modified commit to master, but I'm running into issues on Travis.

I've wasted an hour of my life on incompatibilities that seem to me completely ridiculous but it's still not working.

I think you're getting the wrong issue. My changes do not affect dependencies.

@marcandre
Copy link
Collaborator

My changes do not affect dependencies.

Indeed, there is no doubt about that.

I think you're getting the wrong issue.

No. I'm just explaining where I'm at. I have never thought or implied that your commit, or my modified version of it, was responsible for the troubles on Travis.

@marcandre
Copy link
Collaborator

Merged in with modifications as a1a5d36

Should be fixed in 2.9.2

Thanks again for the PR 👍

@marcandre marcandre closed this Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants