Fix an incorrect autocorrect for Capybara/RedundantWithinFind when escape required css selector
#137
Conversation
…escape required css selector Fix: #136 Previously, the following automatic corrections were made. before: ```ruby within find_by_id("array-form-session.dates") do expect(page).to have_text(:visible, "YYYY/MM/DD") end ``` after: ```ruby within "#array-form-session.dates" do expect(page).to have_text(:visible, "YYYY/MM/DD") end ``` This is `.' in find_by_id. ` has the same meaning as the escaped id. In other words, when replacing within, the `. ` must be escaped. This PR has been modified so that escaping is correctly done in selectors that require escaping. This will prevent the behavior from changing before and after the automatic correction.
|
@hatsu38 Does this change solve your issue? |
|
@ydah It seems that RuboCop now properly escapes the within find_by_id("array-form-session.dates") do ~ end-> match within "array-form-session\.dates" do ~ end-> match both So, As mentioned in the ISSUE, the behavior changes as a result of running rubocop -A. Therefore, I believe my issue has not yet been resolved. |
|
For the context: https://stackoverflow.com/questions/12310090/css-selector-with-period-in-id @hatsu38 does the second method, I suggest opening an issue against capybara, as if the backslash quote doesn’t work, it’s in their code. |
| if node.method?(:find_by_id) | ||
| replaced.sub(/\A(["'])/, '\1#') | ||
| node.first_argument.source.match(/\A(['"])(.*)['"]\z/) do |
There was a problem hiding this comment.
This won’t work with any other form of string, like triple-quoted, those weird %{} etc ones, won’t work with multi-line?
I suggest creating an issue to keep track of this
There was a problem hiding this comment.
Can we use something like string_value, does rubocop-ast provide this?
There was a problem hiding this comment.
"[id='
boom']"
Doesn’t it?
| if node.method?(:find_by_id) | ||
| replaced.sub(/\A(["'])/, '\1#') | ||
| node.first_argument.source.match(/\A(['"])(.*)['"]\z/) do |
There was a problem hiding this comment.
Can we use something like string_value, does rubocop-ast provide this?
| end | ||
|
|
||
| context 'when selector contains NULL character' do | ||
| let(:selector) { "abc\0def" } |
| context 'when selector is a single hyphen' do | ||
| let(:selector) { '-' } | ||
|
|
||
| it 'escapes the hyphen character' do |
There was a problem hiding this comment.
Why? Does it have a special meaning as a CSS selector?
| context 'when selector contains control characters' do | ||
| let(:selector) { "abc\x01\x1F\x7Fdef" } | ||
|
|
||
| it 'escapes control characters as hexadecimal with a trailing space' do |
| end | ||
| end | ||
|
|
||
| context 'when selector starts with a digit' do |
There was a problem hiding this comment.
Is this possible? Why would anyone do this?
| end | ||
| end | ||
|
|
||
| context 'when selector starts with a hyphen followed by a digit' do |
There was a problem hiding this comment.
In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit.
This seems to e an invalid CSS selector case, why should we handle it?
| if node.method?(:find_by_id) | ||
| replaced.sub(/\A(["'])/, '\1#') | ||
| node.first_argument.source.match(/\A(['"])(.*)['"]\z/) do |
There was a problem hiding this comment.
"[id='
boom']"
Doesn’t it?
| # css_escape('some-id.with-priod') # => some-id\.with-priod | ||
| # @reference | ||
| # https://github.com/mathiasbynens/CSS.escape/blob/master/css.escape.js | ||
| def css_escape(selector) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity,Metrics/MethodLength,Metrics/PerceivedComplexity |
There was a problem hiding this comment.
How do you feel to only add handling for the period in this PR, and leaving the rest for later?
|
Thanks for your comment! I tried it again. Originally, I was using And, I found on Stack Overflow that |
Fix: #136
Previously, the following automatic corrections were made.
before:
after:
This is
.infind_by_idhas the same meaning as the escaped id. In other words, when replacing within, the.must be escaped.This PR has been modified so that escaping is correctly done in selectors that require escaping. This will prevent the behavior from changing before and after the automatic correction.
Before submitting the PR make sure the following are checked:
main(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).