Radio groups without a selected radio button should have :indeterminate applying

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: mounir, Assigned: jessica, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla51
dev-doc-complete, html5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [lang=c++][parity-chrome][parity-safari][parity-webkit], URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
See:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21820
http://html5.org/tools/web-apps-tracker?from=7909&to=7910
Created attachment 791404 [details] [diff] [review]
bug885359.patch

Mounir, could you please provide some details to solve this bug ? 
I was looking to HTMLInputElement::AddedToRadioGroup() and HTMLInputElement::WillRemoveFromRadioGroup() to set mIndeterminate state.
When the page loads, indeterminate state is set to false even if it's not in a group. It only works when I change manually the name attribute.
Attachment #791404 - Flags: feedback?(mounir)
(Reporter)

Comment 2

4 years ago
Comment on attachment 791404 [details] [diff] [review]
bug885359.patch

To give a summary of what the feature is about: a radio group is a group of elements a user can check but only one element can be checked at the same time but at load time, no element could be checked, in which case, the radio group is in an indeterminate situation.

For example, those groups are indeterminate:
<input type='radio' name='foo'>
<input type='radio' name='foo'>
--
<input type='radio' name='bar'>
--
<input type='radio'>

But not those ones:
<input type='radio' checked>
--
<input type='radio' name='foo'>
<input type='radio' name='foo' checked>
--
<input type='radio' name='bar' checked>
<input type='radio' name='bar' checked>

