[Webkit-unassigned] [Bug 76995] WebKit fails IETC :indeterminate and input type=radio test

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #124121|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #15 from Darin Adler <darin at apple.com>  2012-01-26 09:54:44 PST ---
(From update of attachment 124121)
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();

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