Do not reuse label in showLabel() method #991

Closed
pszalko opened this Issue Jan 21, 2014 · 5 comments

Comments

Projects
None yet
3 participants

pszalko commented Jan 21, 2014

I have use case where I add custom HTML markup into error label in errorPlacement callback method.
Custom HTML is a fontawesome icon:

HTML generated by default jQuery validation:

<label for=".." class="error">ERROR MESSAGE</label>

What I try to achieve is:

<label for=".." class="error"><i class="fa fa-times-circle"></i>ERROR MESSAGE</label>

However, current jQuery validation implementation reuses existing error labels and removes all content from inside the label tag, without calling errorPlacement callback again.

The issue is that my custom HTML is removed from label and there is no way to put it back when error message is replaced.

My suggested solution is to not allow to reuse existing error labels in showLabel() method. Instead it may try to remove existing label just before creating one.

Please see line 4. of the code below.
Here is fixed code for latest release:

showLabel: function( element, message ) {
            var label = this.errorsFor( element );
            if ( label.length ) {
                // do not reuse existing label - just remove one if it already exists (P.Szalko)
                label.remove();
            } 
            // create label
            label = $("<" + this.settings.errorElement + ">")
                .attr("for", this.idOrName(element))
                .addClass(this.settings.errorClass)
                .html(message || "");
            if ( this.settings.wrapper ) {
                // make sure the element is visible, even in IE
                // actually showing the wrapped element is handled elsewhere
                label = label.hide().show().wrap("<" + this.settings.wrapper + "/>").parent();
            }
            if ( !this.labelContainer.append(label).length ) {
                if ( this.settings.errorPlacement ) {
                    this.settings.errorPlacement(label, $(element) );
                } else {
                    label.insertAfter(element);
                }
            }
            if ( !message && this.settings.success ) {
                label.text("");
                if ( typeof this.settings.success === "string" ) {
                    label.addClass( this.settings.success );
                } else {
                    this.settings.success( label, element );
                }
            }
            this.toShow = this.toShow.add(label);
        },

thomasmi commented Aug 5, 2014

Previously this would show the custom message within the label. This is a big issue to me as I need to show the text within my labels. I would recommend commenting out the following line to allow using custom error messages within the markup.

// TODO: Why replace message on existing label? This allows custom errors via the html markup 
showLabel: function( element, message ) {
            var place, group, errorID,
                error = this.errorsFor( 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 );
                // TODO: comment out this line
                //error.html( message );
            } else {
                // create error element
                error = $( "<" + this.settings.errorElement + ">" )
                    .attr( "id", elementID + "-error" )
                    .addClass( this.settings.errorClass )
                    .html( message || "" );

                // Maintain reference to the element to be placed into the DOM
                place = error;
                if ( this.settings.wrapper ) {
                    // make sure the element is visible, even in IE
                    // actually showing the wrapped element is handled elsewhere
                    place = error.hide().show().wrap( "<" + this.settings.wrapper + "/>" ).parent();
                }
                if ( this.labelContainer.length ) {
                    this.labelContainer.append( place );
                } else if ( this.settings.errorPlacement ) {
                    this.settings.errorPlacement( place, $( element ) );
                } else {
                    place.insertAfter( element );
                }

                // Link error back to the element
                if ( error.is( "label" ) ) {
                    // If the error is a label, then associate using 'for'
                    error.attr( "for", elementID );
                } 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

                    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 ];
                    if ( group ) {
                        $.each( this.groups, function( name, testgroup ) {
                            if ( testgroup === group ) {
                                $( "[name='" + name + "']", this.currentForm )
                                    .attr( "aria-describedby", error.attr( "id" ) );
                            }
                        });
                    }
                }
            }
            if ( !message && this.settings.success ) {
                error.text( "" );
                if ( typeof this.settings.success === "string" ) {
                    error.addClass( this.settings.success );
                } else {
                    this.settings.success( error, element );
                }
            }
            this.toShow = this.toShow.add( error );
        }
Collaborator

jzaefferer commented Dec 9, 2014

For the given issue, I recommend using CSS to show the icon inside the label without an extra element, using padding and background-image. If that won't work, I'm sure there is some solution that doesn't require hacking the plugin source. If there's still interest, let me know.

@jzaefferer jzaefferer closed this Dec 9, 2014

It's not only embedded tags or images. Anything in between the beginning label tag and closing label tagged is cleared out. It actually overwrites everything. For example:
if you have a label with an error message like:
<label for=".." class="error">ERROR MESSAGE</label>
First time triggering the error, the error message is shown. If you correct the error, and then cause re-trigger the same error on the same field an the error label is empty.
<label for=".." class="error"></label>
Thus if you comment out the line I noted before it hides and shows the error label as expected.

Collaborator

jzaefferer commented Dec 11, 2014

Okay, that sounds more like a bug. Your suggested solution is going to cause other problems though.

@jzaefferer jzaefferer reopened this Dec 11, 2014

@jzaefferer jzaefferer added the Bug label Dec 11, 2014

Collaborator

jzaefferer commented Jun 28, 2015

I'm sorry for the lack of activity on this issue. Instead of leaving it open any longer, I decided to close old issues without trying to address them, to longer give the false impression that it will get addressed eventually, especially after several years with no activity. It doesn't mean I'm abandoning the project, just that I'm unable to work through 200+ open issues with the little time I can afford to spend on this project.

To the reporter (or anyone else interested in this issue): If you're still affected by the same issue, please consider opening a new issue, with a testpage that demonstrates the issue with a current version of the plugin. Even better, make an attempt to fix the issue yourself, and improve the project by sending a pull request. This may seem daunting at first, but you'll likely learn some useful skills that you can apply elsewhere as well. And you can help keep this project alive. We've documented how to do these things, too. A patch is worth a thousand issues!

@jzaefferer jzaefferer closed this Jun 28, 2015

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