[webkit-reviews] review denied: [Bug 76995] WebKit fails IETC :indeterminate and input type=radio test : [Attachment 124121] Patch2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Joe Thomas <joethomas at motorola.com>'s
request for review:
Bug 76995: WebKit fails IETC :indeterminate and input type=radio test
https://bugs.webkit.org/show_bug.cgi?id=76995

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124121&action=review


While this patch seems OK, I have a few problems with it, things that seem to
be loose ends. Mainly, this seems to leave the indeterminate feature
half-implemented for radio buttons in quite a few ways that seem random and
some of those are probably wrong. The comments make it seem like we are trying
to change indeterminate only from the point of view of CSS pseudo-classes, but
the patch does more than that.

1) HTMLInputElement::setIndeterminate goes out of its way to allow setting
indeterminate on radio buttons as well as check boxes. If setting a radio
button to indeterminate should not be allowed, then the best fix for this bug
might be right there. We need a test covering this.

2) The isIndeterminate function is used in the RenderTheme::isIndeterminate
function, so this means that radio buttons likely won’t be rendering as
indeterminate any more. Is that change intentional? Either way, that change
should be covered by a test.

3) The isIndeterminate function is used for accessibility code in
AccessibilityRenderObject as well. Thus, after this patch radio buttons will no
longer show up as indeterminate for screen reader users. We should figure out
if this change is desirable or not, and make sure that we have test coverage
for it.

4) I can see by inspection of SelectorChecker::checkOneSelector that the
isIndeterminate function is used for the pseudo-style :checked as well as for
the pseudo-style :indeterminate, so this patch affects that style too in
certain cases. We should have a test case showing that behavior has changed to
make sure the change is for the better.

5) There is some radio-button-specific code to track indeterminate state in
RadioInputType::willDispatchClick and RadioInputType::didDispatchClick. Perhaps
that code is now dead and should be removed. We should figure out which tests
cover that code.

> Source/WebCore/html/HTMLInputElement.cpp:1837
> +    if (!isCheckbox())
> +	   return false;
> +    return indeterminate();

I would write this with &&

    return isCheckBox() && indeterminate();