Add API for operation with modal (simple) dialogs

[mikepack]

Add an official notification API for handling alerts, confirms and prompts. See README for API usage.

The default naming is singular because Capybara ships with a Selenium driver which can only deal with a single alert/confirm/prompt at a time. While other drivers can support multiple notifications, I figured it would be best to stay consistent with Capybara core. I did, however, alias the methods in their plural form so that they would read better when dealing with multiple notifications.

[jnicklas]

Wow this is very impressive. I'm intent on pushing Capybara 2.1 RC1 very soon, so this will not make it into Capybara 2.1, as it will take some further deliberation before we can get this merged. Just wanted to point that out up front.

A few things I noticed:

1. We metaprogram in all other methods in the session, why not do the same with NOTIFICATION_METHODS?
2. This API should be opt-in for driver authors, so it should have it's own :requires metadata, so that driver authors can skip it if they desire.
3. I find the utility of confirming multiple prompts/alerts/confirms dubious, especially since we can't mix and match with the suggested API. There is no way to handle both a prompt and a following alert, so why bother with this at all?
4. The surfact area towards drivers is very large, do we really have to add that many methods?
5. I'm concerned about synchronization. Capybara should not leave that up to the driver. If for example the alert appears due to an async process 50ms later than the action that was supposed to trigger it, then Capybara should be able to handle that, and that should work without the driver authors having to do anything special, just as it does with elements.

Finally, let's get the driver authors involved in this discussion, @jferris, @jonleighton, @mhoran, @jcoglan, any comments on this?

[mikepack]

Awesome feedback. I considered this a starting point for discussion more than a complete solution anyway. I'll make a couple additional commits based on this discussion and squash before merging.

1. We metaprogram in all other methods in the session, why not do the same with NOTIFICATION_METHODS?

I'm not entirely sure what you mean here. Can you elaborate?

2. This API should be opt-in for driver authors

I'm assuming you are referring to the the metadata for specs. How does :requires => [:notifications] sound?

3. I find the utility of confirming multiple prompts/alerts/confirms dubious

I'm in agreement here, and it would improve consistency across drivers. I'd be in favor of supporting only one response. However, it would be impossible to test some cases, for example: when a single user interaction produces multiple prompts, maybe for a wizard-like experience.

4. The surfact area towards drivers is very large, do we really have to add that many methods?

I've thought through this a bit, and think I can reduce the driver API down to either one or two additional methods, depending on preference.

Option one:

def respond_to_notification(type, accept, *responses, &blk)
  # ...
end

Arguments:
type: :alert, :confirm, or :prompt
accept: true or false
responses: an array of optional responses
bkl: the code that produces a notification

Option two:

def accept_notification(type, *responses, &blk)
  # ...
end

def reject_notification(type, &blk)
  # ...
end

The motivation behind option two is to eliminate the boolean argument and the responses which are never applicable when rejecting a notification.

5. I'm concerned about synchronization.

I'll play with this a bit.

[jnicklas]

1. I'm talking about something like this: https://github.com/jnicklas/capybara/blob/master/lib/capybara/session.rb#L351-L356
2. Exactly, sounds perfect!
3. I think this use case is sufficiently rare that we can ignore it for now.
4. I like the second option

[mikepack]

Alright, I've made some updates. A few comments:

Previously, it seemed like it might make sense to metaprogram the notification methods because there was a strong parity between the session and driver method names. Now that the driver methods have changed, it doesn't seem to be as applicable. I'd be interested to hear I'm wrong, however.

I've added a couple async specs, though I'm not entirely sure it's covering all bases. I would expect this spec to pass because of the synchronization around #have_xpath. However, I expected this spec to fail because the link would be clicked and the browser session would terminate, leaving #alert_messages empty. It seems that Selenium is blocking on the link click and doing it's own synchronization. Feels like I'm missing something...

[twalpole]

Whats the point in adding alert_messages, confirm_messages, and prompt_messages to the session. Each call to accept/reject_notification returns the text for the box confirmed/rejected so when would a tester ever want to check that later rather than when accepting/rejecting the box, and if they would ever want to is it common enough to warrant 3 new methods on the session?

