Bug 27959

Summary: Support for validationMessage
Product: WebKit Reporter: Michelangelo De Simone <michelangelo>
Component: FormsAssignee: Michelangelo De Simone <michelangelo>
Status: RESOLVED FIXED    
Severity: Enhancement CC: adele, darin, eric, gustavo, kevino, pkasting, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#dom-cva-validationmessage
Bug Depends on: 27455    
Bug Blocks: 19264, 28649, 53034    
Attachments:
Description Flags
Preview patch
none
Demo patch 2
none
Complete patch, not yet reviewable
none
Patch v1
none
Patch v1a
none
Patch v2
none
Patch v2a
none
Patch v2b
none
Patch v2c
none
Patch v2c, corrected none

Description Michelangelo De Simone 2009-08-03 16:01:29 PDT
The implementation for such attribute should follow the specification in section 4.10.15.3
Comment 1 Michelangelo De Simone 2009-08-11 09:52:20 PDT
To solve this issue I'd say that an enum structure with neutral messages would be sufficient. Opinions?
Comment 2 Peter Kasting 2009-08-11 10:10:07 PDT
(In reply to comment #1)
> To solve this issue I'd say that an enum structure with neutral messages would
> be sufficient. Opinions?

What does that mean?  Surely you're not intending to code the message strings into WebCore itself.

The solution to this needs to call out to the embedder, supplying it the type of validation failure, and the custom error string if any; then the embedder can return an appropriate localized string.

I don't know if there is an existing set of pathways to request strings from the embedder.  My first guess at a place to look would be the context menu code.
Comment 3 Peter Kasting 2009-08-20 22:42:22 PDT
The following comment was lost due to database corruption:

--- Comment #3 from Michelangelo De Simone <micdesim@gmail.com>  2009-08-20 15:38:37 PDT ---
(In reply to comment #2)

> What does that mean?  Surely you're not intending to code the message strings
> into WebCore itself.

Not at all. I intended some neutral representation of standard validation
messages that the embedder can relate internally with desired localized
strings. Embedder can query WebCore and get the appropriate validationMessage,
if any.

Perhaps something like an "enum ValidationMessages { PATTERN_MISMATCHED,
EMPTY_REQUIRED, ... CUSTOM}" could do the trick?

> The solution to this needs to call out to the embedder, supplying it the type
> of validation failure, and the custom error string if any; then the embedder
> can return an appropriate localized string.

That's my point.

Does something like that sound feasible to you?

****

I replied with a comment #4, something to the effect of "OK, that's exactly what I was thinking."
Comment 4 Michelangelo De Simone 2009-08-21 16:01:40 PDT
How do we want this? In #27455 we put customValidationMessage into ValidityState.

IMO, customValidity message stuff and validationMessage would stay better inside HTMLFormControlElement (could that create some odd effect on the desired memory footprint?); I'd like to hear Darin's opinion about this.

This is the proposal:

1) Add "enum ValidationMessage { NONE, CUSTOM, STD_MSG1, STD_MSG2, STD_MSG3, ...}" into HTMLFormControlElement,
2) Move m_customErrorMessage from ValidityState to HTMLFormControlElement
Comment 5 Michelangelo De Simone 2009-08-24 16:35:24 PDT
Created attachment 38509 [details]
Preview patch

This is just a "demo" patch to gather some feedback:

ValidationMessage enum has been placed into LocalizedStrings to avoid code-duplication. I need to better understand how localized strings are used on the Mac can I use  e Chrome port before continuing.

For the Mac: is acceptable to use NSLocalizedStrings into LocalizedStringsMac? If no, what is the "trick"?
For Chrome: where should I look at!?:D
Comment 6 Michelangelo De Simone 2009-08-24 16:36:41 PDT
Ah, for the "VALIDATIONMSG_CUSTOM": it's (obviously) a mistake.:) It will return the customerror message.
Comment 7 Michelangelo De Simone 2009-08-25 10:13:08 PDT
Created attachment 38554 [details]
Demo patch 2

