Fix removing invalid values from color input suggestions

[Keishi Hattori]

Removing invalid values from color input suggestions is broken.

[Keishi Hattori]

Created attachment 154931 [details]
Patch

[Kent Tamura]

Comment on attachment 154931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154931&action=review

> Source/WebCore/html/ColorInputType.cpp:229
> -            if (!element()->isValidValue(option->value()))
> +            if (!isValidColorString(option->value()))

I think we should fix HTMLInputElement::isValidValue() in this case.  isValidValue() exists for this purpose.

[Keishi Hattori]

I shouldn't be returning true for typeMismatch etc., so do I need to add a method to InputType and  check that in HTMLInputElement::isValidValue()?

[Keishi Hattori]

Created attachment 154943 [details]
Patch

[Build Bot]

Comment on attachment 154943 [details]
Patch

Attachment 154943 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13367530

[WebKit Review Bot]

Comment on attachment 154943 [details]
Patch

Attachment 154943 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13372493

[Early Warning System Bot]

Comment on attachment 154943 [details]
Patch

Attachment 154943 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13371564

[Early Warning System Bot]

Comment on attachment 154943 [details]
Patch

Attachment 154943 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13371566

[Gyuyoung Kim]

Comment on attachment 154943 [details]
Patch

Attachment 154943 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13376570

[Darin Adler]

Comment on attachment 154943 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154943&action=review

> Source/WebCore/html/InputType.h:310
> +    virtual bool isUsefulValue(const String& value) const;

What does “useful value” mean in this context? How is “useful” different from “valid”? I find this terminology unclear.

[Eric Seidel (no email)]

Comment on attachment 154943 [details]
Patch

The EWS bots are unhappy.

[Kent Tamura]

Comment on attachment 154943 [details]
Patch

What you need to do is just to implement ColorInputType::typeMismatchFor(const String&).

As you know, <input type=color> never cause typeMismatch validation failure because of the aggressive value sanitization.  However implementing typeMismatch() is helpful for HTMLInputElement::isValidValue() because isValidValue() doesn't sanitize the specified string.  NumberInputType does the same thing.

If you feel implementing typeMismatchFor() is hacky, you had better update HTMLInputElement::isValidValue() so that it handles value sanitization correctly.

[Keishi Hattori]

Created attachment 155204 [details]
Patch

[Keishi Hattori]

(In reply to comment #12)
> (From update of attachment 154943 [details])
> What you need to do is just to implement ColorInputType::typeMismatchFor(const String&).
> 
> As you know, <input type=color> never cause typeMismatch validation failure because of the aggressive value sanitization.  However implementing typeMismatch() is helpful for HTMLInputElement::isValidValue() because isValidValue() doesn't sanitize the specified string.  NumberInputType does the same thing.
> 
> If you feel implementing typeMismatchFor() is hacky, you had better update HTMLInputElement::isValidValue() so that it handles value sanitization correctly.

Ok, I implemented typeMismatchFor()

[Kent Tamura]

Comment on attachment 155204 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 155204 [details]
Patch

Clearing flags on attachment: 155204

Committed r123997: <http://trac.webkit.org/changeset/123997>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.