[mikepack]

@twalpole Good point, I partially agree. Yes, accept/reject_notification returns the text, but readability would suffer if we removed the message accessors, in my opinion. Specifically, because the text is assigned above the line where the alert is clicked. Granted, this is also the case for accepting/rejecting. The name #accept_alert doesn't lend itself to returning the text, either.

message = accept_alert do
  click_link "Show alert"
end
message.should == "Alert text"

accept_alert do
  click_link "Show alert"
end
alert_messages.should include("Alert text")

All that said, I would be in favor of removing the message accessors.

[twalpole]

@mikepack - btw the reason the test you think should fail doesn't is that the javascript code used by selenium for the firefox driver (havent checked others) has an alertTimeout which it will wait for a modal dialog to show https://github.com/SeleniumHQ/selenium/blob/master/javascript/firefox-driver/js/firefoxDriver.js . Its set to 2 seconds by default, so you'll need to figure out if its possible to shorten that timeout in some cross browser fashion or extend your delay on the alert - and then the test will fail with a Selenium::WebDriver::Error::NoAlertPresentError

[twalpole]

@mikepack I dont think having a var for the return really hurts readability much, and I think its better than the accessors where values "magically" appear but not until you've accepted or rejected the alert. e.g. at a quick glance I might assume that alert_messages would include the message for the alert currently on the screen, but it doesnt until I call accept or reject

[mikepack]

@twalpole Thanks for the tip, I'll mess with the timeouts.

I could see the potential confusion around alert_messages; this is how capybara-webkit surfaces the message, though I don't know the general sentiment towards it.

Having alert_messages could also cause confusion if used in conjunction with manual management of the notification. The alert must be wrapped with accept/reject to populate any messages. Eg:

click_link "Show alert"
page.driver.browser.switch_to.alert.accept
alert_messages #=> You might expect this to contain the message, but it's empty

[abotalov]

What about?

expect { click_link "Show alert" }.to open_alert("Alert text")

[jnicklas]

This is shaping up quite nicely, @mikepack.

I think I agree with @twalpole, the alert_messages style accessors are confusing, and I think it's better to skip them. I think the most common use case is not to care about the message of the alert at all. One thing that would be useful, imo, is something like this:

accept_confirm "Are you sure?" do
  click_link "Delete"
end

I think that gives a more elegant API to the most common reason one might want to look at alert/confirm messages in the first place, to make sure that we're confirming the right one. What do you think?

@abotalov that's too RSpec specific, imo. We might want to add matchers around that particular API, but honestly I don't really see the point.

[jnicklas]

@mikepack also, can you shorten the README section a bit. It's very longwinded, imo.

[abotalov]

accept_confirm "Are you sure?" do
  click_link "Delete"
end

It's short but I think it's not too good that row "Are you sure?" is before click_link "Delete" which doesn't conform to real sequence of events.

Variant that I proposed conforms to real sequence of events.

But it's more about personal preference

[jnicklas]

@abotalov well, accept_confirm doesn't conform to the sequence of events either, but that's just a sacrifice we have to make to allow wider support in drivers. I'd rather that accept_confirm didn't take a block, and always appeared after the action that triggered the alert to appear. I.e. this:

click_link "Delete"
accept_confirm "Are you sure?"

Unfortunately, it seems like that's not technically possible in some drivers, from what I understand.

[jferris]

@jnicklas the accept_confirm after the fact approach would definitely be harder in capybara-webkit, but if it's agreed that it would be a much better API, I could do some spiking to see if it's possible.

[mikepack]

I've updated the README to be more succinct and removed the message accessors.

@jnicklas while passing the expected message to accept/reject, if the actual message presented does not match the expectation, would you assume this throws an error? If so, the implicit expectation here is a little unclear.

@jferris thanks a ton. I'm interested in your results.

[jnicklas]

@jferris if it's not too much bother, that would be excellent. I would definitely prefer an API without blocks if it's possible. I think Selenium supports that nicely.

@mikepack I would expect accept_confirm to raise an error if there is no confirm box to accept, so if passed an argument, it should raise an error if the text does not match.

