[webkit-reviews] review denied: [Bug 84359] datalist: Form control in a <datalist> should be barred from constraint validation : [Attachment 138946] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 84359: datalist: Form control in a <datalist> should be barred from
constraint validation
https://bugs.webkit.org/show_bug.cgi?id=84359

Attachment 138946: Patch
https://bugs.webkit.org/attachment.cgi?id=138946&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138946&action=review


> Source/WebCore/ChangeLog:7
> +	   To do so we had to remove HTMLFormControlElement::m_willValidate
which cached that value.

"had to remove" is not true.  It's possible to implement this behavior without
removing it.
You wanted to remove it for simplicity, right?

> Source/WebCore/html/HTMLButtonElement.cpp:104
> +	   setNeedsStyleRecalc();
> +	   if (!willValidate())
> +	       hideVisibleValidationMessage();

This code is copied many times.  It's bad.

We should keep setNeedsWillValidateCheck() function.

> Source/WebCore/html/HTMLFormControlElement.cpp:142
> -    setNeedsWillValidateCheck();
> +    setNeedsStyleRecalc();
> +    if (!willValidate())
> +	   hideVisibleValidationMessage();

Do not copy the code.

setNeedsStleRecalc() is heavy operation.  We shouldn't call it whenever an
attribute is updated. We should call it only when willValidate state can be
changed. e.g. disabledAttr or readonlyAttr is updated.

> Source/WebCore/html/HTMLFormControlElement.cpp:358
> +    for (ContainerNode* ancestor = parentNode(); ancestor; ancestor =
ancestor->parentNode()) {
> +	   if (ancestor->hasTagName(datalistTag))
> +	       return false;

So, when the element leaves from a datalist ancestor or join under a datalist
ancestor, we need to update the style to reflect :valid and :invalid.

> Source/WebCore/html/HTMLInputElement.cpp:529
> +    setNeedsStyleRecalc();
> +    if (!willValidate())
> +	   hideVisibleValidationMessage();

Do not copy the code.

> Source/WebCore/html/HTMLMeterElement.h:63
> +    virtual bool willValidate() const OVERRIDE { return false; }

I think this will make a compilation error because LabelableElement doesn't
have willValidate().

> LayoutTests/ChangeLog:9
> +	   * fast/forms/datalist/datalist-fallback-content-validation.html:
Added.Tests that an require input inside a datalist does not block submit.
> +

You have to add -expected.txt.

> LayoutTests/fast/forms/datalist/datalist-fallback-content-validation.html:32
> +    } else {
> +	   // HTMLFormElement::submit() skips validation. Use the submit
button.
> +	   document.getElementById('s').click();
> +	   testFailed('The form was not submitted.');
> +    }
> +}

You don't need to submit a form.  Checking
document.getElementsByName('req')[0].willValidate is enough.

You need to check if a form control in a <datalist> doesn't match both of
:valid and :invalid.