Ok, I've figured out everything that I needed: this demo patch seems ok for the Mac.
If this is feasible I get things completed with other ports.
Comment 8 Michelangelo De Simone 2009-08-28 17:20:01 PDT
Created attachment 38764 [details]
Complete patch, not yet reviewable

This is the patch I'm gonna submit (once I find a way to diff WebKit/English.lproj/Localizable.strings with git); it might be of some help to tkent.
These lines have been added to WebKit/English.lproj/Localizable.strings:

/* Validity states */
"element has no value but is a required field" = "element has no value but is a required field";
"element's value is not in the correct syntax" = "element's value is not in the correct syntax";
"element's value doesn't match the provided pattern" = "element's value doesn't match the provided pattern";
"element's value is longer than the provided maximum length" = "element's value is longer than the provided maximum length";
"element's value is lower than the provided minimum" = "element's value is lower than the provided minimum";
"element's value is higher than the provided maximum" = "element's value is higher than the provided maximum";
"element's value doesn't fit the rules given by the step attribute" = "element's value doesn't fit the rules given by the step attribute";
Comment 9 Kent Tamura 2009-08-30 18:13:46 PDT
We should avoid to use the words `element' and `attribute' in UI messages.  End-users don't understand such technical terms.
Comment 10 Peter Kasting 2009-08-30 22:24:12 PDT
(In reply to comment #9)
> We should avoid to use the words `element' and `attribute' in UI messages. 
> End-users don't understand such technical terms.

Do the strings in this file actually get used directly, or will the Safari team replace them?  No reason to quibble if they're just advisory to a L10N team...
Comment 11 Michelangelo De Simone 2009-08-31 10:47:24 PDT
(In reply to comment #10)

> Do the strings in this file actually get used directly, or will the Safari team
> replace them?  No reason to quibble if they're just advisory to a L10N team...

Those strings are used as base for future localization but, actually, they're used as default choice on the Mac port.

Being a non-native english speaker I might not be the best candidate to choose those messages, so I'd be happy to hear any suggestion about this matter.
Comment 12 Darin Adler 2009-08-31 12:49:14 PDT
It’s not correct to edit the localizable strings file manually.

The file is regenerated by the update-webkit-localizable-strings script.

Instead you need to use the UI_STRING macro in WebKit code to call out the localizable strings. Search the WebKit source code for UI_STRING to see how it’s done.
Comment 13 Michelangelo De Simone 2009-09-02 14:13:51 PDT
(In reply to comment #12)

> Instead you need to use the UI_STRING macro in WebKit code to call out the

The "unreviewable patch" already makes use of UI_STRING. Am I supposed to land base localizable strings for the Mac port or should I ask to someone specific?
Comment 14 Michelangelo De Simone 2009-09-02 14:36:16 PDT
Created attachment 38942 [details]
Patch v1
Comment 15 Eric Seidel (no email) 2009-09-02 14:46:49 PDT
Comment on attachment 38942 [details]
Patch v1

Lots of style violations in:
 36 String ValidityState::validationMessage()

Why do we have such ugly string names for these validation messages?
Comment 16 Michelangelo De Simone 2009-09-02 15:10:47 PDT
(In reply to comment #15)

> Lots of style violations in:
> Why do we have such ugly string names for these validation messages?

You mean the "VALIDATIONMSG_VALUEMISSING"-kinda strings?
Comment 17 Michelangelo De Simone 2009-09-04 18:06:11 PDT
Created attachment 39107 [details]
Patch v1a
Comment 18 Adam Barth 2009-10-13 13:19:03 PDT
Comment on attachment 39107 [details]
Patch v1a

This patch seems blocked on not understanding how string localization is supposed to work.  I can't really help with that...  I recommend bugging Darin or Maciej on IRC.

Also, there is a minor style issue:

+    if (customError()) {
+        return m_customErrorMessage;
+    } else if (valueMissing()) {

We're not supposed to use { } for single-line conditionals (applies to this whole code block).
Comment 19 Darin Adler 2009-10-13 14:06:28 PDT
Here is how localized strings in WebCore work:

    1) We try to minimize the number of localized strings that are needed in WebCore. There are very few strings which appear to end users, so this is usually possible. It's unfortunate that the HTML 5 design here requires localized strings without giving the localizer a clear idea where and in what format those strings will be displayed. This is something we normally try to avoid.

    2) Because there are so few localized strings needed in WebCore, we create a separate function for each string in the LocalizedStrings.h header file in WebCore/platform.

    3) Each platform then needs an implementation of each of these functions. For the Mac, the class WebCoreViewFactory in WebCore and WebViewFactory in WebKit has to be modified. And there are similar changes for other platforms.
Comment 20 Michelangelo De Simone 2009-10-20 06:48:45 PDT
(In reply to comment #19)

>     2) Because there are so few localized strings needed in WebCore, we create
> a separate function for each string in the LocalizedStrings.h header file in
> WebCore/platform.

A new patch with separate functions is on its way. (eg.: validationMessageValueMissingText()).
Comment 21 Michelangelo De Simone 2009-10-20 15:34:29 PDT
Created attachment 41526 [details]
Patch v2
Comment 22 Michelangelo De Simone 2009-10-20 15:37:24 PDT
Created attachment 41527 [details]
Patch v2a
Comment 23 Michelangelo De Simone 2009-10-20 15:43:41 PDT
A quick rationale:

1) this patch addresses LOCALIZED validationMessage only on the Mac port; on
other ports validationMessage() returns the custom validation message (if
present) or the empty string. Reason: I can't test it on the other ports and
don't want to break anything.:-)

2) related test "validationMessage.html" is currently disabled for the same
reason.