[twalpole]

@jnicklas Just to confirm

I would expect accept_confirm to raise an error if there is no confirm box to accept, so if passed an argument, it should raise an error if the text does not match.

Those errors would only be raised after the appropriate wait time, using synchronize, correct?

[twalpole]

@mikepack it would be great if these methods could accept a regex or a string to match against the text in the alert

[twalpole]

@mikepack The current PR has methods for dealing with alerts, confirms, and prompts in the session with these all calling accept/reject_notification, but not passing pass a type. I know selenium doesnt differentiate between the 3 different types but are you sure the other drivers dont? I notice your "Option Two" from above did have a type parameter.

[mikepack]

@twalpole Good catch! Updated.

[abotalov]

Do you think it would be better to put added specs to a subfolder of spec/sessionto reduce a number of files?

[twalpole]

if I was writing those specs I think I'd put them all in one file, notifications_spec or modal_spec or something since they are basically all the same thing. @jnicklas what are your feelings on that?

[mikepack]

@abotalov @twalpole The reason I put them into separate files is to keep consistency with all the rest of the DSL methods: all, click_link, execute_script, etc as all_spec.rb, click_link_spec.rb, execute_script_spec.rb, etc

[twalpole]

@mikepack I get completely why you created the multiple files, and it does fit with the way everything else is setup (other than they should be accept_alert_spec.rb instead of accept_alerts_spec.rb etc)

[mikepack]

@twalpole Another great catch. Updated.

[jnicklas]

I'd personally be cool with having just one file, even though it is slightly inconsistent, but I don't really mind either way.

[abotalov]

I'd want to propose to put most of specs into different folders:

session/finders
session/actions
session/matchers
session/notifications

[mikepack]

@jnicklas @abotalov I'd be in favor of subdirectories, but this is a little off topic for this PR 😄. Spending some time today in capybara-webkit

[jferris]

@mikepack I did some research today and added notes to thoughtbot/capybara-webkit#506.

[abotalov]

I see the following issues in this pull request:

