Bug 76995 - WebKit fails IETC :indeterminate and input type=radio test
Summary: WebKit fails IETC :indeterminate and input type=radio test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 76198
  Show dependency treegraph
 
Reported: 2012-01-25 02:20 PST by Eric Seidel (no email)
Modified: 2012-02-06 02:19 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.00 KB, patch)
2012-01-25 21:20 PST, Joe Thomas
no flags Details | Formatted Diff | Diff
Patch2 (3.23 KB, patch)
2012-01-26 08:31 PST, Joe Thomas
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
patch3 (4.15 KB, patch)
2012-02-03 15:31 PST, Joe Thomas
tkent: review-
Details | Formatted Diff | Diff
Patch-BasedOn-ReviewComments (8.61 KB, patch)
2012-02-05 20:34 PST, Joe Thomas
tkent: review-
Details | Formatted Diff | Diff
PatchWithNewTestCase (13.26 KB, patch)
2012-02-05 23:29 PST, Joe Thomas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-01-25 02:20:18 PST
WebKit fails IETC :indeterminate and input type=radio test

http://samples.msdn.microsoft.com/ietestcenter/css3/showselectorstest.htm?indeterminate
Comment 1 Kent Tamura 2012-01-25 02:44:15 PST
We need a type check in HTMLInputElement::isIndeterminate().
Comment 2 Joe Thomas 2012-01-25 08:50:30 PST
what should be the correct behavior when indeterminate state is set for a radio button? I checked the spec at http://www.w3.org/TR/css3-selectors/#indeterminate, but did not get much information.
Comment 3 Kent Tamura 2012-01-25 15:35:28 PST
(In reply to comment #2)
> what should be the correct behavior when indeterminate state is set for a radio button? I checked the spec at http://www.w3.org/TR/css3-selectors/#indeterminate, but did not get much information.

The CSS3 specification says "Radio and checkbox". I think it is a mistake.

The HTML specification says only checkbox type respects indeterminate state.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#dom-input-indeterminate
Comment 4 Joe Thomas 2012-01-25 15:57:28 PST
(In reply to comment #3)
 
> The CSS3 specification says "Radio and checkbox". I think it is a mistake.
> 
> The HTML specification says only checkbox type respects indeterminate state.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#dom-input-indeterminate

Thanks! I will create a patch for this.
Comment 5 Joe Thomas 2012-01-25 16:27:44 PST
Webkit started supporting indeterminate for radio button as per bug https://bugs.webkit.org/show_bug.cgi?id=36273. The url mentioned in the bug 36273 is http://samples.msdn.microsoft.com/ietestcenter/css3/selectors/indeterminate.htm and it is the same IETC test link for this bug. Looks like IE updated their test case.
Comment 6 Joe Thomas 2012-01-25 21:20:11 PST
Created attachment 124061 [details]
Patch
Comment 7 Eric Seidel (no email) 2012-01-25 21:45:55 PST
Comment on attachment 124061 [details]
Patch

SGTM.
Comment 8 WebKit Review Bot 2012-01-26 00:06:30 PST
Comment on attachment 124061 [details]
Patch

Clearing flags on attachment: 124061

Committed r105968: <http://trac.webkit.org/changeset/105968>
Comment 9 WebKit Review Bot 2012-01-26 00:06:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Kent Tamura 2012-01-26 01:51:04 PST
Comment on attachment 124061 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124061&action=review

> Source/WebCore/html/HTMLInputElement.cpp:949
> -    if (!m_inputType->isCheckable() || indeterminate() == newValue)
> +    if (!isCheckbox() || indeterminate() == newValue)

This is wrong.
According to the specification, setter/getter of HTMLInputElement::indeterminate should work regardless of the type.  IE9, Firefox, and Opera work so.

As I already wrote, we need to update HTMLInputElement::isIndeterminate() so that :indeterminate pseudo class doesn't match to non-checkbox types.
Comment 11 Kent Tamura 2012-01-26 02:07:34 PST
Reverted r105968 for reason:

Incorrect behavior change

Committed r105980: <http://trac.webkit.org/changeset/105980>
Comment 12 Eric Seidel (no email) 2012-01-26 02:46:52 PST
Thank you tkent.
Comment 13 Joe Thomas 2012-01-26 08:31:17 PST
Created attachment 124121 [details]
Patch2

Modified the patch as per the review comments
Comment 14 Joe Thomas 2012-01-26 08:32:14 PST
(In reply to comment #10)
> (From update of attachment 124061 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124061&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:949
> > -    if (!m_inputType->isCheckable() || indeterminate() == newValue)
> > +    if (!isCheckbox() || indeterminate() == newValue)
> 
> This is wrong.
> According to the specification, setter/getter of HTMLInputElement::indeterminate should work regardless of the type.  IE9, Firefox, and Opera work so.
> 
> As I already wrote, we need to update HTMLInputElement::isIndeterminate() so that :indeterminate pseudo class doesn't match to non-checkbox types.

Thanks for the review. Modified patch is attached.
Comment 15 Darin Adler 2012-01-26 09:54:44 PST
Comment on attachment 124121 [details]
Patch2

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();
Comment 16 Darin Adler 2012-01-26 09:56:00 PST
Sorry, I didn’t see Kent’s earlier comment. It seems that Kent guided you to change CSS, but nothing else. If so, we should make a patch that does not affect appearance, does not affect accessibility APIs, and we should figure out if there is anything we need to do for :checked.
Comment 17 Kent Tamura 2012-01-26 17:24:38 PST
Are there any platforms supporting indeterminate appearance of radio buttons? At least Safari/OSX and Chromium on all platforms don't support it.
Comment 18 Joe Thomas 2012-01-27 12:16:53 PST
(In reply to comment #16)
> Sorry, I didn’t see Kent’s earlier comment. It seems that Kent guided you to change CSS, but nothing else. If so, we should make a patch that does not affect appearance, does not affect accessibility APIs, and we should figure out if there is anything we need to do for :checked.

Checked with Safari, Chrome, Firefox and IE and none of them have special appearance for indeterminate radio buttons. 

I do not have much idea about Accessibility component but since there is no special appearance for indeterminate radio buttons, I think the change will not affect Accessibility APIs.

For :checked pseudo class in SelectorChecker.cpp, we should respect the :checked property irrespective of radio button having indeterminate set (as indeterminate property does not have any appearance on its own). This patch does the same.

Please provide your feedback. Thanks.
Comment 19 Kent Tamura 2012-01-29 19:53:26 PST
Safari/Windows doesn't support indeterminate radio button.  However I found Safari/iOS supports it.

We had better introduce a compile-time flag or a RenderTheme function for indeterminate radio button support, and HTMLInputElement::isIndeterminate() should change its behavior according to the flag or the function.
Comment 20 Joe Thomas 2012-01-30 11:14:57 PST
(In reply to comment #19)
> Safari/Windows doesn't support indeterminate radio button.  However I found Safari/iOS supports it.
> 
If I try the indeterminate getter function on Safari on windows, I get the value which I set. And I don't see any difference in radio button appearance between Safari on Windows or on Mac if the indeterminate property is set.

I am not sure I missed something here, could you please give more details on what part of the indeterminate property is not supported on Safari/Windows?

> We had better introduce a compile-time flag or a RenderTheme function for indeterminate radio button support, and HTMLInputElement::isIndeterminate() should change its behavior according to the flag or the function.

Is it not better to have uniform behavior across all webkit browser on all platforms?
Comment 21 Kent Tamura 2012-02-01 22:52:41 PST
(In reply to comment #20)
> (In reply to comment #19)
> > Safari/Windows doesn't support indeterminate radio button.  However I found Safari/iOS supports it.
> > 
> If I try the indeterminate getter function on Safari on windows, I get the value which I set. And I don't see any difference in radio button appearance between Safari on Windows or on Mac if the indeterminate property is set.
> 
> I am not sure I missed something here, could you please give more details on what part of the indeterminate property is not supported on Safari/Windows?

I meant the appearance of indeterminate radio buttons.  Supporting indeterminate IDL attribute without the indeterminate appearance makes no sense.

> Is it not better to have uniform behavior across all webkit browser on all platforms?

It's better.  We need feedback from iOS developers.
Comment 22 Joe Thomas 2012-02-02 07:56:01 PST
(In reply to comment #21)

> > I am not sure I missed something here, could you please give more details on what part of the indeterminate property is not supported on Safari/Windows?
> 
> I meant the appearance of indeterminate radio buttons.  Supporting indeterminate IDL attribute without the indeterminate appearance makes no sense.
> 
Thanks for the clarification. I too agree on that.

> > Is it not better to have uniform behavior across all webkit browser on all platforms?
> 
> It's better.  We need feedback from iOS developers.

Since iOS supports indeterminate appearance, it might not be good to break it. As you mentioned in comment #19, it would be good to variate the feature. I will introduce the compile time flag in HTMLInputElement::isIndeterminate().
Comment 23 Joe Thomas 2012-02-03 14:48:02 PST
(In reply to comment #22)
> (In reply to comment #21)
> 
> Since iOS supports indeterminate appearance, it might not be good to break it. As you mentioned in comment #19, it would be good to variate the feature. I will introduce the compile time flag in HTMLInputElement::isIndeterminate().

The feature variation has to be done in HTMLInputElement::setIndeterminate as per the comment #21.
Comment 24 Joe Thomas 2012-02-03 15:31:23 PST
Created attachment 125433 [details]
patch3

Compile time flag added for IOS platform
Comment 25 Kent Tamura 2012-02-05 18:10:53 PST
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > 
> > Since iOS supports indeterminate appearance, it might not be good to break it. As you mentioned in comment #19, it would be good to variate the feature. I will introduce the compile time flag in HTMLInputElement::isIndeterminate().
> 
> The feature variation has to be done in HTMLInputElement::setIndeterminate as per the comment #21.

An, My comment #21 was confusing.  I meant that non-iOS platforms didn't support indeterminate radio buttons because they didn't support indeterminate appearance.  It didn't mean we didn't need to support HTMLInputELement::indeterminate in non-checkbox types.

- We need to support HTMLInputELement::indeterminate() and setIndetermiante() regardless of types.  This is defined by the standard and other browsers work so.
  So, We should remove !m_inputType->isCheckable() in HTMLInputElement::setIndetermiante().

- We don't need to support indeterminate radio button appearance except iOS.
 -- We should not return true from HTMLInputELement::isIndetermiante() except type=checkbox
 -- We don't need indeterminate-related code in RadioInputType.cpp

Additionally, we should not add branches by types like "if (isRadioButton())".  We should introduce InputType::supportsIndeterminateAppearance(), CheckboxInputType should return true for it, and RadioInputType should return true for it only on iOS.
Comment 26 Kent Tamura 2012-02-05 18:11:14 PST
Comment on attachment 125433 [details]
patch3

See comment #25.
Comment 27 Joe Thomas 2012-02-05 20:34:51 PST
Created attachment 125559 [details]
Patch-BasedOn-ReviewComments
Comment 28 Joe Thomas 2012-02-05 20:37:38 PST
(In reply to comment #25)

> An, My comment #21 was confusing.  I meant that non-iOS platforms didn't support indeterminate radio buttons because they didn't support indeterminate appearance.  It didn't mean we didn't need to support HTMLInputELement::indeterminate in non-checkbox types.
> 
> - We need to support HTMLInputELement::indeterminate() and setIndetermiante() regardless of types.  This is defined by the standard and other browsers work so.
>   So, We should remove !m_inputType->isCheckable() in HTMLInputElement::setIndetermiante().
> 
> - We don't need to support indeterminate radio button appearance except iOS.
>  -- We should not return true from HTMLInputELement::isIndetermiante() except type=checkbox
>  -- We don't need indeterminate-related code in RadioInputType.cpp
> 
> Additionally, we should not add branches by types like "if (isRadioButton())".  We should introduce InputType::supportsIndeterminateAppearance(), CheckboxInputType should return true for it, and RadioInputType should return true for it only on iOS.

Thanks for the clarification. A new patch is submitted based on the suggestions.
Comment 29 Kent Tamura 2012-02-05 20:52:47 PST
Comment on attachment 125559 [details]
Patch-BasedOn-ReviewComments

View in context: https://bugs.webkit.org/attachment.cgi?id=125559&action=review

> Source/WebCore/html/HTMLInputElement.cpp:950
>  {
> -    if (!m_inputType->isCheckable() || indeterminate() == newValue)
> +    if (indeterminate() == newValue)
>          return;

We need a test for this new behavior.

> Source/WebCore/html/RadioInputType.cpp:157
>      state->checked = element()->checked();
> -    state->indeterminate = element()->indeterminate();
>      state->checkedRadioButton = element()->checkedRadioButtons().checkedButtonForGroup(element()->name());
>  
> -    if (element()->indeterminate())
> -        element()->setIndeterminate(false);
>      element()->setChecked(true, true);

Need to enclose PLATFORM(IOS)?

> Source/WebCore/html/RadioInputType.cpp:174
>          }
> -        element()->setIndeterminate(state.indeterminate);
>      }

ditto.

> Source/WebCore/html/RadioInputType.cpp:191
> +#if PLATFORM(IOS)
> +    return true;
> +#endif
> +
> +    return false;

There are two returns if PLATFORM(IOS).
Comment 30 Joe Thomas 2012-02-05 23:29:28 PST
Created attachment 125581 [details]
PatchWithNewTestCase

New test case added
Review comments fixed.
Comment 31 Kent Tamura 2012-02-05 23:52:25 PST
Comment on attachment 125581 [details]
PatchWithNewTestCase

ok
Comment 32 Joe Thomas 2012-02-05 23:54:19 PST
(In reply to comment #31)
> (From update of attachment 125581 [details])
> ok

Thanks Kent. Could you please help me to land this patch?
Comment 33 WebKit Review Bot 2012-02-06 02:19:14 PST
Comment on attachment 125581 [details]
PatchWithNewTestCase

Clearing flags on attachment: 125581

Committed r106792: <http://trac.webkit.org/changeset/106792>
Comment 34 WebKit Review Bot 2012-02-06 02:19:23 PST
All reviewed patches have been landed.  Closing bug.