Bug 43537 – Introduce isValidValue(const String&) of HTMLInputElement and HTMLTextAreaElement

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

[Kent Tamura]

Introduce isValidValue(const String&) of HTMLInputElement and HTMLTextAreaElement

[Kent Tamura]

Created attachment 63544 [details]
Patch

[Kent Tamura]

Created attachment 63548 [details]
Patch 2

[Kent Tamura]

Created attachment 63549 [details]
Patch 3

[James Hawkins]

"to propose form field values without breaking form validity."

Can you describe how we'll use this in Chrome?

[Kent Tamura]

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

[Darin Adler]

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.

[Kent Tamura]

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.