[Webkit-unassigned] [Bug 50380] Implement "required" attribute for select tags

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50380


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #75494|review?                     |review-
               Flag|                            |




--- Comment #14 from Kent Tamura <tkent at chromium.org>  2010-12-05 18:16:24 PST ---
(From update of attachment 75494)
View in context: https://bugs.webkit.org/attachment.cgi?id=75494&action=review

> LayoutTests/fast/forms/ValidityState-valueMissing-001-expected.txt:2
> -There are two form control elements below, both required and blank: validity.valueMissing should be true in both cases.
> +This test checks validity.valueMissing with blank values, blank options selected, or nothing selected.
>  

The purpose of ValidityState-valueMissing-001.html was test cases for validity.valueMissing==true.
If you follow it, you should remove cases with valueMissing==false.

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:71
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

We don't need to call waitUntilDone() and notifyDone().  The document text is dumped automatically when the document loading is completed.

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:91
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:53
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

ditto

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:69
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/ValidityState-valueMissing-003-expected.txt:15
> +FAIL valueMissingFor("select-none-selected-multiple") should be false. Was true.
> +PASS valueMissingFor("select-fake-placeholder-selected-multiple") is false
> +PASS valueMissingFor("select-without-fake-placeholder-multiple") is false
> +FAIL valueMissingFor("select-none-selected-size2-multiple") should be false. Was true.

The result contains FAIL lines.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:61
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

waitUntilDone() and notifyDone() are not needed.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:79
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/checkValidity-002-expected.txt:11
> +PASS checkValidityFor("select-non-placeholder") is true

checkValidity-002.html was for cases with checkValidity()==false.  You shouldn't include checkValidity()==true case if you follow it.

> LayoutTests/fast/forms/required-attribute-001.html:25
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

waitUntilDone() and notifyDone() are not needed.

> LayoutTests/fast/forms/required-attribute-001.html:34
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/required-attribute-002.html:25
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

ditto.

> LayoutTests/fast/forms/required-attribute-002.html:46
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:76
> +// --------------------------------
> +//     value change
> +// --------------------------------

We need more test cases for this.
You added some setNeedsValidityCheck() calls.  The test should cover all of code paths with them.

> WebCore/dom/SelectElement.cpp:217
> -void SelectElement::scrollToSelection(SelectElementData& data, Element* element)
> +void SelectElement::scrollToSelection(const SelectElementData& data, Element* element)

Adding "const" to existing code is not related to the bug.  Please make a separated bug and patch.

> WebCore/html/HTMLSelectElement.cpp:119
> +    // The number of the first option selected.
> +    int index = selectedIndex();

The variable name should be "firstSelectionIndex" and the comment should be removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.