Bug 27959 – Support for validationMessage

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Michelangelo De Simone]

The implementation for such attribute should follow the specification in section 4.10.15.3

[Michelangelo De Simone]

To solve this issue I'd say that an enum structure with neutral messages would be sufficient. Opinions?

[Peter Kasting]

(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.

[Peter Kasting]

The following comment was lost due to database corruption:

--- Comment #3 from Michelangelo De Simone <[e-mail]>  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."

[Michelangelo De Simone]

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

[Michelangelo De Simone]

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

[Michelangelo De Simone]

Ah, for the "VALIDATIONMSG_CUSTOM": it's (obviously) a mistake.:) It will return the customerror message.

[Michelangelo De Simone]

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.

[Michelangelo De Simone]

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";

[Kent Tamura]

We should avoid to use the words `element' and `attribute' in UI messages.  End-users don't understand such technical terms.

[Peter Kasting]

(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...

[Michelangelo De Simone]

(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.

[Darin Adler]

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.

[Michelangelo De Simone]

(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?

[Michelangelo De Simone]

Created attachment 38942 [details]
Patch v1

[Eric Seidel (no email)]

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?

[Michelangelo De Simone]

(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?

[Michelangelo De Simone]

Created attachment 39107 [details]
Patch v1a

[Adam Barth]

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).

[Darin Adler]

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.

[Michelangelo De Simone]

(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()).

[Michelangelo De Simone]

Created attachment 41526 [details]
Patch v2

[Michelangelo De Simone]

Created attachment 41527 [details]
Patch v2a

[Michelangelo De Simone]

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.

[Michelangelo De Simone]

Created attachment 41530 [details]
Patch v2b

[Eric Seidel (no email)]

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.

[Michelangelo De Simone]

(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.

[Michelangelo De Simone]

Created attachment 43322 [details]
Patch v2c

[Michelangelo De Simone]

Created attachment 43327 [details]
Patch v2c, corrected

[Darin Adler]

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

[Kent Tamura]

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().

[Kent Tamura]

Committed r51172: <http://trac.webkit.org/changeset/51172>

[Michelangelo De Simone]

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.

[Michelangelo De Simone]

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...:)

[Darin Adler]

(In reply to comment #32)
> Darin: do Mac and Win ports share the same localization mechanisms?

Yes.