-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Allow option values containing quotation marks to be selected #29214
fix: Allow option values containing quotation marks to be selected #29214
Conversation
|
04f6331
to
c52bd77
Compare
@davidfurey Could you add a changelog entry to this PR? Thanks! https://github.com/cypress-io/cypress/blob/develop/guides/writing-the-cypress-changelog.md |
c52bd77
to
9331828
Compare
Does this belong in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidfurey For the changelog, this should be added to the cli/changelog.md
file, not the file within the main changelog. (This isn't used anymore).
I verified that this fixes the previously failing test. Thanks for the contribution!
Before
After
38530d1
to
db8f4e7
Compare
…es containing quotation marks can be selected
db8f4e7
to
ed91b9d
Compare
Thanks for the speedy review @jennifer-shehane I've updated the changelog |
@@ -128,7 +128,7 @@ export default (Commands, Cypress, cy) => { | |||
values.push(value) | |||
|
|||
// https://github.com/cypress-io/cypress/issues/24739 | |||
if (options.$el.find(`option[value="${value}"]`).length > 1) { | |||
if (options.$el.find(`option[value="${value.replace(/"/g, '\\\"')}"]`).length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidfurey Only if you have the spare cycles - but I'm curious as a RegEx noob what this logic actually does and how it resolves the bug. Would appreciate an explanation (but again, only if you have time). Thank you!
Going to merge with these chrome beta tests failing, this is a known issue we're investigating here: #29199 |
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
I believe this issue was introduced by #25016
PR Tasks
cypress-documentation
?type definitions
?