1. Those things are called modal dialogs (http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#simple-dialogs). Notifications are different things - http://notifications.spec.whatwg.org/
2. Webdriver is in the process of W3C standartization (https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html) and therefore it's better to stick with its terminology and use dismiss instead of reject. Also after it will become W3C standard there won't be sense for Capybara to wait for a particular feature to be implemented in Capybara-Webkit/Poltergeist/whatever. Following W3C standard is enough 😏. Also Webdriver has phantomjs driver and it doesn't seem to be worse than poltergeist.

The following API usage seems good enough to me:

accept_alert
accept_confirm "Are you sure?"
dismiss_confirm
accept_prompt "What is your name?", with: 'Andrey Botalov'

[mhoran]

On Sun, Mar 09, 2014 at 07:19:08AM -0700, Andrey Botalov wrote:

I see the following issues in this pull request:

1. Those things are called modal dialogs (http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#simple-dialogs). Notifications are different things - http://notifications.spec.whatwg.org/
2. Webdriver is in the process of W3C standartization (https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html) and therefore it's better to stick with its terminology and use dismiss instead of reject. Also after it will become W3C standard there won't be sense for Capybara to wait for a particular feature to be implemented in Capybara-Webkit/Poltergeist/whatever. Following W3C standard is enough 😏. Also Webdriver has phantomjs driver and it doesn't seem to be worse than poltergeist.

While I agree implementing the standard makes sense, leaving existing
users of Poltergeist or capybara-webkit out in the cold is not a great
option. Each of these testing tools, unfortunately, has their own set of
bugs which users have crafted test suites around, and they cannot be
used interchangeably without effort.

I believe someone had looked into implementing this API for
capybara-webkit. I'm not sure where Poltergeist is regarding the API.
However, if it's possible with Poltergeist then it's likely possible in
capybara-webkit. What I'd like to know is if the Webdriver alert API
works with the PhantomJS driver. If it does, then we should be able to
implement the API in Poltergeist and capybara-webkit.

Cheers.

Matt Horan matt@matthoran.com http://matthoran.com/

[abotalov]

Ghostdriver doesn't support that part of Webdriver API - https://github.com/detro/ghostdriver/issues/20

[mikepack]

@abotalov in terms of naming, I agree we should stick to the spec as much as possible.

The spec seems to only support reactive dialog management. As the issue you linked to points out, phantom doesn't accommodate this approach. So, the goal of this PR is to support both proactive and reactive management, which is why this PR currently wraps such behavior in a block.

Proactive management in headless drivers (eg capybara-webkit, poltergeist, ghostdriver) looks like this:

1. Developer takes actions on forthcoming prompts
2. Page "displays" prompt, but because it's headless does not block
3. Make assertions about the prompt messages cached by driver

Reactive management in Selenium looks like this:

1. Page displays prompt and blocks UI until action is taken
2. Make assertions about the message contents
3. Developer explicitly takes action to close dialog

Can you provide examples of how your proposed API would accommodate both of these cases?

[twalpole]

@mikepack Any chance of getting this PR rebased against current master and the naming changes @abotalov mentioned integrated, I'd like to move forward with getting this finished up and integrated. I agree that the block passing method is probably the only way to currently allow this feature to be implemented by all/most of the current drivers available, so unless @abotalov has a suggestion for how the API can work with the headless-drivers I think we can move forward with the current approach

[mikepack]

@twalpole I've rebased off master, replaced instances of "reject" with "dismiss", and "notification" with "modal".

[abotalov]

I would prefer to wait for response to https://www.w3.org/Bugs/Public/show_bug.cgi?id=24979 to be made in Webdriver W3C bug tracker.

I think W3C specs are important and IMO it would be better to provide similar non-block API instead of rolling block one. Maybe, @AutomatedTester will be able to say something about it.

[twalpole]

@abotalov Ok - we can hold off a little while, but I would like to get this functionality into Capybara in the next few weeks, and in a way that all/most of the current drivers can implement. Currently that looks to be with the block format, since prior knowledge of a modal dialog about to be opened, and the desired response seems to be needed by some of the drivers.

[twalpole]

@mikepack - The async tests are failing since there is no code after executing the block that is waiting for the modal to appear

[twalpole]

@mikepack - also if you could update the tests to the "expect" format rather than should due to the upcoming RSpec 3 that would be great -- thanks

[mikepack]

@twalpole Yeah I noticed that as well. I'll do another pass and get everything passing.

[AutomatedTester]

Sorry for taking so long to reply.

switchTo.Alert() is not described in the webdriver specification since it a purely local end API. We make the assumption that the remote end will still have access to modals no matter which context we are in. We are not switching windows (the Javascript sense) so we don't have to change any contexts on the remote end.

Hope that helps

[abotalov]

I think I'm fine with merging this after issues pointed by @twalpole will be fixed.

However, what about naming a method accept_alert_on instead of accept_alert? IMO it would enhance readability.

[twalpole]

I've merged this into the modal branch - and then added wait and text options, to allow for adjustable wait times (selenium has its own 2 second wait so for selenium this actually only works in multiples of 2 seconds) and for matching the text of the modal before accepting/dismissing - comments welcome -- if there are no concerns I will merge into master in a few days

[abotalov]

@twalpole It would be also good to add more info to yard comments:

# @return [String]  the message shown in the modal
# @raise [Capybara::ModalNotFound]  if modal dialog hasn't been found

Also it would be good to make a pull request from modal to master branch. I think commit comments will have better visibility.

[twalpole]

Closing this PR - its continued on PR #1322

[mikepack]

Apologies for the lack of movement. I'm not sure how I got this far without handling the async case that @twalpole mentioned (though I thought I tested it). One of the problems with it is that it would require the same wait logic that happens on #synchronize. I didn't want to dive too deep into sharing that logic.

[twalpole]

@mikepack - it did work with async - but only because selenium itself has a 2 second wait for modal boxes. I implemented somewhat adjustable delays using the wait functionality built into selenium-webdriver. Its only somewhat because of the selenium 2 second delay which means it actually ends up being multiples of 2 seconds. Anyway -- no problem for the lack of movement you had it mostly done - I've moved it over to PR #1322 if you want to see what I added to your PR