NOTE TO THE REVIEWER: this patch blocks 28649 which, I believe, is quite
urgent: localization issues can be addressed separately.

Comments and feedbacks greatly appreciated.
Comment 24 Michelangelo De Simone 2009-10-20 15:49:26 PDT
Created attachment 41530 [details]
Patch v2b
Comment 25 Eric Seidel (no email) 2009-11-10 09:34:59 PST
Comment on attachment 41530 [details]
Patch v2b

OK.  So this test shoudl just be Skipped on other platforms instead of disabled for all.

I agree, I think it's lame that the HTML5 spec exposes localized strings in this way.  It will be impossible for localizers to know how these strings are used, and thus impossible for them to end up with the right localizations.

I don't think this should be PLATFORM(MAC) only.  Instead "notImplemented()" style functions should be added to the other platforms. 

Your localizable.strings patch doesn't look right as ir removes accessiblity strings.

Otherwise this could be OK.
Comment 26 Michelangelo De Simone 2009-11-10 12:27:20 PST
(In reply to comment #25)

> OK.  So this test shoudl just be Skipped on other platforms instead of disabled
> for all.

Ok.

> I agree, I think it's lame that the HTML5 spec exposes localized strings in
> this way.  It will be impossible for localizers to know how these strings are
> used, and thus impossible for them to end up with the right localizations.

Agreed.

> I don't think this should be PLATFORM(MAC) only.  Instead "notImplemented()"
> style functions should be added to the other platforms. 

Ok.

> Your localizable.strings patch doesn't look right as ir removes accessiblity
> strings.

Uhmmm, that's weird... I'll look into it.

> Otherwise this could be OK.

Thanks, a new patch is underway.
Comment 27 Michelangelo De Simone 2009-11-16 14:10:08 PST
Created attachment 43322 [details]
Patch v2c
Comment 28 Michelangelo De Simone 2009-11-16 14:33:21 PST
Created attachment 43327 [details]
Patch v2c, corrected
Comment 29 Darin Adler 2009-11-18 15:17:23 PST
Comment on attachment 43327 [details]
Patch v2c, corrected

> +    if (customError())
> +        return m_customErrorMessage;
> +    else if (valueMissing())
> +        return validationMessageValueMissingText();
> +    else if (typeMismatch())
> +        return validationMessageTypeMismatchText();
> +    else if (patternMismatch())
> +        return validationMessagePatternMismatchText();
> +    else if (tooLong())
> +        return validationMessageTooLongText();
> +    else if (rangeUnderflow())
> +        return validationMessageRangeUnderflowText();
> +    else if (rangeOverflow())
> +        return validationMessageRangeOverflowText();
> +    else if (stepMismatch())
> +        return validationMessageStepMismatchText();

Normally we don't do else after return.

> +    return emptyAtom;

emptyAtom is an AtomicString and we just need a String here. On the other hand, emptyAtom might be a relatively efficient way to return the empty string, so I'm not sure this is a bad thing!

It seems to me that returning a non-localized hard-wired string before calling notImplemented() would probably be better for all those ports where this is not done yet.

This patch sure does seem to be an eloquent argument for not having all these localized strings in the HTML5 form element design! I don't see a lot of value of returning these strings that might not even be in the same language as the website. I wish we could get that changed.

r=me
Comment 30 Kent Tamura 2009-11-18 20:47:21 PST
I'll commit the patch manually with the following changes.

(In reply to comment #29)
> (From update of attachment 43327 [details])
> > +    if (customError())
> > +        return m_customErrorMessage;
> > +    else if (valueMissing())
> > +        return validationMessageValueMissingText();
> > +    else if (typeMismatch())
> > +        return validationMessageTypeMismatchText();
> > +    else if (patternMismatch())
> > +        return validationMessagePatternMismatchText();
> > +    else if (tooLong())
> > +        return validationMessageTooLongText();
> > +    else if (rangeUnderflow())
> > +        return validationMessageRangeUnderflowText();
> > +    else if (rangeOverflow())
> > +        return validationMessageRangeOverflowText();
> > +    else if (stepMismatch())
> > +        return validationMessageStepMismatchText();
> 
> Normally we don't do else after return.

Will fix them.

> > +    return emptyAtom;
> 
> emptyAtom is an AtomicString and we just need a String here. On the other hand,
> emptyAtom might be a relatively efficient way to return the empty string, so
> I'm not sure this is a bad thing!

Returning a different type object seems curious to me.  I'll change emptyAtom to String().
Comment 31 Kent Tamura 2009-11-18 21:39:35 PST
Committed r51172: <http://trac.webkit.org/changeset/51172>
Comment 32 Michelangelo De Simone 2009-11-19 16:11:16 PST
This patch landed without {gtk, qt, win}/Skipped (3 files were missing).

Kevin Ollivier fixed the Wx port in r51190.
Gustavo Noronha Silva completed the GTK+ implementation in r51200.
I've landed missing files (wx/Skipped and win/Skipped) in r51210.

Darin: do Mac and Win ports share the same localization mechanisms? If yes I'll exclude validationMessage.html from skipped tests on Win port, or perhaps someone else (Peter?) could do it: I can't test it directly on Windows.

> This patch sure does seem to be an eloquent argument for not having all these
> localized strings in the HTML5 form element design! I don't see a lot of value
> of returning these strings that might not even be in the same language as the
> website. I wish we could get that changed.

I agree. I'm eager to understand reasons of such choice.
Comment 33 Michelangelo De Simone 2009-11-19 16:14:32 PST
CC'ing Gustavo and Kevin, they should be kept apprised (I think).

(In reply to comment #32)
> I've landed missing files (wx/Skipped and win/Skipped) in r51210.
Obviously I meant qt/Skipped and Win/Skipped...:)
Comment 34 Darin Adler 2009-11-19 16:21:09 PST
(In reply to comment #32)
> Darin: do Mac and Win ports share the same localization mechanisms?

Yes.