FIxed has_content? whitespace comparison #521

Merged
merged 3 commits into from Nov 15, 2011

Projects

None yet

5 participants

@rmontgomery429

has_content? should ignore any whitespace in the content being compared (the matcher text) so that comparing whitespace sensitive text i.e. content in a /pre/ tag will be matched properly.

@rmontgomery429 rmontgomery429 Fixed has_content?
* has_content? should ignore any whitespace in the content being compared (the matcher text) so that comparing whitespace sensitive text i.e. content in a <pre> tag will be matched properly.
48ede68
@joliss
Collaborator
joliss commented Oct 14, 2011

Wouldn't it be better to use something like /\s/ instead of "\n"?

@rmontgomery429

@joliss that change made more tests fail. The important thing would be to match however the XPath::HTML.content is ignoring whitespace. It looks like content is using an xpath contains which may be where whitespace is being ignored. I didn't dig much further but I believe that would be handled by Nokogiri and in turn LibXML. I'm not 100% sure though.

@jnicklas
Collaborator

I agree with Jo that if we do this, the regexp should be something like /\s/m or whatever that modifier is that switches to multiline mode.

@joliss
Collaborator
joliss commented Oct 16, 2011

the regexp should be something like /\s/m or whatever that modifier is that switches to multiline mode.

The m modifier only matters for /./ (so that the dot matches newlines), not for /\s/, so far as I know.

Jo,
still recovering from the shock that the MULTILINE option is written /m in Ruby, not /s like the rest of the world

@mkdynamic

I think this change makes sense, since in my experience you basically never care about whitespace when using content matchers. The only downside to this is that the one time you do care about it, you're screwed.

Re: the regex issue, in this case, the /m option makes no difference. It only affects the behaviour of ^ and $:

>> "apple\norange".gsub(/\n/, '')
=> "appleorange"
>> "apple\norange".gsub(/\n/m, '')
=> "appleorange"

We could use a regex for consistency, but it doesn't actually make any difference in this case either.

@rmontgomery429

It turns out that XPath::HTML normalizes content through xpath function normalize-whitespace. This doesn't behave exactly the same as gsub(/\s/m, ""). The normalize-whitespace function works by "stripping leading and trailing whitespace and replacing sequences of whitespace characters by a single space". http://www.w3.org/TR/xpath/#function-normalize-space

What I ended up doing to make this behave more like the normalize-whitespace function was to replace any one or more matched whitespace character with a single space.

I think the /\s/ suggestion was a good one and I've just pushed a change to make it work better in all cases.

Thanks for the feedback @joliss and @jnicklas.

@mkdynamic

We probably want to normalize whitespace vs. just removing newlines, like so:

>> " apple\n   orange  ".gsub(/\s+/, ' ').strip
=> "apple orange"

This is symmetrical with how the XPath normalize-space function works.

@rmontgomery429

@mkdynamic that makes sense. I must have pushed just as you commented. :) The latest change (removing the /m modifier) if pushed.

@rmontgomery429

@mkdynamic do you know if it is preferable within capybara to make something like gsub(/\s/, " ").strip a call to a function like normalize_whitespace(content) or leave it in-line?

@rmontgomery429 rmontgomery429 Fixed has_content?
* use regex 'any one or more whitespace character' instead of simply removing all newline characters as suggested by @jnicklas and @joliss [#521]
a74e821
@mkdynamic

Since you'll need to call this from both has_content? and has_no_content? I would move it into it's own (probably private) method.

@rmontgomery429 rmontgomery429 Fixing has_content?
* moved the gsub and strip into a function named normalize_whitespace
* added the call to has_no_content?
1a55377
@elabs elabs merged commit 1a55377 into teamcapybara:master Nov 15, 2011
@jnicklas
Collaborator

Good job!

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