Bug 27455 - Support custom validity error message
Summary: Support custom validity error message
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Michelangelo De Simone
URL:
Keywords:
Depends on:
Blocks: HTML5Forms 27959
  Show dependency treegraph
 
Reported: 2009-07-20 12:08 PDT by Peter Kasting
Modified: 2009-08-06 16:45 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (no -expected files attached, no review asked, yet) (7.02 KB, patch)
2009-08-03 17:04 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Proposed patch, complete. (10.58 KB, patch)
2009-08-04 18:40 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Proposed patch, rev. 2 (14.59 KB, patch)
2009-08-05 14:30 PDT, Michelangelo De Simone
darin: review-
Details | Formatted Diff | Diff
Proposed patch, rev. 3 (14.24 KB, patch)
2009-08-05 17:14 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Proposed patch, rev. 3a (14.27 KB, patch)
2009-08-05 17:21 PDT, Michelangelo De Simone
darin: review-
Details | Formatted Diff | Diff
Proposed patch, 3b (13.89 KB, patch)
2009-08-05 18:30 PDT, Michelangelo De Simone
abarth: review-
abarth: commit-queue-
Details | Formatted Diff | Diff
Proposed patch, rev. 3c (15.30 KB, patch)
2009-08-06 12:09 PDT, Michelangelo De Simone
abarth: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-07-20 12:08:11 PDT
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.
Comment 1 Michelangelo De Simone 2009-08-03 15:51:50 PDT
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.
Comment 2 Peter Kasting 2009-08-03 15:55:52 PDT
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.
Comment 3 Michelangelo De Simone 2009-08-03 16:03:31 PDT
(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.
Comment 4 Michelangelo De Simone 2009-08-03 17:04:08 PDT
Created attachment 34025 [details]
Proposed patch (no -expected files attached, no review asked, yet)
Comment 5 Michelangelo De Simone 2009-08-04 18:40:26 PDT
Created attachment 34111 [details]
Proposed patch, complete.
Comment 6 Darin Adler 2009-08-05 09:19:55 PDT
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.
Comment 7 Michelangelo De Simone 2009-08-05 14:30:36 PDT
Created attachment 34177 [details]
Proposed patch, rev. 2
Comment 8 Darin Adler 2009-08-05 16:32:10 PDT
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.
Comment 9 Michelangelo De Simone 2009-08-05 17:14:48 PDT
Created attachment 34194 [details]
Proposed patch, rev. 3
Comment 10 Michelangelo De Simone 2009-08-05 17:17:52 PDT
I forgot to comply with Darin's comment on ValidityState::setCustomValidity->ValidityState::setCustomErrorMessage.

A couple of minutes and it'll be updated.
Comment 11 Michelangelo De Simone 2009-08-05 17:21:42 PDT
Created attachment 34195 [details]
Proposed patch, rev. 3a

Here we are.
Comment 12 Darin Adler 2009-08-05 18:09:53 PDT
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.
Comment 13 Michelangelo De Simone 2009-08-05 18:30:52 PDT
Created attachment 34196 [details]
Proposed patch, 3b
Comment 14 Adam Barth 2009-08-05 23:23:55 PDT
Comment on attachment 34196 [details]
Proposed patch, 3b

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

fast/dom/domListEnumeration.html -> failed
Comment 15 Peter Kasting 2009-08-06 11:57:25 PDT
(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.
Comment 16 Michelangelo De Simone 2009-08-06 12:09:21 PDT
Created attachment 34217 [details]
Proposed patch, rev. 3c
Comment 17 Adam Barth 2009-08-06 13:35:19 PDT
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
Comment 18 Adam Barth 2009-08-06 16:45:44 PDT
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
Comment 19 Adam Barth 2009-08-06 16:45:48 PDT
All reviewed patches have been landed.  Closing bug.