Bug in showLabel method - creates duplicate aria-describedby elements #1200

Closed
rodgerramjet opened this Issue Jul 6, 2014 · 8 comments

Comments

Projects
None yet
3 participants
@rodgerramjet

Found in 1.13.0

This code (I assume) is trying to send the \b to the regex, but its being escaped by the JS

Should be something like "\b" + errorID + "\b"?

CODE:
} else if ( !describedBy.match( new RegExp( "\b" + errorID + "\b" ) ) ) {
// Add to end of list if not already present
describedBy += " " + errorID;
}

This creates loads of duplicate aria-describedby in the markup

@staabm

This comment has been minimized.

Show comment
Hide comment
Member

staabm commented Jul 6, 2014

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jul 6, 2014

Collaborator

My apologies for this error. While this is a bug, it didn't appear in testing due to the incorrect code being bypassed if showLabel could detect an existing error label to place this element into. When I wrote a few test cases I found this issue only could be replicated if I placed the aria-describedby reference prior to validation being setup. I hope the test case correctly captures your scenario.

Collaborator

tractorcow commented Jul 6, 2014

My apologies for this error. While this is a bug, it didn't appear in testing due to the incorrect code being bypassed if showLabel could detect an existing error label to place this element into. When I wrote a few test cases I found this issue only could be replicated if I placed the aria-describedby reference prior to validation being setup. I hope the test case correctly captures your scenario.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Jul 6, 2014

Member

@davidhedges could you verify that @tractorcow's PR fixes your issue?

Member

staabm commented Jul 6, 2014

@davidhedges could you verify that @tractorcow's PR fixes your issue?

@rodgerramjet

This comment has been minimized.

Show comment
Hide comment
@rodgerramjet

rodgerramjet Jul 6, 2014

Yep that fixes it- thanks guys :)

In terms of how I reproduced it- I haven't gone into too much detail - we are using jquery validate, unobtrusive validation, so a lot of stuff is getting done behind the scenes.

Here is a JSFiddle I threw together for you. Not sure if its an unobtrusive bug exacerbating your bug or not?

http://jsfiddle.net/cW9n5/

Yep that fixes it- thanks guys :)

In terms of how I reproduced it- I haven't gone into too much detail - we are using jquery validate, unobtrusive validation, so a lot of stuff is getting done behind the scenes.

Here is a JSFiddle I threw together for you. Not sure if its an unobtrusive bug exacerbating your bug or not?

http://jsfiddle.net/cW9n5/

@rodgerramjet

This comment has been minimized.

Show comment
Hide comment
@rodgerramjet

rodgerramjet Jul 6, 2014

perhaps your test case should be updated to this scenario?

perhaps your test case should be updated to this scenario?

@rodgerramjet

This comment has been minimized.

Show comment
Hide comment
@rodgerramjet

rodgerramjet Jul 6, 2014

Updated Jsfiddle to show aria-described by on the page- test it by flling in invalid values & tabbing in and out of the field
http://jsfiddle.net/cW9n5/1/

Updated Jsfiddle to show aria-described by on the page- test it by flling in invalid values & tabbing in and out of the field
http://jsfiddle.net/cW9n5/1/

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jul 6, 2014

Collaborator

It looks like this example breaks the code because there is no error label, so showError continually attempts to re-place it on the page. Is this the behaviour that unobtrusive validation circumvents?

Collaborator

tractorcow commented Jul 6, 2014

It looks like this example breaks the code because there is no error label, so showError continually attempts to re-place it on the page. Is this the behaviour that unobtrusive validation circumvents?

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jul 6, 2014

Collaborator

I'm not sure we could update this test case to depend on third party libraries, if it is the case that this is necessary to replicate the full error. jquery.validate.unobtrusive.js isn't a supported module is it? The new test case I added failed under the old code, so that's probably enough (as far as our responsibility goes here) to validate we've fixed the issue.

Collaborator

tractorcow commented Jul 6, 2014

I'm not sure we could update this test case to depend on third party libraries, if it is the case that this is necessary to replicate the full error. jquery.validate.unobtrusive.js isn't a supported module is it? The new test case I added failed under the old code, so that's probably enough (as far as our responsibility goes here) to validate we've fixed the issue.

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