core: Respect non-error aria-describedby #1140
Conversation
| ok( !field.valid() ); | ||
| assert.hasError( field, "required" ); | ||
|
|
||
| field.val( "foo" ); |
There was a problem hiding this comment.
Should the assertion checking for the aria-describedby attribute come here? Something like
equal ( field.attr( "aria-describedby" ), "testForm17text-description "testForm17text-error" );Then later, when the field is valid, it should only reference the original description, not there error anymore?
There was a problem hiding this comment.
Ah, yes the test probably should be moved up to this point. However, I didn't want to make any strict enforcement of the order of the ids, since even though it's predictable in this case. It was only necessary for the success of this test to check that each id existed, hence the individual check for each id. I should probably check that the error id isn't present prior to validation also. I'll add this in.
Wouldn't it be sufficient to hide the error label, but allow the reference to remain? We could probably remove the reference, but then we would have to remove the error label and re-add it on every subsequent validation.
There was a problem hiding this comment.
though it's predictable in this case
Let's use the strict test then.
but then we would have to remove the error label and re-add it on every subsequent validation.
Why is that?
Leaving a reference to a hidden element seems problematic to me, that was my primary concern.
There was a problem hiding this comment.
Why is that?
If we remove the reference, then we could possibly end up with an element left in the DOM without any reference between it and the validated field. I suppose we could solve this problem by having a separate reference somewhere else on the input field. a data-error-id field perhaps? If a valid field is subsequently invalidated, then the value of this field could be substituted back into the aria-describedby. How do you feel about this kind of a solution?
|
Hi @jzaefferer, I've fixed the test and the issue with the global replacement. I just have a few implementation queries regarding your suggested enhancement regarding removing the aria-describedby reference for valid fields. Would we need to perform the necessary attribute logic into hideErrors (to ensure that the reference is removed on hide). I've also thought that perhaps we could use events to handle the above behaviour. Perhaps when a field is added to the aria-describedby, a trigger hook could be added that knows how to remove or re-add the field id. $( element ).attr( "aria-describedby", describedBy );
error
.on( "show.validate", function() { /* re-add this to aria-describedby */ })
.on( "hide.validate", function() {
/* remove from aria-describedby, perhaps storing in a temporary data-error-id attribute.
This would be necessary for errorsFor to find this label again for subsequent validations. */
});and then in hideErrors hideErrors: function() {
this.addWrapper( this.toHide ).hide().trigger( "hide" );
}And so on in like fashion. :) How do you feel about my logic? |
|
What happens when an element has an I'm not a fan of using events internally, but we can get back to that if necessary. |
|
I'm not sure, I'd probably need to trial a few screenreaders to see how they behave. Do you have any that you suggest in particular? I'll download and test when I get a moment this week. :) Thanks for the feedback, as always. I'm enjoying contributing to this module. |
|
Sorry for the delay. To answer your questions: On Windows, get NVDA (free, simple to install). On OSX, enable VoiceOver (somewhere in System Preferences, works best on recent OSX versions). On Linux, install a VM with Windows and NVDA... |
|
Hey, thanks @jzaefferer for the reply. I apologise for being busy and neglecting this pull request; I'll fix this up soon. :) I appreciate the tips, thank you. |
|
@tractorcow I'd like to release 1.13 next Tuesday and would want to include this PR. |
|
Well, I haven't fixed the issue yet, but I can confirm that NVDA DOES screen read hidden text if it's set as the aria-describedby. Annoyingly this probably means the fix won't make it into the next release. If you like, you could include THIS fix into this release, and we could mark the screen reader issue as a separate issue. I would like to spend some more time on this but I have other work I must do today. Perhaps I could put some time away tonight and work on another fix, but likely it would be rushed and I never feel confident in rushed code, especially since I rely on your feedback. :) |
|
Hi @jzaefferer, since I really want to make an effort I've made a big push to get this working perfectly. :) Instead of removing the aria-describedby, I've taken the simpler solution of ensuring that every error label, if it exists against a valid field, is emptied. This should mean that there are no longer fields with hidden error labels with old messages in them. This means that screen readers have no invalid text to even try to read out. Voila. I've added an explicit check case to the new test case, as well as updating the |
|
Thank you so much. I did a test run with browserstack-runner (a few existing failures, nothing related to this PR), tested manually with some of the demos and did a code review. Only issue I could find was some missing whitespace on the As a token of my appreciation and the hope to have you contribute more in the future, I've made you a collaborator on the GitHub repository. Its mostly symbolic, though please don't push to master directly. Thanks again! |
|
Wow thanks @jzaefferer. I promise never to abuse this power, other than the necessary boasting at the bar to impress the ladies. I really just want the code to work well. :) |
To follow up from my previous pull request at #1083 I've improved this solution to better respect existing aria-describedby that do not refer to error labels.
Previously jquery-validate would monopolise this field without regard for aria-describedby references which may point to valid descriptive elements related to the source field (such as captions, right titles, or so on).
It was pretty trivial to reduce the scope of errorsFor to select only error elements, and update showLabel to merge new error IDs with the existing aria-describedby rather than replacing it.