Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Checkboxradio: Clicking on an unnamed radio button must also work #7082

Closed
wants to merge 2 commits into from

Conversation

gabrielschulhof
Copy link

Original commit message: Fixed a bug that the input type="radio" button without
the name attribute is not changed to the checked status when it is clicked.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 26bda05 on 6659-hyunsook-nameless-checkboxradio into 5190a95 on master.

gabrielschulhof pushed a commit that referenced this pull request Feb 7, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 2737961 on 6659-hyunsook-nameless-checkboxradio into 4696651 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.91%) when pulling 43a96a0 on 6659-hyunsook-nameless-checkboxradio into 4696651 on master.

@gabrielschulhof
Copy link
Author

@arschmitz I would suggest that, for 1.4.1, we apply the fix as is. It is no less accurate than what we've had so far, so it shouldn't introduce any regressions.

We should construct a scenario where the radios are grouped incorrectly, and we should file a separate issue against that.

@arschmitz
Copy link
Contributor

@gabrielschulhof while you are correct that its no less wrong, I'm hesitant to change a line we know is incorrect. If it was more separate i would agree however this effects the same line. Also the fix is fairly simple

var formParentSelector = "form, :jqmData(role='page'), :jqmData(role='dialog'), body",
    formParent =  this.element.closest( formParentSelector );

return formParent.find( "input[name='" + this.element[ 0 ].name + "']" + "[type='" + this.inputtype + "']" )
    .filter(function(){
        return $( this ).closest( formParentSelector )[ 0 ] === formParent[ 0 ];
    }).add( this.element );

This makes sure that each element in the set has the same formParent as the widget does which is the correct behavior according to spec.

@arschmitz
Copy link
Contributor

@gabrielschulhof I would also argue this is all really the same issue in that the real issue is that this statement returns an incorrect array of elements. I think we should update issue #7088 to reflect this fact. The issue with this function returning elements that that should not be in the same set just reflects this is a bigger issue then originally reported.

My suggestion would be to fix this as I suggest above and add tests for different combinations where this would have previously failed.

  1. radios in body.
  2. radio in body with same name radio in page / form.
  3. radio in page / from with same name radio in form in page.
  4. radio in form with radio of same name in nested form.

@gabrielschulhof
Copy link
Author

After looking into it, I found some things:

  1. We cannot add :jqmData(role='page') or stuff like that to the scope, because there's nothing in the spec about that.
  2. Forms cannot be nested - in fact, the browser discards form tags appearing inside other form tags (but older browsers may not - not sure - need to check, so I wrote the code to grab the outermost form: formParent = thisElement.parents( "form" ).last(); ... instead of .closest()).
  3. We need to take into account the fact that, if we're inside a form that has an ID, there might be radio buttons scattered throughout the document bearing the same name and having their form attribute set to the ID of the parent form.
  4. You must only filter such that you eliminate inputs that have a different formParent from yours if your formParent is the body. Otherwise it's as simple as formParent.find( radioButtons ), because formParent must not contain any other nested forms.

We may wanna look into making use of nativeElement.form instead of ```$( nativeElement ).closest( "form, body" )

@arschmitz
Copy link
Contributor

The addition of page and dialog here is an intentional divergance from spec this was discussed and decided a long time ago when 5b8d806 was discussed. This is because the specs have no concept of pages but in out navigation system we treat each page likes its own document ( infact this is also how they are generally written even unless its a multi-page template ) so we decided page and dialog should stay. at the time body was not added because widgets outside pages were not supported yet.

@gabrielschulhof
Copy link
Author

@arschmitz Here's a potential problem with that.

gabrielschulhof pushed a commit that referenced this pull request Feb 9, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) when pulling dcbe28b on 6659-hyunsook-nameless-checkboxradio into 4696651 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling 6f3afde on 6659-hyunsook-nameless-checkboxradio into 4696651 on master.

@gabrielschulhof gabrielschulhof modified the milestones: 1.4.2, 1.4.1 Feb 13, 2014
gabrielschulhof pushed a commit that referenced this pull request Feb 17, 2014
@gabrielschulhof
Copy link
Author

So, based on our discussion I have removed the special handling of pages. Thus, the code for establishing the radio button group is now consistent with the spec.

radio = this.element[ 0 ],
name = radio.name,
form = radio.form,
doc = radio.ownerDocument,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the radio may not have an owner document in could be in a detached state

@arschmitz
Copy link
Contributor

👍

gabrielschulhof pushed a commit that referenced this pull request Feb 23, 2014
@gabrielschulhof gabrielschulhof deleted the 6659-hyunsook-nameless-checkboxradio branch February 23, 2014 22:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants