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

[breaking change] unobtrusive datepicker interaction #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jgigault
Copy link

@jgigault jgigault commented Jan 6, 2021

Dear maintainer,

In our project we are using Capybara Selenium with Chrome browser.
We have encountered issues with obtrusive interactions in conflicts with external factors such as dialogs.

In our opinion, there are two acceptable/required changes in this gem:

  1. Stop to check result at the end of select_bootstrap_date because it's not the role of the helper
  2. Stop to click on //body at the end of select_date and rely on autoclose

Those changes will certainly break some test suites of current users, but it increases the compatibility of your gem.

Please give me some feedbacks!

current behavior

Using Chrome, dialogs must be caught explicitely before making additional interaction with DOM:

  1) Bootstrap Datepicker with chrome_headless Boostrap 4.4 behaves like a datepicker alert/confirm callback fills in a datepicker while passing alert/confirm
     Failure/Error: fail if Date.parse(date_input.value) != value

     Selenium::WebDriver::Error::UnexpectedAlertOpenError:
       unexpected alert open: {Alert text : Date has changed}
         (Session info: chrome=87.0.4280.88)
     Shared Example Group: "a datepicker" called from ./spec/features/bootstrap_datepicker_spec.rb:87
     # 0   chromedriver                        0x0000000102901d79 chromedriver + 2510201
     # 1   chromedriver                        0x0000000102f6e743 chromedriver + 9246531
     # 2   chromedriver                        0x000000010273c623 chromedriver + 652835
     # 3   chromedriver                        0x00000001026edebb chromedriver + 331451
     # 4   chromedriver                        0x00000001026dee23 chromedriver + 269859
     # 5   chromedriver                        0x00000001026b800a chromedriver + 110602
     # 6   chromedriver                        0x00000001026b8fc7 chromedriver + 114631
     # 7   chromedriver                        0x00000001028d3429 chromedriver + 2319401
     # 8   chromedriver                        0x00000001028df53f chromedriver + 2368831
     # 9   chromedriver                        0x00000001028df150 chromedriver + 2367824
     # 10  chromedriver                        0x00000001028ba23b chromedriver + 2216507
     # 11  chromedriver                        0x00000001028dfa91 chromedriver + 2370193
     # 12  chromedriver                        0x00000001028c84f9 chromedriver + 2274553
     # 13  chromedriver                        0x00000001028f5f89 chromedriver + 2461577
     # 14  chromedriver                        0x00000001029073b3 chromedriver + 2532275
     # 15  libsystem_pthread.dylib             0x00007fff6a11a109 _pthread_start + 148
     # 16  libsystem_pthread.dylib             0x00007fff6a115b8b thread_start + 15
     # ./lib/capybara-bootstrap-datepicker.rb:53:in `select_bootstrap_date'
     # ./lib/capybara-bootstrap-datepicker.rb:21:in `select_date'
     # ./spec/features/bootstrap_datepicker_spec.rb:51:in `block (3 levels) in <top (required)>'

Also, when using autoclose: true, the additional click on //body made at the end of select_date is useless.

expected behavior

We expect helper to be less obtrusive and more explicit:

# click outside of input should be performed outside of "select_date", only if required
select_date Date.today, from: 'Label of date input with dialog callback', datepicker: :simple
first(:xpath, '//body').click
# "select_date" should be usable in this form with dialogs handling
accept_alert do
  select_date Date.today, from: 'Label of date input with dialog callback', datepicker: :bootstrap
end

@@ -28,4 +28,6 @@ Gem::Specification.new do |gem|
gem.add_development_dependency 'phantomjs', '~> 2.1', '>= 2.1.1.0'
gem.add_development_dependency 'poltergeist', '~> 1.18', '>= 1.18.1'
gem.add_development_dependency 'rspec', '~> 3.9', '>= 3.9.0'
gem.add_development_dependency 'webdrivers'
gem.add_development_dependency 'selenium-webdriver'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not used to update gemspec file, so that I have not marked versions.
I would require some help for that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax is the same as in a Gemfile. The site rubygems.org provides an example of the correct syntax based on the latest version available on the detail page of each gem. For example :

gem 'webdrivers', '~> 4.6'

Just convert it to the Gemspec syntax:

gem.add_development_dependency 'webdrivers' , '~> 4.6'

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it seems that selenium-webdriver is a dependency of webdrivers, so you may not need to add it explicitly.

- remove validation ('fail' statement) at the end of `#select_bootstrap_date`
- remove additional click on '//body' at the end of `#select_date`
- extend test suite covering with "selenium_chrome_headless"
@jgigault jgigault force-pushed the feature/unobstrusive-datepicker-interaction branch from e7d954a to 918ad43 Compare January 6, 2021 08:13
@jgigault
Copy link
Author

jgigault commented Jan 6, 2021

Click on "//body" could be optional, with new option "skip_blur" for instance

@akarzim
Copy link
Owner

akarzim commented May 22, 2021

Great catch!

You are doubly right! Explicit behaviour is better. And I agree about autoclose: true, it's a cleaner approach than simulating a click outside the datepicker.

Would you consider adding autoclose: true to the HTML example files and a note in the readme about what to do if its value is false? That way we wouldn't have to trigger a click on the page body in the tests. Also, please provide an example of accept_alert in the readme as you did in this PR description 🙏.

I'll really be glad to merge this PR! Thanks a lot.

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

Successfully merging this pull request may close these issues.

None yet

2 participants