Skip to content

Only uncheck radios in same form#1007

Closed
zpao wants to merge 2 commits into
jsdom:masterfrom
zpao:checked-radios-forms
Closed

Only uncheck radios in same form#1007
zpao wants to merge 2 commits into
jsdom:masterfrom
zpao:checked-radios-forms

Conversation

@zpao

@zpao zpao commented Jan 21, 2015

Copy link
Copy Markdown
Contributor

When selecting another radio element, only other radios in the same form should be unchecked.

See http://jsbin.com/daqoge/1/ as an example. This is taken pretty much directly from a test we have in React where we're ensuring we maintain browser behavior. In order to make it pass in jest we actually "fix" this in our (ancient) jsdom env by overridding the setter for HTMLInputElement.checked (https://github.com/facebook/jest/blob/master/src/lib/jsdom-compat.js#L245-L284).

I think this is the right fix but there's a lot of code I didn't read. I also didn't write a test - I'll look into that process but if anybody more familiar wants to jump in and help, that would be appreciated.

When selecting another radio element, only other radios in the same form
should be unchecked.
@domenic

domenic commented Jan 21, 2015

Copy link
Copy Markdown
Member

I am almost certain this was fixed in jsdom 1.4.0. As such I'd appreciate a failing test to show that this is still an issue. /cc @Joris-van-der-Wel.

@domenic

domenic commented Jan 21, 2015

Copy link
Copy Markdown
Member

Actually it might have been fixed in 1.0.0-pre.6.

@zpao

zpao commented Jan 21, 2015

Copy link
Copy Markdown
Contributor Author

https://github.com/tmpvar/jsdom/blob/master/test/level2/html.js#L19814-L19833 would lead me to believe you're correct. However that's just testing when you have 2 sibling forms. If I copy & modify that test to match the jsbin, then it fails (will add that to PR in a moment)

@zpao

zpao commented Jan 21, 2015

Copy link
Copy Markdown
Contributor Author

Note: I did confirm that the new test fails in master without the rest of my patch.

@domenic

domenic commented Jan 21, 2015

Copy link
Copy Markdown
Member

Sweet!! We'd better merge this then :D.

@Joris-van-der-Wel

Copy link
Copy Markdown
Member

Here are a few quotes from WHATWG

radio button group definition

The radio button group that contains an input element a also contains all the other input elements b that fulfill all of the following conditions:

  • The input element b's type attribute is in the Radio Button state.
  • Either a and b have the same form owner, or they both have no form owner.
  • Both a and b are in the same home subtree.
  • They both have a name attribute, their name attributes are not empty, and the value of a's name attribute is a compatibility caseless match for the value of b's name attribute.

A document must not contain an input element whose radio button group contains only that element.

html.js:1036 loops over only the descendants of the radio button group (or at least, that was my intention :) ). But it does not strictly follow the spec with nested forms.
So a check for the form owner should be added. The form owner can be changed by setting the IDL. Read here: https://html.spec.whatwg.org/multipage/forms.html#form-owner

@domenic

domenic commented Jan 21, 2015

Copy link
Copy Markdown
Member

Well it would be lovely if you wanted to make it follow the spec exactly. Let me know if you're up for that and if so I'll wait on merging.

@Joris-van-der-Wel

Copy link
Copy Markdown
Member

Sure, but probably not before this weekend.

It looks pretty involved though, iirc jsdom does not implement setting the form attribute. Following the spec strictly would require tracking form owners explicitly (instead of looping over parentNode).

@domenic

domenic commented Jan 21, 2015

Copy link
Copy Markdown
Member

Oh I see, yeah that sounds like a pain. Not worth tying up this PR for.

@domenic

domenic commented Jan 21, 2015

Copy link
Copy Markdown
Member

Merged as 3a75559, with 3.0.2 going out shortly!

@domenic domenic closed this Jan 21, 2015
@zpao

zpao commented Jan 21, 2015

Copy link
Copy Markdown
Contributor Author

Awesome turnaround, thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants