Skip to content

core: Respect non-error aria-describedby #1140

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

core: Respect non-error aria-describedby #1140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ $.extend( $.validator, {
if ( this.settings.unhighlight ) {
this.settings.unhighlight.call( this, element, this.settings.errorClass, this.settings.validClass );
}
this.addWrapper( this.errorsFor( element ) ).hide();
this.hideThese( this.errorsFor( element ) );
}
},
onfocusout: function( element ) {
Expand Down Expand Up @@ -472,7 +472,12 @@ $.extend( $.validator, {
},

hideErrors: function() {
this.addWrapper( this.toHide ).hide();
this.hideThese( this.toHide );
},

hideThese: function( errors ) {
errors.not( this.containers ).text( "" );
this.addWrapper( errors ).hide();
},

valid: function() {
Expand Down Expand Up @@ -722,9 +727,10 @@ $.extend( $.validator, {
},

showLabel: function( element, message ) {
var place, group,
var place, group, errorID,
error = this.errorsFor( element ),
elementID = this.idOrName( element );
elementID = this.idOrName( element ),
describedBy = $( element ).attr( "aria-describedby" );
if ( error.length ) {
// refresh error/success class
error.removeClass( this.settings.validClass ).addClass( this.settings.errorClass );
Expand Down Expand Up @@ -759,7 +765,16 @@ $.extend( $.validator, {
} else if ( error.parents( "label[for='" + elementID + "']" ).length === 0 ) {
// If the element is not a child of an associated label, then it's necessary
// to explicitly apply aria-describedby
$( element ).attr( "aria-describedby", error.attr( "id" ) );

errorID = error.attr( "id" );
// Respect existing non-error aria-describedby
if ( !describedBy ) {
describedBy = errorID;
} else if ( !describedBy.match( new RegExp( "\b" + errorID + "\b" ) ) ) {
// Add to end of list if not already present
describedBy += " " + errorID;
}
$( element ).attr( "aria-describedby", describedBy );

// If this element is grouped, then assign to all elements in the same group
group = this.groups[ element.name ];
Expand All @@ -786,16 +801,15 @@ $.extend( $.validator, {

errorsFor: function( element ) {
var name = this.idOrName( element ),
describer = $( element ).attr( "aria-describedby" );
describer = $( element ).attr( "aria-describedby" ),
selector = "label[for='" + name + "'], label[for='" + name + "'] *";
// aria-describedby should directly reference the error element
if ( describer ) {
// aria-describedby should directly reference the error element
return $( "#" + describer, this.errorContext );
} else {
// If no describer is used then errors are either associated labels, or children of non-error labels
return this
.errors()
.filter( "label[for='" + name + "'], label[for='" + name + "'] *" );
selector = selector + ", #" + describer.replace( /\s+/g, ", #" );
}
return this
.errors()
.filter( selector );
},

idOrName: function( element ) {
Expand Down
22 changes: 21 additions & 1 deletion test/error-placement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,29 @@ test( "test existing non-label used as error element", function(assert) {
form.validate({ errorElement: "span" });

ok( !field.valid() );
assert.hasError( field );
assert.hasError( field, "required" );

field.val( "foo" );
ok( field.valid() );
assert.noErrorFor( field );
});

test( "test existing non-error aria-describedby", function( assert ) {
expect( 8 );
var form = $( "#testForm17" ),
field = $( "#testForm17text" );

equal( field.attr( "aria-describedby" ), "testForm17text-description" );
form.validate({ errorElement: "span" });

ok( !field.valid() );
equal( field.attr( "aria-describedby" ), "testForm17text-description testForm17text-error" );
assert.hasError( field, "required" );

field.val( "foo" );
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

ok( field.valid() );
assert.noErrorFor( field );

strictEqual( "This is where you enter your data", $("#testForm17text-description").text() );
strictEqual( "", $("#testForm17text-error").text(), "Error label is empty for valid field" );
});
6 changes: 6 additions & 0 deletions test/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@ <h2 id="qunit-userAgent"></h2>
<input name="testForm15text" id="testForm15text" data-rule-required="true" data-msg="required" aria-describedby="testForm15text-error" />
<span id="testForm15text-error" class="error"></span>
</form>
<form id="testForm17">
<!-- test existing non-error aria-describedby -->
<label for="testForm17text">My Label</label>
<input name="testForm17text" id="testForm17text" data-rule-required="true" data-msg="required" aria-describedby="testForm17text-description" />
<span id="testForm17text-description">This is where you enter your data</span>
</form>
<form id="dataMessages">
<input name="dataMessagesName" id="dataMessagesName" class="required" data-msg-required="You must enter a value here" />
</form>
Expand Down
2 changes: 1 addition & 1 deletion test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ QUnit.assert.hasError = function( element, text, message ) {
// Asserts that there is no visible error for the given element
QUnit.assert.noErrorFor = function( element, message ) {
var errors = $( element ).closest( "form" ).validate().errorsFor( element[ 0 ] ),
hidden = ( errors.length === 0 ) || errors.is( ":hidden" ) || ( errors.text() === "" );
hidden = ( errors.length === 0 ) || (errors.is( ":hidden" ) && ( errors.text() === "" ));
QUnit.push( hidden, hidden, true, message );
};

Expand Down