Bug 27959 - Support for validationMessage
: Support for validationMessage
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
: http://www.whatwg.org/specs/web-apps/...
:
: 27455
: 19264 28649 53034
  Show dependency treegraph
 
Reported: 2009-08-03 16:01 PST by
Modified: 2011-01-24 11:53 PST (History)


Attachments
Preview patch (8.03 KB, patch)
2009-08-24 16:35 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Demo patch 2 (8.23 KB, patch)
2009-08-25 10:13 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Complete patch, not yet reviewable (14.82 KB, patch)
2009-08-28 17:20 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v1 (24.58 KB, patch)
2009-09-02 14:36 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v1a (25.36 KB, patch)
2009-09-04 18:06 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (28.06 KB, patch)
2009-10-20 15:34 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2a (28.07 KB, patch)
2009-10-20 15:37 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2b (28.36 KB, patch)
2009-10-20 15:49 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2c (31.92 KB, patch)
2009-11-16 14:10 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2c, corrected (34.47 KB, patch)
2009-11-16 14:33 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-03 16:01:29 PST
The implementation for such attribute should follow the specification in section 4.10.15.3
------- Comment #1 From 2009-08-11 09:52:20 PST -------
To solve this issue I'd say that an enum structure with neutral messages would be sufficient. Opinions?
------- Comment #2 From 2009-08-11 10:10:07 PST -------
(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 From 2009-08-20 22:42:22 PST -------
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 From 2009-08-21 16:01:40 PST -------
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 From 2009-08-24 16:35:24 PST -------
Created an attachment (id=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 From 2009-08-24 16:36:41 PST -------
Ah, for the "VALIDATIONMSG_CUSTOM": it's (obviously) a mistake.:) It will return the customerror message.
------- Comment #7 From 2009-08-25 10:13:08 PST -------
Created an attachment (id=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 From 2009-08-28 17:20:01 PST -------
Created an attachment (id=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 From 2009-08-30 18:13:46 PST -------
We should avoid to use the words `element' and `attribute' in UI messages.  End-users don't understand such technical terms.
------- Comment #10 From 2009-08-30 22:24:12 PST -------
(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 From 2009-08-31 10:47:24 PST -------
(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 From 2009-08-31 12:49:14 PST -------
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 From 2009-09-02 14:13:51 PST -------
(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 From 2009-09-02 14:36:16 PST -------
Created an attachment (id=38942) [details]
Patch v1
------- Comment #15 From 2009-09-02 14:46:49 PST -------
(From update of attachment 38942 [details])
Lots of style violations in:
 36 String ValidityState::validationMessage()

Why do we have such ugly string names for these validation messages?
------- Comment #16 From 2009-09-02 15:10:47 PST -------
(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 From 2009-09-04 18:06:11 PST -------
Created an attachment (id=39107) [details]
Patch v1a
------- Comment #18 From 2009-10-13 13:19:03 PST -------
(From update of attachment 39107 [details])
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 From 2009-10-13 14:06:28 PST -------
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 From 2009-10-20 06:48:45 PST -------
(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 From 2009-10-20 15:34:29 PST -------
Created an attachment (id=41526) [details]
Patch v2
------- Comment #22 From 2009-10-20 15:37:24 PST -------
Created an attachment (id=41527) [details]
Patch v2a
------- Comment #23 From 2009-10-20 15:43:41 PST -------
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 From 2009-10-20 15:49:26 PST -------
Created an attachment (id=41530) [details]
Patch v2b
------- Comment #25 From 2009-11-10 09:34:59 PST -------
(From update of attachment 41530 [details])
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 From 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 From 2009-11-16 14:10:08 PST -------
Created an attachment (id=43322) [details]
Patch v2c
------- Comment #28 From 2009-11-16 14:33:21 PST -------
Created an attachment (id=43327) [details]
Patch v2c, corrected
------- Comment #29 From 2009-11-18 15:17:23 PST -------
(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.

> +    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 From 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] [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 From 2009-11-18 21:39:35 PST -------
Committed r51172: <http://trac.webkit.org/changeset/51172>
------- Comment #32 From 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 From 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 From 2009-11-19 16:21:09 PST -------
(In reply to comment #32)
> Darin: do Mac and Win ports share the same localization mechanisms?

Yes.