Regarding the code, here, what we want is to have a pseudo-class applying. The way to test that is to use .mozMatchesSelector(":indeterminate) in Javascript. For example:
 alert(input.mozMatchesSelector(":indeterminate"));
... would prompt the current indeterminate state of |input|.

Those states are defined in ::IntrinsicState() for HTML elements. For HTMLInputElement, it would be:
https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#4691

As you can see, a checkbox is marked as indeterminate when it is not checked but a radio button is never marked as indeterminate.

We want to mark a radio button as indeterminate if no radio is selected in its current group. You can call HTMLInputElement::GetSelectedRadioButton() to get the currently selected radio in the group. This will return nullptr if there is no selected radio or if the radio is not in a group. The radio is not in a group when there is no name attribute defined. Like in "<input type='radio'>".
If the radio is not in a group, we would like to still know if it is indeterminate. In that case, it would simply be that it is not checked. So, we could simply know if a radio is indeterminate by doing:
  bool indeterminate = !GetSelectedRadioButton() && !mChecked;

Note that ::IntrinsicState() is called at some specific times (when needed) for performance reasons so we need to make sure that anytime the selected radio changes or the checkedness changes we call ::IntrinsicState(). It happens that it should be the case because ":invalid" state should be doing those checks.

Finally, you should write some tests for this. At least a mochitest in content/html/content/html/test/forms/. See https://developer.mozilla.org/en-US/docs/Mochitest to learn about mochitests.
Also, writing some reftests would be great. Especially to make sure that the changes happen when they should. Some information about reftests: https://developer.mozilla.org/en/docs/Creating_reftest-based_unit_tests However, styling radio buttons is a pain and I am not sure whan kind of reftests you could write.

Let me know if you need more information.
Attachment #791404 - Flags: feedback?(mounir) → feedback-
Assignee: nobody → s
Status: NEW → ASSIGNED
Created attachment 792286 [details] [diff] [review]
WIP

Okey, Here is the new patch, I'm still a little bit confused but let me know your feedbacks.

I couldn't use the HTMLInput::GetSelectedRadioButton() function inside HTMLInputElement::IntrinsicState() because it's a non-const function member.
I tried to make IntrinsicState() non-static but this didn't fix the radio indeterminate state.

So here is my solution but still buggy.
In the case of : 
<input type='radio' name='foo' id='1'>
<input type='radio' name='foo' id='2'>

When the page finished loading, theses elements are in an indeterminate state (.mozMatchesSelector(':indeterminate') == true) but when I try to change one element checked to true for example elt1.checked = true, the indeterminate state of the checked element is false, and all the others in the same group still indeterminate == true.


I have also a little question, is there any differences between the indeterminate state (:indeterminate) and the indeterminate attribute of the element ? (input.indeterminate) ? because nothing changes in that side any explanations ?
Attachment #791404 - Attachment is obsolete: true
Attachment #792286 - Flags: feedback?(mounir)
I'm also worried about what you said about calling ::IntrinsicState() for performance reasons, when I added some debug outputs after assigning the bool indeterminate variable I noticed that the out put was printed 3 or 4 times for each element, do the calls that I added are good ?
(Reporter)

Comment 5

4 years ago
(In reply to Alexandre BM (:rednaks) from comment #3)
> I couldn't use the HTMLInput::GetSelectedRadioButton() function inside
> HTMLInputElement::IntrinsicState() because it's a non-const function member.
> I tried to make IntrinsicState() non-static but this didn't fix the radio
> indeterminate state.

You should try to make HTMLInputElement::GetSelectedRadioButton() const. Tries to understand what forbids this. It might be easily doable. If this is really a pain, you can do tricks like:
const_cast<HTMLInputElement*>(this)->GetSelectedRadioButton()
but please check if making the function |const| is doable before going that way.

> So here is my solution but still buggy.
> In the case of : 
> <input type='radio' name='foo' id='1'>
> <input type='radio' name='foo' id='2'>
> 
> When the page finished loading, theses elements are in an indeterminate
> state (.mozMatchesSelector(':indeterminate') == true) but when I try to
> change one element checked to true for example elt1.checked = true, the
> indeterminate state of the checked element is false, and all the others in
> the same group still indeterminate == true.

Reading your code, I do not see what could be wrong. Could you show me the test page?

> I have also a little question, is there any differences between the
> indeterminate state (:indeterminate) and the indeterminate attribute of the
> element ? (input.indeterminate) ? because nothing changes in that side any
> explanations ?

The indeterminate attribute is defined here:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#dom-input-indeterminate

So, the behaviour you are seeing is quite expected.

(In reply to Alexandre BM (:rednaks) from comment #4)
> I'm also worried about what you said about calling ::IntrinsicState() for
> performance reasons, when I added some debug outputs after assigning the
> bool indeterminate variable I noticed that the out put was printed 3 or 4
> times for each element, do the calls that I added are good ?

No. Actually, I was not clear about this. IntrinsicState() is not called explicitly. Updates are made via |UpdateState(bool aNotify);| and you do not need to call UpdateState() for SetChecked(), DoSetChecked() will actually call UpdateState() if needed (it is ending calling HTMLInputElement::SetCheckedChangedInternal()).

You might need to call UpdateState() when a radio element is added/removed from a radio group. I think the added/removed element will be updated but not the other elements in the group.
(Reporter)

Updated

4 years ago
Attachment #792286 - Flags: feedback?(mounir)
(In reply to Mounir Lamouri (:mounir) from comment #5)
> (In reply to Alexandre BM (:rednaks) from comment #3)
> > I couldn't use the HTMLInput::GetSelectedRadioButton() function inside
> > HTMLInputElement::IntrinsicState() because it's a non-const function member.
> > I tried to make IntrinsicState() non-static but this didn't fix the radio
> > indeterminate state.
> 
> You should try to make HTMLInputElement::GetSelectedRadioButton() const.
> Tries to understand what forbids this. It might be easily doable. If this is

Changeing ::GetSelectedRadioButton() method to const would probably mean that I have to change a lot of code ..., It seems that the issue is due to AddRef, which is modifying refcount (IRC discussion) so I'll try your to make it const then your trick.

> really a pain, you can do tricks like:
> const_cast<HTMLInputElement*>(this)->GetSelectedRadioButton()
> but please check if making the function |const| is doable before going that
> way.
> 
> > So here is my solution but still buggy.
> > In the case of : 
> > <input type='radio' name='foo' id='1'>
> > <input type='radio' name='foo' id='2'>
> > 
> > When the page finished loading, theses elements are in an indeterminate
> > state (.mozMatchesSelector(':indeterminate') == true) but when I try to
> > change one element checked to true for example elt1.checked = true, the
> > indeterminate state of the checked element is false, and all the others in
> > the same group still indeterminate == true.
> 
> Reading your code, I do not see what could be wrong. Could you show me the
> test page?
It's probably related to the ::IntrinsicState() call, I'll investigate.
Okey I'll attach my test page.
> 
> > I have also a little question, is there any differences between the
> > indeterminate state (:indeterminate) and the indeterminate attribute of the
> > element ? (input.indeterminate) ? because nothing changes in that side any
> > explanations ?
> 
> The indeterminate attribute is defined here:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-
> element.html#dom-input-indeterminate
> 
> So, the behaviour you are seeing is quite expected.
Right :)
> 
> (In reply to Alexandre BM (:rednaks) from comment #4)
> > I'm also worried about what you said about calling ::IntrinsicState() for
> > performance reasons, when I added some debug outputs after assigning the
> > bool indeterminate variable I noticed that the out put was printed 3 or 4
> > times for each element, do the calls that I added are good ?
> 
> No. Actually, I was not clear about this. IntrinsicState() is not called
> explicitly. Updates are made via |UpdateState(bool aNotify);| and you do not
> need to call UpdateState() for SetChecked(), DoSetChecked() will actually
> call UpdateState() if needed (it is ending calling
> HTMLInputElement::SetCheckedChangedInternal()).
> 
> You might need to call UpdateState() when a radio element is added/removed
> from a radio group. I think the added/removed element will be updated but
> not the other elements in the group.
Created attachment 792936 [details]
radioButtonIndeterminateState.html test page
Sorry I was wrong, Making ::GetSelectedRadioButton() const worked. But what about the IRC discussion ? that AddRef is modifing refcout, what's making it non callable in const functions ?
(Reporter)

Comment 9

4 years ago
(In reply to Alexandre BM (:rednaks) from comment #8)
> Sorry I was wrong, Making ::GetSelectedRadioButton() const worked. But what
> about the IRC discussion ? that AddRef is modifing refcout, what's making it
> non callable in const functions ?

I did not participate to that discussion so I will need more context about this :)
I was asking on IRC what forbids the call of ::GetSelectedRadioButton(), and that's what I got as an answer : 

"sounds like you're holding a strong pointer, which involved calling AddRef, which changes a refcount member... Sounds like something you shouldn't do in a const method"

What about the test did you try it ?
Created attachment 793516 [details] [diff] [review]
WIP

Now I'm using ::GetSelectedRadioButton() after making it const.
I also removed ::IntrinsicState() calls, and the tests still good, only in the case mentioned in the comment #6
Attachment #792286 - Attachment is obsolete: true
(Reporter)

Comment 12

4 years ago
Comment on attachment 793516 [details] [diff] [review]
WIP

Review of attachment 793516 [details] [diff] [review]:
-----------------------------------------------------------------

f+ but we need some tests :)

::: content/html/content/src/HTMLInputElement.cpp
@@ +4715,5 @@
>      }
>  
> +    //Checking current indeterminate state for radio button
> +    if (mType == NS_FORM_INPUT_RADIO) {
> +      bool indeterminate = !static_cast<nsCOMPtr<nsIDOMHTMLInputElement>>(GetSelectedRadioButton()) && !mChecked;

Lets do:
nsCOMPtr<nsIDOMHTMLInputElement> selected = GetSelectedRadioButton();
bool indeterminate = !selected && !checked;

It will be easier to read :)

@@ +4717,5 @@
> +    //Checking current indeterminate state for radio button
> +    if (mType == NS_FORM_INPUT_RADIO) {
> +      bool indeterminate = !static_cast<nsCOMPtr<nsIDOMHTMLInputElement>>(GetSelectedRadioButton()) && !mChecked;
> +      if (indeterminate)
> +        state |= NS_EVENT_STATE_INDETERMINATE;

Coding style requires:
if (indeterminate) {
  state |= NS_EVENT_STATE_INDETERMINATE;
}
Attachment #793516 - Flags: feedback+
(Reporter)

Comment 13

4 years ago
(In reply to Alexandre BM (:rednaks) from comment #10)
> I was asking on IRC what forbids the call of ::GetSelectedRadioButton(), and
> that's what I got as an answer : 
> 
> "sounds like you're holding a strong pointer, which involved calling AddRef,
> which changes a refcount member... Sounds like something you shouldn't do in
> a const method"

You don't AddRef |this| so I do not see what's wrong here. There must have been some misunderstanding.

> What about the test did you try it ?

Unfortunately not and I will not have time for this today. Let me know if you have some specific question.
Keywords: dev-doc-needed
Mentor: mounir@lamouri.fr
Whiteboard: [mentor=mounir][lang=c++] → [lang=c++]
Created attachment 8443978 [details] [diff] [review]
Radiobutton have indeterminate state if not selected
Attachment #8443978 - Flags: review?(mounir)
Attachment #793516 - Attachment is obsolete: true

Comment 15

3 years ago
Hi mounir,
I was writing the tests when I noticed that |indeterminate = !selected && !mChecked| is not covering all the cases.

Everything is okey for : 

<input id="1" type="radio" name="foo"/>
<input id="2" type="radio" name="foo"/>

It's state is indeterminate,
But, if we change |checked| of one element to true, only the state of the checked button will change to determinate. the other still indeterminate. 
The first idea was that it seems like other buttons in the group were not notified.

Actually I'm talking with jdm, and he suggested to replace ::UpdateState() in ::SetCheckedInternal() with ::DoSetCheckedChanged().
(Reporter)

Comment 16

3 years ago
Comment on attachment 8443978 [details] [diff] [review]
Radiobutton have indeterminate state if not selected

Review of attachment 8443978 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! :) Nice catch for the invalidation bug but I think we should try to fix it more generally instead of adding the call where you did. What do you think?

Also, for the tests, I was thinking of a few more scenarios:
- what if you set a radio to checked = false after setting it to checked = true? Should that trigger :indeterminate back?
- check that changing the selected radio keep the state as expected
- check that if the selected radio is removed, the group is back to :indeterminate (that's the expected behaviour, right?)
- same with adding a selected radio in a group of unselected radios.

::: content/html/content/src/HTMLInputElement.cpp
@@ +3091,5 @@
>    // Notify the document that the CSS :checked pseudoclass for this element
>    // has changed state.
> +
> +  // |UpdateState| is not notifying all the members groups - bug 885359
> +  DoSetCheckedChanged(false, aNotify);

I do not think that's the right thing to do. As you can see from the comments in ::DoSetChecked(), the code seems to assume that all the radio elements will be notified of the change. However, the notification happens at the beginning of the function (::DoSetCheckedChanged()) when the radio selected element has not changed.

You should start by understanding why the update is done at the beginning of the function and whether it's safe to have the state update done at the end. We should not have two state updates in one call ideally though.

::: content/html/content/test/mochitest.ini
@@ +413,5 @@
>  [test_bug879319.html]
>  [test_bug885024.html]
>  [test_bug893537.html]
>  [test_bug969346.html]
> +[test_bug885359.html]

I think that the test is at the wrong place and with the wrong name.
It should be in content/html/content/test/forms/
and named test_input_radio_indeterminate.html

A lot of tests have test_bug${BUG}.html as a name but that is a bad practice because you have no idea what the test is testing until you actually open it.

::: content/html/content/test/test_bug885359.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<html>
> +

nit: remove this empty line

@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +
> +  <head>
> +

nit: remove this empty line

@@ +4,5 @@
> +  <head>
> +
> +    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +

nit: remove this empty line

@@ +12,5 @@
> +      function check1(){
> +        var elt1 = document.getElementById('1');
> +        if(elt1.checked)
> +          return;
> +        elt1.checked = true;

I think you can have a generic check(id) function that looks like:
function check(id) {
  document.getElementById(id).checked = true;
}

@@ +14,5 @@
> +        if(elt1.checked)
> +          return;
> +        elt1.checked = true;
> +      }
> +    

nit: trailing white spaces

@@ +22,5 @@
> +        if(elt2.checked)
> +          return;
> +        elt2.checked = true;
> +      }
> +    

nit: trailing white spaces

@@ +29,5 @@
> +        var elt3 = document.getElementById('3');
> +        if(!elt2.checked)
> +          return;
> +        elt2.checked = false;
> +      }

Same as for check, you can do:
function uncheck(id) {
  document.getElementById(id).checked = false;
}

You don't seem to use that function though.

@@ +30,5 @@
> +        if(!elt2.checked)
> +          return;
> +        elt2.checked = false;
> +      }
> +      

nit: trailing white spaces

@@ +40,5 @@
> +        var elt5Matches = document.getElementById('5').mozMatchesSelector(':indeterminate');
> +
> +        is(elt1Matches, true, "Should be indeterminate if not checked");
> +        is(elt2Matches && elt3Matches, true, "All states Should be indeterminate if no one is checked");
> +        is(elt4Matches || elt5Matches, false, "All states Should not be indeterminate if one is checked");

Could you change that with a list like:

is(document.getElementById('1').mozMatchesSelector(':indeterminate'), true, ...);
is(document.getElementById('2').mozMatchesSelector(':indeterminate'), true, ...);
is(document.getElementById('3').mozMatchesSelector(':indeterminate'), true, ...);
is(document.getElementById('4').mozMatchesSelector(':indeterminate'), false, ...);
is(document.getElementById('5').mozMatchesSelector(':indeterminate'), false, ...);

@@ +46,5 @@
> +    
> +      function testSetChecked() {
> +        check1();
> +        var elt1Matches = document.getElementById('1').mozMatchesSelector(':indeterminate');
> +        is(elt1Matches, false, "If set checked, input should not indeterminate anymore");

This should look like:
check('1');
is(document.getElementById('1').mozMatchesSelector(':indeterminate'), false, ...);

@@ +52,5 @@
> +        check2();
> +        var elt2Matches = document.getElementById('2').mozMatchesSelector(':indeterminate');
> +        var elt3Matches = document.getElementById('3').mozMatchesSelector(':indeterminate');
> +
> +        is(elt2Matches || elt3Matches, false, "The group is anymore indeterminate after checking one element");

And that should look like:
check('2');
is(document.getElementById('2').mozMatchesSelector(':indeterminate'), false, ...);
is(document.getElementById('3').mozMatchesSelector(':indeterminate'), false, ...);

@@ +74,5 @@
> +      testDefault();
> +      testSetChecked();
> +    </script>
> +  </body>
> + 

nit: remove this empty line
Attachment #8443978 - Flags: review?(mounir) → feedback+
(In reply to Mounir Lamouri (:mounir) from comment #16)
> Comment on attachment 8443978 [details] [diff] [review]
> Radiobutton have indeterminate state if not selected
> 
> Review of attachment 8443978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! :) Nice catch for the invalidation bug but I think we
> should try to fix it more generally instead of adding the call where you
> did. What do you think?
> 
> Also, for the tests, I was thinking of a few more scenarios:
> - what if you set a radio to checked = false after setting it to checked =
> true? Should that trigger :indeterminate back?
> - check that changing the selected radio keep the state as expected
> - check that if the selected radio is removed, the group is back to
> :indeterminate (that's the expected behaviour, right?)
> - same with adding a selected radio in a group of unselected radios.

Ofcourse !

> 
> ::: content/html/content/src/HTMLInputElement.cpp
> @@ +3091,5 @@
> >    // Notify the document that the CSS :checked pseudoclass for this element
> >    // has changed state.
> > +
> > +  // |UpdateState| is not notifying all the members groups - bug 885359
> > +  DoSetCheckedChanged(false, aNotify);
> 
> I do not think that's the right thing to do. As you can see from the
> comments in ::DoSetChecked(), the code seems to assume that all the radio
> elements will be notified of the change. However, the notification happens
> at the beginning of the function (::DoSetCheckedChanged()) when the radio
> selected element has not changed.
> 
> You should start by understanding why the update is done at the beginning of
> the function and whether it's safe to have the state update done at the end.
> We should not have two state updates in one call ideally though.


We forced the call of DoSetCheckedChanged(false, aNotify) because for some reason,
|mCheckedChanged| is set to |true| after the load of the page, 
and this will prevent DoSetCheckedChanged() from creating a vistor to notify all 
the other elements of the group in SetCheckedInternal().

I'm not sure about the role of |mCheckedChanged| but I suppose that's used to
break the recursive call of DoSetCheckedChanged() when visiting elements.
So, I updated the tests, and I discovered that as you said, the problem might be more general because it's failing for thes last two cases : 

> - check that if the selected radio is removed, the group is back to
> :indeterminate (that's the expected behaviour, right?)
> - same with adding a selected radio in a group of unselected radios.
PS: I'm not sure if it's the right way to add/remove an element from a group, I just modified the @name.

Comment 20

a year ago
WebKit just implemented this: http://trac.webkit.org/changeset/202197
And it already works in Chrome.
Whiteboard: [lang=c++] → [lang=c++][parity-chrome][parity-safari][parity-webkit]

Updated

a year ago
(Assignee)

Comment 21

a year ago
Hi Alexandre, are you still working on this? If not, can I carry on with the work here? Thanks.
Flags: needinfo?(s)
(Assignee)

Comment 22

11 months ago
If you don't mind, I'd like to carry on with the work here. :)
Assignee: s → jjong
(Assignee)

Comment 23

10 months ago
Created attachment 8781859 [details] [diff] [review]
patch, v1.
(Assignee)

Comment 24

10 months ago
Created attachment 8781902 [details] [diff] [review]
patch, v1.

Olli, we are adding :indeterminate support for radio buttons, may I have your review? Thanks.
Attachment #8781859 - Attachment is obsolete: true
Attachment #8781902 - Flags: review?(bugs)

Comment 25

10 months ago
Comment on attachment 8781902 [details] [diff] [review]
patch, v1.

>+function test() {
>+  // Initial State.
>+  verifyIndeterminateState(radio1, true,
>+    "Unchecked radio in its own group (no name attribute)");
>+  verifyIndeterminateState(g1radio1, true, "No selected radio in its group");
>+  verifyIndeterminateState(g1radio2, true, "No selected radio in its group");
>+  verifyIndeterminateState(g1radio3, true, "No selected radio in its group");
>+  verifyIndeterminateState(g2radio1, false, "Selected radio in its group");
>+  verifyIndeterminateState(g2radio2, false, "Selected radio in its group");
>+
>+  // Selecting radio buttion.
>+  g1radio1.checked = true;
>+  verifyIndeterminateState(g1radio1, false,
>+    "Selecting a radio should affect all radios in the group");
>+  verifyIndeterminateState(g1radio2, false,
>+    "Selecting a radio should affect all radios in the group");
>+  verifyIndeterminateState(g1radio3, false,
>+    "Selecting a radio should affect all radios in the group");
>+
>+  // Changing the selected radio button.
>+  g1radio3.checked = true;
>+  verifyIndeterminateState(g1radio1, false,
>+    "Selecting a radio should affect all radios in the group");
>+  verifyIndeterminateState(g1radio2, false,
>+    "Selecting a radio should affect all radios in the group");
>+  verifyIndeterminateState(g1radio3, false,
>+    "Selecting a radio should affect all radios in the group");
>+
>+  // Deselecting radio button.
>+  g2radio2.checked = false;
>+  verifyIndeterminateState(g2radio1, true,
>+    "Deselecting a radio should affect all radios in the group");
>+  verifyIndeterminateState(g2radio2, true,
>+    "Deselecting a radio should affect all radios in the group");
>+
>+  // Move a selected radio button to another group.
>+  g1radio3.name = "group2";
>+
>+  // The radios' state in the original group becomes indeterminated.
>+  verifyIndeterminateState(g1radio1, true,
>+    "Removing a radio from a group should affect all radios in the group");
>+  verifyIndeterminateState(g1radio2, true,
>+    "Removing a radio from a group should affect all radios in the group");
>+
>+  // The radios' state in the new group becomes determinated.
>+  verifyIndeterminateState(g1radio3, false,
>+    "Adding a radio from a group should affect all radios in the group");
>+  verifyIndeterminateState(g2radio1, false,
>+    "Adding a radio from a group should affect all radios in the group");
>+  verifyIndeterminateState(g2radio2, false,
>+    "Adding a radio from a group should affect all radios in the group");

You should test also what happens when element's type changes.
Or does wpt test that?
That tested, r+



>+++ b/testing/web-platform/meta/html/semantics/selectors/pseudo-classes/indeterminate.html.ini
>@@ -1,11 +1,5 @@
> [indeterminate.html]
>   type: testharness
>-  [':progress' matches <input>s radio buttons whose radio button group contains no checked input and <progress> elements without value attribute]
>-    expected: FAIL
:progress? Is that just some mistake in the comment.
Attachment #8781902 - Flags: review?(bugs) → review+
(Assignee)

Comment 26

10 months ago
(In reply to Olli Pettay [:smaug] from comment #25)
> Comment on attachment 8781902 [details] [diff] [review]
> patch, v1.
> 
> >+function test() {
> >+  // Initial State.
> >+  verifyIndeterminateState(radio1, true,
> >+    "Unchecked radio in its own group (no name attribute)");
> >+  verifyIndeterminateState(g1radio1, true, "No selected radio in its group");
> >+  verifyIndeterminateState(g1radio2, true, "No selected radio in its group");
> >+  verifyIndeterminateState(g1radio3, true, "No selected radio in its group");
> >+  verifyIndeterminateState(g2radio1, false, "Selected radio in its group");
> >+  verifyIndeterminateState(g2radio2, false, "Selected radio in its group");
> >+
> >+  // Selecting radio buttion.
> >+  g1radio1.checked = true;
> >+  verifyIndeterminateState(g1radio1, false,
> >+    "Selecting a radio should affect all radios in the group");
> >+  verifyIndeterminateState(g1radio2, false,
> >+    "Selecting a radio should affect all radios in the group");
> >+  verifyIndeterminateState(g1radio3, false,
> >+    "Selecting a radio should affect all radios in the group");
> >+
> >+  // Changing the selected radio button.
> >+  g1radio3.checked = true;
> >+  verifyIndeterminateState(g1radio1, false,
> >+    "Selecting a radio should affect all radios in the group");
> >+  verifyIndeterminateState(g1radio2, false,
> >+    "Selecting a radio should affect all radios in the group");
> >+  verifyIndeterminateState(g1radio3, false,
> >+    "Selecting a radio should affect all radios in the group");
> >+
> >+  // Deselecting radio button.
> >+  g2radio2.checked = false;
> >+  verifyIndeterminateState(g2radio1, true,
> >+    "Deselecting a radio should affect all radios in the group");
> >+  verifyIndeterminateState(g2radio2, true,
> >+    "Deselecting a radio should affect all radios in the group");
> >+
> >+  // Move a selected radio button to another group.
> >+  g1radio3.name = "group2";
> >+
> >+  // The radios' state in the original group becomes indeterminated.
> >+  verifyIndeterminateState(g1radio1, true,
> >+    "Removing a radio from a group should affect all radios in the group");
> >+  verifyIndeterminateState(g1radio2, true,
> >+    "Removing a radio from a group should affect all radios in the group");
> >+
> >+  // The radios' state in the new group becomes determinated.
> >+  verifyIndeterminateState(g1radio3, false,
> >+    "Adding a radio from a group should affect all radios in the group");
> >+  verifyIndeterminateState(g2radio1, false,
> >+    "Adding a radio from a group should affect all radios in the group");
> >+  verifyIndeterminateState(g2radio2, false,
> >+    "Adding a radio from a group should affect all radios in the group");
> 
> You should test also what happens when element's type changes.
> Or does wpt test that?
> That tested, r+

Will do.

> 
> 
> 
> >+++ b/testing/web-platform/meta/html/semantics/selectors/pseudo-classes/indeterminate.html.ini
> >@@ -1,11 +1,5 @@
> > [indeterminate.html]
> >   type: testharness
> >-  [':progress' matches <input>s radio buttons whose radio button group contains no checked input and <progress> elements without value attribute]
> >-    expected: FAIL
> :progress? Is that just some mistake in the comment.

I think the comment is kind of confusing, this wpt tests for :indeterminate for progress, checkbox and radio button, and it's just saying that progress and radio buttons are in indeterminate state.

Another thing is, I think one of the tests is wrong, that is why we still fail in that test. In [1], only 'progress1' should be left in indeterminate state. Should I file a spec bug, or should I change it directly in our tree?

[1] http://hg.mozilla.org/mozilla-central/file/24763f58772d/testing/web-platform/tests/html/semantics/selectors/pseudo-classes/indeterminate.html#l27
Flags: needinfo?(bugs)

Comment 27

10 months ago
You can fix wpt tests in our tree. They get merged to w3c testsuite rather often, and semi-automatically ( == jgraham does the merge somehow).
Flags: needinfo?(bugs)
(Assignee)

Comment 28

10 months ago
Created attachment 8784645 [details] [diff] [review]
patch, v2.

added type change test in mochitest and fixed a wpt.

Olli, re-asking your review since I changed the wpt. Thanks.
Attachment #8781902 - Attachment is obsolete: true
Attachment #8784645 - Flags: review?(bugs)

Updated

10 months ago
Attachment #8784645 - Flags: review?(bugs) → review+
(Assignee)

Comment 29

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e56e3e5e7f5
Flags: needinfo?(s)
Keywords: checkin-needed
Attachment #8443978 - Attachment is obsolete: true

Comment 30

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c266914d349a
Support :indeterminate pseudo-class for radio groups. r=smaug
Keywords: checkin-needed

Comment 31

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c266914d349a
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox51: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated compatibility at https://developer.mozilla.org/en-US/docs/Web/CSS/:indeterminate and listed at the developer release notes at https://developer.mozilla.org/en-US/Firefox/Releases/51.

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.