Remove name attribute radio inputs inside cloned nodes by dd-proxy (Fix #1663) #1666

Merged
merged 3 commits into from Mar 18, 2014

Projects

None yet

4 participants

@jbalsas
Contributor
jbalsas commented Feb 28, 2014

This is a possible fix for #1663.

When cloning a node, removing the name attribute from any inputs of type radio inside the dragged node, will prevent them from unchecking the selected radio button on its group.

I've added a test to verify this. Somehow it works properly when launched with yeti, but fails to pass when launched via yogi. Am I missing something here?

I've also created a demo with the working fix

@jbalsas
Contributor
jbalsas commented Mar 5, 2014

This is the yeti report when running the dd tests:

  Agent connected: Internet Explorer (8.0) / Windows XP 64 from XXX.XX.XX.XX
  Agent connected: Firefox (27.0) / Windows XP 64 from XXX.XX.XX.XX
  Agent connected: Chrome (23.0.1271.97) / Windows XP 64 from XXX.XX.XX.XX
✓ Testing started on Internet Explorer (8.0) / Windows XP 64, Firefox (27.0) / Windows XP 64, Chrome (23.0.1271.97) / Windows XP 64
✓ Agent completed: Chrome (23.0.1271.97) / Windows XP 64
✓ Agent completed: Firefox (27.0) / Windows XP 64
✓ Agent completed: Internet Explorer (8.0) / Windows XP 64
✓ 144 tests passed! (48 seconds)
@jbalsas
Contributor
jbalsas commented Mar 13, 2014

No takers? 😢

@okuryu
Member
okuryu commented Mar 13, 2014

Sorry for being late. I confirmed that passed tests with IE6, IE7, IE9, IE10, IE11 and Safari, but it seems to fails tests with PhantomJS as Travis CI shows.

[okuryu.local](yui3@DD-Proxy-reset-radio)$ grover tests/unit/index.html                                                                                                     
Starting Grover on 1 files with PhantomJS@1.9.7
  Running 15 concurrent tests at a time.
✖ [DragDrop]: Passed: 47 Failed: 1 Total: 48 (ignored 0) (4.204 seconds)
    test: proxy cloneNode with radio inputs
       Value should be true.
       Expected: true (boolean)
       Actual: false (boolean)
----------------------------------------------------------------
✖ [Total]: Passed: 47 Failed: 1 Total: 48 (ignored 0) (4.204 seconds)
  [Grover Execution Timer] 5.165 seconds
@jbalsas
Contributor
jbalsas commented Mar 13, 2014

Hey @okuryu, no problem!

I'm a bit puzzled about the test failing on PhantomJS... do you have any idea as to how to approach/fix this? Could we skip this given test for PhantomJS?

@okuryu
Member
okuryu commented Mar 14, 2014

hmm, I have looked but it's weird. How about putting ignore conditional with Y.UA.phantomjs?

@okuryu okuryu commented on the diff Mar 14, 2014
src/dd/tests/unit/index.html
@@ -23,6 +23,9 @@
<li>item</li>
</ul>
</div>
+ <div id="radio">
+ <input type="radio" name="test" checked></input>
@okuryu
okuryu Mar 14, 2014 Member

Maybe I think that </input> is unnecessary.

@jbalsas
jbalsas Mar 14, 2014 Contributor

I tried, but made no difference. Test still fails in PhantomJS 😢

@yahoocla

CLA is valid!

@jbalsas
Contributor
jbalsas commented Mar 14, 2014

@okuryu I've pushed a commit ignoring the test in PhantomJS. Not sure if this has triggered a new CI build, though...

@okuryu
Member
okuryu commented Mar 15, 2014

@jbalsas Thanks. Looks good to me. I'll merge this when get +1 from a reviewer or after 72 hours (excludes holidays).

@okuryu okuryu added the 3 - Review label Mar 15, 2014
@okuryu okuryu self-assigned this Mar 15, 2014
@jbalsas
Contributor
jbalsas commented Mar 15, 2014

Awesome @okuryu, thanks a lot!

@okuryu okuryu assigned apipkin and unassigned okuryu and apipkin Mar 16, 2014
@okuryu okuryu added this to the Sprint 13 milestone Mar 16, 2014
@okuryu okuryu self-assigned this Mar 16, 2014
@okuryu okuryu merged commit af77757 into yui:dev-master Mar 18, 2014

1 check passed

default The Travis CI build passed
Details
@okuryu
Member
okuryu commented Mar 18, 2014

Merged. I believe this will be available in the next release.

@okuryu okuryu removed the 3 - Review label Mar 18, 2014
@jbalsas
Contributor
jbalsas commented Mar 18, 2014

Cool!!

@okuryu Thanks so much for helping out and seeing this through!

@tripp tripp referenced this pull request Apr 9, 2014
Merged

dd proxy test fix. #1770

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