Bug 76995 – WebKit fails IETC :indeterminate and input type=radio test

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

WebKit fails IETC :indeterminate and input type=radio test

http://samples.msdn.microsoft.com/ietestcenter/css3/showselectorstest.htm?indeterminate

[Kent Tamura]

We need a type check in HTMLInputElement::isIndeterminate().

[Joe Thomas]

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.

[Kent Tamura]

(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

[Joe Thomas]

(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.

[Joe Thomas]

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.

[Joe Thomas]

Created attachment 124061 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 124061 [details]
Patch

SGTM.

[WebKit Review Bot]

Comment on attachment 124061 [details]
Patch

Clearing flags on attachment: 124061

Committed r105968: <http://trac.webkit.org/changeset/105968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

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.

[Kent Tamura]

Reverted r105968 for reason:

Incorrect behavior change

Committed r105980: <http://trac.webkit.org/changeset/105980>

[Eric Seidel (no email)]

Thank you tkent.

[Joe Thomas]

Created attachment 124121 [details]
Patch2

Modified the patch as per the review comments

[Joe Thomas]

(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.

[Darin Adler]

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();

[Darin Adler]

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.

[Kent Tamura]

Are there any platforms supporting indeterminate appearance of radio buttons? At least Safari/OSX and Chromium on all platforms don't support it.

[Joe Thomas]

(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.

[Kent Tamura]

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.

[Joe Thomas]

(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?

[Kent Tamura]

(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.

[Joe Thomas]

(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().

[Joe Thomas]

(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.

[Joe Thomas]

Created attachment 125433 [details]
patch3

Compile time flag added for IOS platform

[Kent Tamura]

(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.

[Kent Tamura]

Comment on attachment 125433 [details]
patch3

See comment #25.

[Joe Thomas]

Created attachment 125559 [details]
Patch-BasedOn-ReviewComments

[Joe Thomas]

(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.

[Kent Tamura]

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).

[Joe Thomas]

Created attachment 125581 [details]
PatchWithNewTestCase

New test case added
Review comments fixed.

[Kent Tamura]

Comment on attachment 125581 [details]
PatchWithNewTestCase

ok

[Joe Thomas]

(In reply to comment #31)
> (From update of attachment 125581 [details])
> ok

Thanks Kent. Could you please help me to land this patch?

[WebKit Review Bot]

Comment on attachment 125581 [details]
PatchWithNewTestCase

Clearing flags on attachment: 125581

Committed r106792: <http://trac.webkit.org/changeset/106792>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.