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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 2 23:25:30 PST 2010 

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


Kent Tamura <tkent at chromium.org> changed:

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




--- Comment #8 from Kent Tamura <tkent at chromium.org>  2010-12-02 23:25:30 PST ---
(From update of attachment 75465)
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().

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

More information about the webkit-unassigned mailing list