Bug 43537 - Introduce isValidValue(const String&) of HTMLInputElement and HTMLTextAreaElement
Summary: Introduce isValidValue(const String&) of HTMLInputElement and HTMLTextAreaEle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-04 21:31 PDT by Kent Tamura
Modified: 2010-08-29 22:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (24.70 KB, patch)
2010-08-04 21:34 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (101.50 KB, patch)
2010-08-04 21:42 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (24.79 KB, patch)
2010-08-04 21:47 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-08-04 21:31:46 PDT
Introduce isValidValue(const String&) of HTMLInputElement and HTMLTextAreaElement
Comment 1 Kent Tamura 2010-08-04 21:34:17 PDT
Created attachment 63544 [details]
Patch
Comment 2 Kent Tamura 2010-08-04 21:42:12 PDT
Created attachment 63548 [details]
Patch 2
Comment 3 Kent Tamura 2010-08-04 21:47:09 PDT
Created attachment 63549 [details]
Patch 3
Comment 4 James Hawkins 2010-08-05 14:06:40 PDT
"to propose form field values without breaking form validity."

Can you describe how we'll use this in Chrome?
Comment 5 Kent Tamura 2010-08-05 17:56:11 PDT
(In reply to comment #4)
> "to propose form field values without breaking form validity."
> 
> Can you describe how we'll use this in Chrome?

One of points we can use it at is:

Index: chrome/renderer/autofill_helper.cc
===================================================================
--- chrome/renderer/autofill_helper.cc  (revision 54514)
+++ chrome/renderer/autofill_helper.cc  (working copy)
@@ -75,14 +75,24 @@
   // Any popup currently showing is now obsolete.
   web_view->hidePopups();
 
-  // No suggestions: nothing to do.
-  if (values.empty())
-    return;
-
   std::vector<string16> v(values);
   std::vector<string16> l(labels);
   std::vector<string16> i(icons);
   std::vector<int> ids(unique_ids);
+  for (size_t i = 0 < v.size(); ++i) {
+    if (autofill_query_node_->isValidValue(v[i]))
+      continue;
+    v.erase[i];
+    l.erase[i];
+    i.erase[i];
+    ids.erase[i];
+    i--;
+  }
+
+  // No suggestions: nothing to do.
+  if (v.empty())
+    return;
+
   int separator_index = -1;
 
   // The form has been auto-filled, so give the user the chance to clear the

Note: I didn't confirm to compile or test this change.
Comment 6 Darin Adler 2010-08-29 12:27:20 PDT
Comment on attachment 63549 [details]
Patch 3

> +    // Common flag for HTMLInputElement::tooLong() and HTMLTextAreaElement::tooLong().
> +    enum NeedsToCheckDirtyFlag {
> +        CheckDirtyFlag,
> +        IgnoreDirtyFlag,
> +    };

I think this reads better on a single line than on four lines.

> +    Color color(value); // This accepts named colors such as "white".
> +    return color.isValid() && !color.hasAlpha();

This seems wrong. Why should "transparent" be an invalid color? Why is a color valid only if it has alpha of 255? There is no need to check isValid if you are also checking alpha, by the way, since invalid color is guaranteed to have an alpha of 0. I don't think there are enough test cases for this.

> +    int matchLength;
> +    int matchOffset = regExp.match(address, 0, &matchLength);
> +
> +    return !matchOffset && matchLength == addressLength;

Matching and then checking if the offset is 0 is an inefficient way to do an anchored regular expression match. We should probably add a complete string match operation to RegularExpression so you don't have to do things this roundabout way.

> +bool HTMLInputElement::isValidValue(const String& value) const
> +{
> +    if (inputType() == CHECKBOX || inputType() == FILE || inputType() == RADIO) {
> +        ASSERT_NOT_REACHED();
> +        return false;
> +    }

This probably needs a "why" comment.

> +    return !typeMismatch(value) && !stepMismatch(value) && !rangeUnderflow(value)
> +        && !rangeOverflow(value) && !tooLong(value, IgnoreDirtyFlag)
> +        && !patternMismatch(value) && !valueMissing(value);

I would format these with one check per line.

Since my comments are on code you are moving, not code you are adding, I am going to say review+ but I'd like to see a lot more test cases here and that Color thing sure does seem wrong.
Comment 7 Kent Tamura 2010-08-29 22:18:46 PDT
Thank you for reviewing.

(In reply to comment #6)
> (From update of attachment 63549 [details])
> > +    // Common flag for HTMLInputElement::tooLong() and HTMLTextAreaElement::tooLong().
> > +    enum NeedsToCheckDirtyFlag {
> > +        CheckDirtyFlag,
> > +        IgnoreDirtyFlag,
> > +    };
> 
> I think this reads better on a single line than on four lines.

Fixed.

> > +    Color color(value); // This accepts named colors such as "white".
> > +    return color.isValid() && !color.hasAlpha();
> 
> This seems wrong. Why should "transparent" be an invalid color? Why is a color valid only if it has alpha of 255? There is no need to check isValid if you are also checking alpha, by the way, since invalid color is guaranteed to have an alpha of 0. I don't think there are enough test cases for this.

Because HTML5 color has no alpha component.
Probably I will rewrite this color check code in the future.
 - We don't need to check named colors if we implement a color picker UI.
 - We should support localized names if we continue to use the current text field UI.

> > +    int matchLength;
> > +    int matchOffset = regExp.match(address, 0, &matchLength);
> > +
> > +    return !matchOffset && matchLength == addressLength;
> 
> Matching and then checking if the offset is 0 is an inefficient way to do an anchored regular expression match. We should probably add a complete string match operation to RegularExpression so you don't have to do things this roundabout way.

I agree with you.

> 
> > +bool HTMLInputElement::isValidValue(const String& value) const
> > +{
> > +    if (inputType() == CHECKBOX || inputType() == FILE || inputType() == RADIO) {
> > +        ASSERT_NOT_REACHED();
> > +        return false;
> > +    }
> 
> This probably needs a "why" comment.

Added a comment.

> > +    return !typeMismatch(value) && !stepMismatch(value) && !rangeUnderflow(value)
> > +        && !rangeOverflow(value) && !tooLong(value, IgnoreDirtyFlag)
> > +        && !patternMismatch(value) && !valueMissing(value);
> 
> I would format these with one check per line.

Fixed.

I landed the patch with the above modifications as r66357.