WebKit fails IETC :indeterminate and input type=radio test http://samples.msdn.microsoft.com/ietestcenter/css3/showselectorstest.htm?indeterminate
We need a type check in HTMLInputElement::isIndeterminate().
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.
(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
(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.
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.
Created attachment 124061 [details] Patch
Comment on attachment 124061 [details] Patch SGTM.
Comment on attachment 124061 [details] Patch Clearing flags on attachment: 124061 Committed r105968: <http://trac.webkit.org/changeset/105968>
All reviewed patches have been landed. Closing bug.
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.
Reverted r105968 for reason: Incorrect behavior change Committed r105980: <http://trac.webkit.org/changeset/105980>
Thank you tkent.
Created attachment 124121 [details] Patch2 Modified the patch as per the review comments
(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 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();
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.
Are there any platforms supporting indeterminate appearance of radio buttons? At least Safari/OSX and Chromium on all platforms don't support it.
(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.
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.
(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?
(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.
(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().
(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.
Created attachment 125433 [details] patch3 Compile time flag added for IOS platform
(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 on attachment 125433 [details] patch3 See comment #25.
Created attachment 125559 [details] Patch-BasedOn-ReviewComments
(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 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).
Created attachment 125581 [details] PatchWithNewTestCase New test case added Review comments fixed.
Comment on attachment 125581 [details] PatchWithNewTestCase ok
(In reply to comment #31) > (From update of attachment 125581 [details]) > ok Thanks Kent. Could you please help me to land this patch?
Comment on attachment 125581 [details] PatchWithNewTestCase Clearing flags on attachment: 125581 Committed r106792: <http://trac.webkit.org/changeset/106792>