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

core: Respect non-error aria-describedby #1140

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@tractorcow
Collaborator

tractorcow commented Jun 3, 2014

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.

Show outdated Hide outdated test/error-placement.js
ok( !field.valid() );
assert.hasError( field, "required" );
field.val( "foo" );

This comment has been minimized.

@jzaefferer

jzaefferer Jun 4, 2014

Collaborator

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?

@jzaefferer

jzaefferer Jun 4, 2014

Collaborator

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?

This comment has been minimized.

@tractorcow

tractorcow Jun 4, 2014

Collaborator

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.

@tractorcow

tractorcow Jun 4, 2014

Collaborator

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.

This comment has been minimized.

@jzaefferer

jzaefferer Jun 5, 2014

Collaborator

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.

@jzaefferer

jzaefferer Jun 5, 2014

Collaborator

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.

This comment has been minimized.

@tractorcow

tractorcow Jun 8, 2014

Collaborator

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?

@tractorcow

tractorcow Jun 8, 2014

Collaborator

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?

Show outdated Hide outdated src/core.js
return this
.errors()
.filter( "label[for='" + name + "'], label[for='" + name + "'] *" );
selector = selector + ", #" + describer.replace( /\s+/, ", #" );

This comment has been minimized.

@jzaefferer

jzaefferer Jun 4, 2014

Collaborator

What is the replace supposed to do here? If you want to support more than one id inside aria-describedby, then you need to assume that there's any number, so two, three or more. At least make the replace global?

@jzaefferer

jzaefferer Jun 4, 2014

Collaborator

What is the replace supposed to do here? If you want to support more than one id inside aria-describedby, then you need to assume that there's any number, so two, three or more. At least make the replace global?

This comment has been minimized.

@tractorcow

tractorcow Jun 4, 2014

Collaborator

All it does is turn a list of space separated ids into a jquery filter that would match those ids. The replace just means with an aria-describedby="myid myotherid anotherid" we want a jquery selector similar to the below.

errors.filter("label[for=name], #myid, #myotherid, #anotherid");

The replacement simply adds the necessary verbitage between each id. :)

@tractorcow

tractorcow Jun 4, 2014

Collaborator

All it does is turn a list of space separated ids into a jquery filter that would match those ids. The replace just means with an aria-describedby="myid myotherid anotherid" we want a jquery selector similar to the below.

errors.filter("label[for=name], #myid, #myotherid, #anotherid");

The replacement simply adds the necessary verbitage between each id. :)

This comment has been minimized.

@jzaefferer

jzaefferer Jun 5, 2014

Collaborator

You need to make this a global replace. Otherwise you'll only replace the first occurence:

"myid myotherid anotherid".replace( /\s+/, ", #" )
// "myid, #myotherid anotherid"
@jzaefferer

jzaefferer Jun 5, 2014

Collaborator

You need to make this a global replace. Otherwise you'll only replace the first occurence:

"myid myotherid anotherid".replace( /\s+/, ", #" )
// "myid, #myotherid anotherid"

This comment has been minimized.

@tractorcow

tractorcow Jun 8, 2014

Collaborator

Yes, I see my error, sorry!

@tractorcow

tractorcow Jun 8, 2014

Collaborator

Yes, I see my error, sorry!

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 8, 2014

Collaborator

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?

Collaborator

tractorcow commented Jun 8, 2014

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?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 10, 2014

Collaborator

What happens when an element has an aria-describedby attribute, and the referenced element has display:none? Will the screenreader ignore the attribute? Or, in case there's multiple references, only read out the visible element? If both of those are true, then we're probably good with leaving it as-is (with this patch). Could you check that?

I'm not a fan of using events internally, but we can get back to that if necessary.

Collaborator

jzaefferer commented Jun 10, 2014

What happens when an element has an aria-describedby attribute, and the referenced element has display:none? Will the screenreader ignore the attribute? Or, in case there's multiple references, only read out the visible element? If both of those are true, then we're probably good with leaving it as-is (with this patch). Could you check that?

I'm not a fan of using events internally, but we can get back to that if necessary.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 10, 2014

Collaborator

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.

Collaborator

tractorcow commented Jun 10, 2014

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.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 17, 2014

Collaborator

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

Collaborator

jzaefferer commented Jun 17, 2014

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

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 17, 2014

Collaborator

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.

Collaborator

tractorcow commented Jun 17, 2014

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.

@nschonni nschonni added the Core label Jun 18, 2014

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 20, 2014

Collaborator

@tractorcow I'd like to release 1.13 next Tuesday and would want to include this PR.

Collaborator

jzaefferer commented Jun 20, 2014

@tractorcow I'd like to release 1.13 next Tuesday and would want to include this PR.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 22, 2014

Collaborator

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. :)

Collaborator

tractorcow commented Jun 22, 2014

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. :)

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 23, 2014

Collaborator

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 QUnit.assert.noErrorFor assertion to demand that errors for valid fields are not only hidden, but that they have empty texts. This should be an overall improvement anyway, since there's no valid need (pun intended) for the old text persisting.

Collaborator

tractorcow commented Jun 23, 2014

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 QUnit.assert.noErrorFor assertion to demand that errors for valid fields are not only hidden, but that they have empty texts. This should be an overall improvement anyway, since there's no valid need (pun intended) for the old text persisting.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 23, 2014

Collaborator

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 test/test.js change, added that myself. Great to have this part of the next release.

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!

Collaborator

jzaefferer commented Jun 23, 2014

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 test/test.js change, added that myself. Great to have this part of the next release.

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!

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 23, 2014

Collaborator

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. :)

Collaborator

tractorcow commented Jun 23, 2014

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. :)

@tractorcow tractorcow deleted the tractorcow:pulls/ariarespect branch Jun 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment