Escape quotes in value for radio matcher #309
Conversation
|
|
||
| <!--script type="text/javascript" src="http://cdnjs.cloudflare.com/ajax/libs/zepto/1.1.1/zepto.min.js"></script> | ||
| <script type="text/javascript" src="vendor/zepto-data.js"></script--> | ||
| <script type="text/javascript" src="vendor/jquery.js"></script> | ||
| <script type="text/javascript" src="vendor/qunit.js"></script> | ||
| <script type="text/javascript" src="https://code.jquery.com/jquery-1.12.4.min.js"></script> |
There was a problem hiding this comment.
Can you please keep this as the vendored code so we don't need the internet to run the tests?
| <script type="text/javascript" src="vendor/jquery.js"></script> | ||
| <script type="text/javascript" src="vendor/qunit.js"></script> | ||
| <script type="text/javascript" src="https://code.jquery.com/jquery-1.12.4.min.js"></script> | ||
| <script type="text/javascript" src="vendor/qunit-2.0.1.js"></script> |
There was a problem hiding this comment.
Please split this change into another PR. And replace the file in place instead of creating a new one?
There was a problem hiding this comment.
Done. See #311.
| @@ -449,7 +449,7 @@ | |||
| selector: 'input[type="radio"]', | |||
| events: ['change'], | |||
| update: function($el, val) { | |||
| $el.filter('[value="'+val+'"]').prop('checked', true); | |||
| $el.filter('[value="'+val.toString().replace(/"/g, '\\"')+'"]').prop('checked', true); | |||
There was a problem hiding this comment.
Please pull this variable out to the line above. Perhaps with a comment explaining what it is for.
jeremywadsack
force-pushed
the
quote_radio_value
branch
from
e5b399a to
3a3fb68
Compare
|
@akre54 Fixes done. Rebased, squashed, and pushed. Let me know if there are any other changes that I should look at. Thanks for the quick response on this! |
| // Because we use " in the value descriptor, we need to escape any | ||
| // quotes in the value itself otherwise jQuery/Sizzle complain that | ||
| // the selector is invalid. | ||
| escapedVal = val.toString().replace(/"/g, '\\"') |
There was a problem hiding this comment.
Needs a var before escapedVal
There was a problem hiding this comment.
Ack, good catch. Too much coffee script lately.
Fixed, sqaushed, pushed.
Under jQuery 1.12, the provided expression for matching a radio button when the value containers a quote throws a Syntax error, unrecognized expression. This resolves that by properly escaping quote characters in the value before building the matching expression.
jeremywadsack
force-pushed
the
quote_radio_value
branch
from
3a3fb68 to
a5beff3
Compare
Under jQuery 1.12, the provided expression for matching a radio button when the value contains a quote throws a "Syntax error, unrecognized expression".
This resolves that by properly escaping quote characters in the value before building the matching expression.
Note that in order to prove this and build a failing test I fixed up
test/index.htmlto pull inqunitfrom thevendorfolder (it was missing). This is because the phantomjs runner is broken right now in thenpm/karmatests. (There's some discussion of this under #307.)Then I found that this doesn't fail in the bundled jQuery (1.8.3), but does fail with 1.12.x that we are using. I updated the reference in
test/index.htmlto use the version from the jQuery CDN just to make the test show that the code is fixed. I don't know if you want to update the vendored jQuery to the latest from the 1.x branch, but I'm happy to make that change as well.