[webkit-reviews] review denied: [Bug 50380] Implement "required" attribute for select tags : [Attachment 75465] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 2 23:25:29 PST 2010
Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 50380: Implement "required" attribute for select tags
https://bugs.webkit.org/show_bug.cgi?id=50380
Attachment 75465: Patch
https://bugs.webkit.org/attachment.cgi?id=75465&action=review
------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75465&action=review
> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:30
> + shouldBe('v[0].validity.valueMissing', 'true');
> + shouldBe('v[1].validity.valueMissing', 'true');
> + shouldBe('v[2].validity.valueMissing', 'true');
> + shouldBe('v[3].validity.valueMissing', 'false');
> + shouldBe('v[4].validity.valueMissing', 'false');
> + shouldBe('v[5].validity.valueMissing', 'false');
> + shouldBe('v[6].validity.valueMissing', 'false');
> + shouldBe('v[7].validity.valueMissing', 'false');
> + shouldBe('v[8].validity.valueMissing', 'false');
> + shouldBe('v[9].validity.valueMissing', 'false');
The resultant text doesn't have good readability.
How about adding id= attributes to the victims, and change these checks like
function valueMissingFor(id) { return
document.getElmeentById(id).validity.valueMissing; }
shouldBeTrue('valueMissingFor("input-requied")');
shouldBeFalse('valueMissingFor("select-without-placeholder-label")');
> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:40
> <body onload="test()">
We don't need to make this test async though It's not your responsibility.
We can move the <script> element into <body>, and add <script> for
js-test-post.js like tests you have updated before.
> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
Same comments as ValidityState-valueMissing-001.html.
> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
Same comments as ValidityState-valueMissing-001.html.
> LayoutTests/fast/forms/checkValidity-002.html:31
> +shouldBe("v[0].checkValidity()", "false");
> +shouldBe("v[1].checkValidity()", "false");
> +shouldBe("v[2].checkValidity()", "false");
> +shouldBe("v[3].checkValidity()", "false");
> +shouldBe("v[4].checkValidity()", "true");
Same comments as ValidityState-valueMissing-001.html.
> LayoutTests/fast/forms/required-attribute-001.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
Same comments as ValidityState-valueMissing-001.html.
> LayoutTests/fast/forms/required-attribute-002.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
Same comments as ValidityState-valueMissing-001.html.
> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:10
> + var i = document.createElement('select');
Using the name "i" for this is not reasonable.
> WebCore/html/HTMLSelectElement.cpp:115
> + if (!isRequiredFormControl() || disabled() || multiple() || size() > 1)
We don't need to check disabled() here. It's responsibility of willValidate().
We should not return false for multiple() unconditionally. The specification
says we never have placeholder label option for multiple, but required should
work with multiple.
> WebCore/html/HTMLSelectElement.cpp:297
> + bool result = SelectElement::appendFormData(m_data, this, list);
> + if (result)
> + setNeedsValidityCheck();
Why is this needed?
> WebCore/html/HTMLSelectElement.cpp:417
> + setNeedsValidityCheck();
We don't need this. setAttribute() calls parseMappedAttribute().
> WebCore/html/HTMLSelectElement.cpp:423
> + setNeedsValidityCheck();
We should handle this in parseMappedAttribtue().
> WebCore/html/HTMLSelectElement.cpp:502
> + setNeedsValidityCheck();
Why is this needed?
> WebCore/html/HTMLSelectElement.h:48
> + bool valueMissing(const String&) const;
> + bool isValidValue(const String& candidate) const { return
!valueMissing(candidate); }
isValidValue() is not used. We don't need to add it.
So, the parameter of valueMissing() is not needed. It's always this->value().
More information about the webkit-reviews
mailing list