[webkit-reviews] review denied: [Bug 87614] REGRESSION(r109729): The optgroup element's "disabled" attribute has no effect to rendering and selection : [Attachment 144471] Patch 5

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 87614: REGRESSION(r109729): The optgroup element's "disabled" attribute has
no effect to rendering and selection
https://bugs.webkit.org/show_bug.cgi?id=87614

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

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


> LayoutTests/ChangeLog:12
> +	   * fast/forms/select/optgroup-rendering.html: Added.

Please add the following line to platform/chromium/test_expectations.txt

BUGWK87614 : fast/forms/select/optgroup-rendering.html = FAIL MISSING

> LayoutTests/fast/forms/select/optgroup-clicking-expected.txt:11
> +On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".
> +
> +
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
> +Click enabled option
> +PASS $("listbox").selectedIndex is 1

'TEST COMPLETE' then showing tests is wrong.  Please follow the comments below.


> LayoutTests/fast/forms/select/optgroup-clicking.html:47
> +window.jsTestIsAsync;

This makes no sense.
It should be:
    window.jsTestIsAsync = true;

> LayoutTests/fast/forms/select/optgroup-clicking.html:52
> +    if (!window.layoutTestController)
> +	   return;

You need to show a message.  'This test needs layoutTestController.' is ok.

> LayoutTests/fast/forms/select/optgroup-clicking.html:75
> +    shouldBe('$("menulist").selectedIndex', '8');
> +}

You should add finishJSTest();.

> LayoutTests/fast/forms/select/optgroup-clicking.html:77
> +<script src="../../js/resources/js-test-post-async.js"></script>

You should load js-test-post.js.

> LayoutTests/fast/forms/select/optgroup-rendering.html:6
> +    optbox:disabled { color: pink; }
> +    optbox:enabled { color: green; }

They should optgroup:disabled and optgroup:enabled, right?