1100535 - HTML 5 Required Validation on radio button does not clear when reset (33.1)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jason Finch]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141106120505

Steps to reproduce:

Firefox 33.1/Windows, when using Angular 1.2.26 to enable/disable 'Required' attribute on a radio button set, the radio buttons will change their state to required and show the HTML5 validation as expected.  But then if they are toggled/alterted to remove 'required' the state still retains the 'required' functionality.

In Chrome/Win, this issue is not experienced using the same plnkr example.

Ref:
https://github.com/angular/angular.js/issues/10089#issuecomment-63296014

Repro here: http://plnkr.co/edit/tBlM5KXb5DLP1Hb8ta8U?p=preview



Actual results:

Radio buttons continue to act as if 'required' is still active by showing the HTML5 validation warning popups.


Expected results:

When the radio buttons 'required' is toggled off the HTML5 validation should not continue to engage.

Comment 1

[:Gijs (out for now; he/him)]

:agi, might you know what's going on here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

This needs a somewhat minimized testcase.  A basic testcase like this:

<style>
  input:invalid { outline: 5px solid red; }
</style>
<form>
  <input type="radio" required name="a">
  <input type="radio" required name="a">
</form>
<script>
  document.writeln(document.forms[0].checkValidity());
  var inputs = document.querySelectorAll("input");
  Array.prototype.forEach.call(inputs, function(input) { input.required = false; });
  document.writeln(document.forms[0].checkValidity());  
</script>

works fine....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, this is exciting.  We get JS code that does this:

      element[name] = value;

where name == "required" and value = true.  This calls SetRequired, which internally calls setAttribute("required", "").

Then we get more JS code doing this:

          element[name] = true;
          element.setAttribute(name, lowercasedName);

(why, who knows).  That first set is a no-op, but the second changes the attr value from "" to "required".

This triggers a bug in HTMLInputElement::AfterSetAttr where we do:

1408     if (mType == NS_FORM_INPUT_RADIO && aName == nsGkAtoms::required) {
1409       nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();
1410 
1411       if (container) {
1412         nsAutoString name;
1413         GetAttr(kNameSpaceID_None, nsGkAtoms::name, name);
1414         container->RadioRequiredChanged(name, this);
1415       }
1416     }

and RadioRequiredChanged actually assumes that the state changed.  But in this case the state did NOT change, since we went from one truthy value to another.  So we end up thinking we have 4 required radios in the group, then remove two of them when required is toggled to false, but still think we have two required radios in there.

Olli, do you have time to look into this one?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimalish testcase

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Do not assume that the radio required status changed when the attribute changes.

(sorry for the delay, my dev pc died a few days ago)

Maybe we can move the RadioRequiredChanged to BeforeSetAttr and only call it if the value actually changed?

Comment 6

[Boris Zbarsky [:bzbarsky]]

RadioRequiredChanged unfortunately examines the value of the attr to figure out what it changed to.  Though we could change that and pass in the value, I guess.  Worth trying.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

comment 5 is probably the simplest approach (+ adding needed params to RadioRequiredChanged).

agi, want to look at this?

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

Yup. I'll post a patch probably tonight.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Do not assume that the radio required status changed when the attribute changes.

This should do. Thanks bz for the investigation & the testcase! :-)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8529431 [details] [diff] [review]
Do not assume that the radio required status changed when the attribute changes.

Maybe s/aNewValue/aRequiredAdded/ ?

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Do not assume that the radio required status changed when the attribute changes.

> Maybe s/aNewValue/aRequiredAdded/ ?

Done! Thanks Olli!

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=459359866650
https://tbpl.mozilla.org/?tree=Try&rev=459359866650

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af03bce1c07f

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/af03bce1c07f