Skip to content
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

Adjust has_select filters to allow invisible options in some cases #1582

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Adjust has_select filters to allow invisible options in some cases #1582

merged 1 commit into from
Oct 6, 2015

Conversation

naw
Copy link
Contributor

@naw naw commented Oct 6, 2015

This fixes #1579

  • selected filter always allows invisible options
  • options filter allows invisible options if the select
    itself is invisible
  • with_options filter allows invisible options if the select
    itself is invisible

@twalpole
Copy link
Member

twalpole commented Oct 6, 2015

I think it needs a test for a visible select with a non-visible but selected option - otherwise looks good to me

@naw
Copy link
Contributor Author

naw commented Oct 6, 2015

My original HTML for the selected spec was actually like that, but then I noticed that selenium considers an invisible selected option as visible. That makes sense --- if you load a form with a pre-selected invisible option into Firefox or Chrome, the option is indeed visible.

Originally, I had a test to verify a visible select with an invisible option would not pass the options filter, but the selenium version of the test failed because selenium treated the invisible option as if it were visible. The RackTest and Chrome version of that test passed.

I can definitely adjust the spec per your suggestion, but in selenium I believe the spec would pass even without the code changes .... in other words, it would pass for the wrong reason(s) and not be testing the correct code path. Perhaps that's fine, as long as the other drivers do test the correct code path?

Any additional thoughts on this before I move forward?

@twalpole
Copy link
Member

twalpole commented Oct 6, 2015

We're saying that a selected option is checked against selected whether it's visible or not, so if firefox happens to ignore display: none on an option that is selected I think it's fine -- other drivers may not and the test will be verifying that the option is included in the selected filter

@naw
Copy link
Contributor Author

naw commented Oct 6, 2015

Added a commit to address your suggestion. I can squash it if it looks OK to you.

@twalpole
Copy link
Member

twalpole commented Oct 6, 2015

looks good -- go ahead and squash and I'll merge it in

- selected filter always allows invisible options

- options filter allows invisible options if the select
  itself is invisible

- with_options filter allows invisible options if hte select
  itself is invisible
@naw
Copy link
Contributor Author

naw commented Oct 6, 2015

Squashed. Thanks for your quick attention to this.

twalpole added a commit that referenced this pull request Oct 6, 2015
Adjust has_select filters to allow invisible options in some cases
@twalpole twalpole merged commit 65ec2d5 into teamcapybara:master Oct 6, 2015
@twalpole
Copy link
Member

twalpole commented Oct 6, 2015

thanks for the work!

@naw naw deleted the has-select-filters branch October 7, 2015 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

have_select with filters does not respect :visible option
3 participants