Support custom validity error message

• Component: Forms
• Status: RESOLVED FIXED

[Peter Kasting]

See spec sections 4.10.15.1, 4.10.15.3.

This covers adding the notion of a custom validity error message (string) to input elements, supporting the setCustomValidity() method to set it, and hooking it to the customError() method of the validityState object.

[Michelangelo De Simone]

I believe this bug includes the support for the validationMessage attribute as well. If it does I need to know exactly the design you intend to adopt for the "localized validation messages".

4.10.15.3
"The validationMessage attribute must return the empty string if the element is not a candidate for constraint validation or if it is one but it satisfies its constraints; otherwise, it must return a suitably localized message that the user agent would show the user if this were the only form with a validity constraint problem. If the element is suffering from a custom error, then the custom validity error message should be present in the return value."

I don't know if having such localizable/localized strings hardcoded directly into WebCore is a good thing, anyway we could use a neutral notation for {typeMismatch, tooLong, ...} \ {customError} validation messages: enumerate them inside a ENUM and then let upper layers deal with the user presentation in his/her natural language.

On the other hand, If "validationMessage" is not supposed to be part of this bug (I doubt it): setCustomValidity and customError are ready, along with a simple tc.

[Peter Kasting]

Let's keep this about setCustomValidity() and customError().  Can you file a separate bug about validationMessage?  The right implementation for that will probably be to get an appropriate string from the embedder, so we'll have to plumb something.  I don't know if there is already a pathway for this kind of thing.  Maybe for context menu strings.

[Michelangelo De Simone]

(In reply to comment #2)

> Let's keep this about setCustomValidity() and customError().  Can you file a
> separate bug about validationMessage?  The right implementation for that will

Done; a tiny patch for this bug is on its way, matter of minutes.

[Michelangelo De Simone]

Created attachment 34025 [details]
Proposed patch (no -expected files attached, no review asked, yet)

[Michelangelo De Simone]

Created attachment 34111 [details]
Proposed patch, complete.

[Darin Adler]

Comment on attachment 34111 [details]
Proposed patch, complete.

> +            v[i].setCustomValidity("");

I'd like to see a test that calls setCustomValidity with too few arguments, too many arguments, and an argument of "null" and and argument of "undefined".

>      RefPtr<ValidityState> m_validityState;
> +    String m_customError;

Can we put m_customError inside ValidityState instead of putting it directly in the form element? I don't want to make form elements on pages that aren't using validation larger, and it seems logical to group this with the rest of the validity system. It would get rid of the need for ValidityState::customError to call back to the control as well.

[Michelangelo De Simone]

Created attachment 34177 [details]
Proposed patch, rev. 2

[Darin Adler]

Comment on attachment 34177 [details]
Proposed patch, rev. 2

>  #include "HTMLElement.h"
> +//#include "ValidityState.h"

I think you forgot to remove this.

>  ValidityState::ValidityState(HTMLFormControlElement* parent)
>      : m_control(parent)
> +    , m_customError("")
>  {
>      ASSERT(parent);
>  }
>  
> +void ValidityState::setCustomValidity(const String& error)
> +{
> +    if (error.isNull()) {
> +        m_customError = "";
> +        return;
> +    }
> +
> +    m_customError = error;
> +}

Why is it important to store an empty string instead of a null string? It would be nice to just let m_customError get initialized to null and not special case null in setCustomValidity. Does it cause some sort of problem?

> +        void setCustomValidity(const String&);

> -        bool customError() { return false; }
> +        bool customError() { return !m_customError.isEmpty(); }

> +        String m_customError;

I assume that customError and setCustomValidity are both names from the HTML 5 specification, so I won't complain to you about them too much, since I guess it's OK to have the ValidityState function names match the DOM function names.

But I must say both names are confusing. Set the custom validity? No, set a custom validity error message. So it should be setCustomErrorMessage on ValidityState. "Custom error"? No, "has custom error message", so the name should be hasCustomErrorMessage, not customError.

Since m_customError is our own name, it can be as clear as we like. I suggest m_customErrorMessage.

I'm going to say review-, just so you can fix these minor issues.

[Michelangelo De Simone]

Created attachment 34194 [details]
Proposed patch, rev. 3

[Michelangelo De Simone]

I forgot to comply with Darin's comment on ValidityState::setCustomValidity->ValidityState::setCustomErrorMessage.

A couple of minutes and it'll be updated.

[Michelangelo De Simone]

Created attachment 34195 [details]
Proposed patch, rev. 3a

Here we are.

[Darin Adler]

Comment on attachment 34195 [details]
Proposed patch, rev. 3a

>  ValidityState::ValidityState(HTMLFormControlElement* parent)
>      : m_control(parent)
> +    , m_customErrorMessage()
>  {
>      ASSERT(parent);
>  }

No change needed here. This will happen without you explicitly listing the name of the data member since it's not a plain type like int, so you should leave this out.

> +        void setCustomErrorMessage(const String& e) { m_customErrorMessage = e; }

We normally use short words rather than single letters for this sort of thing. It should be "message" instead of "e".

> -        bool customError() { return false; }
> +        bool customError() { return !m_customErrorMessage.isEmpty() && !m_customErrorMessage.isNull(); }

Since all null messages are empty there is no need for the separate isNull check here.

This is looking good, but please fix these.

[Michelangelo De Simone]

Created attachment 34196 [details]
Proposed patch, 3b

[Adam Barth]

Comment on attachment 34196 [details]
Proposed patch, 3b

The commit-queue claims this patch fails the following test:

fast/dom/domListEnumeration.html -> failed

[Peter Kasting]

(In reply to comment #14)
> (From update of attachment 34196 [details])
> The commit-queue claims this patch fails the following test:
> 
> fast/dom/domListEnumeration.html -> failed

I bet that's a real failure, and the expected results of this test have to be updated to add one to some number(s).

Michelangelo, you've done this to this test before, so you should have no problem updating and testing this locally.

[Michelangelo De Simone]

Created attachment 34217 [details]
Proposed patch, rev. 3c

[Adam Barth]

Comment on attachment 34217 [details]
Proposed patch, rev. 3c

micdesim, are you a commiter?  I don't see you listed at https://trac.webkit.org/wiki/WebKit%20Team

[Adam Barth]

Comment on attachment 34217 [details]
Proposed patch, rev. 3c

Clearing review flag on attachment: 34217

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/dom/domListEnumeration-expected.txt
	M	LayoutTests/fast/dom/resources/domListEnumeration.js
	A	LayoutTests/fast/forms/ValidityState-customError-001-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-001.html
	A	LayoutTests/fast/forms/ValidityState-customError-002-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-002.html
	A	LayoutTests/fast/forms/ValidityState-customError-003-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-003.html
	A	LayoutTests/fast/forms/ValidityState-customError-004-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-004.html
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLButtonElement.idl
	M	WebCore/html/HTMLFieldSetElement.idl
	M	WebCore/html/HTMLFormControlElement.cpp
	M	WebCore/html/HTMLFormControlElement.h
	M	WebCore/html/HTMLInputElement.idl
	M	WebCore/html/HTMLSelectElement.idl
	M	WebCore/html/HTMLTextAreaElement.idl
	M	WebCore/html/ValidityState.h
Committed r46869
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLInputElement.idl
	M	WebCore/html/HTMLFieldSetElement.idl
	M	WebCore/html/HTMLFormControlElement.h
	M	WebCore/html/HTMLFormControlElement.cpp
	M	WebCore/html/ValidityState.h
	M	WebCore/html/HTMLSelectElement.idl
	M	WebCore/html/HTMLButtonElement.idl
	M	WebCore/html/HTMLTextAreaElement.idl
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/dom/resources/domListEnumeration.js
	M	LayoutTests/fast/dom/domListEnumeration-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-001.html
	A	LayoutTests/fast/forms/ValidityState-customError-003-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-002.html
	A	LayoutTests/fast/forms/ValidityState-customError-001-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-003.html
	A	LayoutTests/fast/forms/ValidityState-customError-004-expected.txt
	A	LayoutTests/fast/forms/ValidityState-customError-004.html
	A	LayoutTests/fast/forms/ValidityState-customError-002-expected.txt
r46869 = bfb58fa6d333ec22cad161a29606909f77d8ec71 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46869

[Adam Barth]

All reviewed patches have been landed.  Closing bug.