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

User prompt handler should not dismiss prompts unless specifically configured #925

Closed
jgraham opened this issue May 24, 2017 · 8 comments
Closed
Labels
Milestone

Comments

@jgraham
Copy link
Member

jgraham commented May 24, 2017

In chapter 18. User Prompts:

missing value default state
not in the table of simple dialogs

    Dismiss the current user prompt. 

This means that you can never get an unexpected user prompt error and then use the /alerts/ endpoints to interact with it.

@jgraham jgraham added this to the Level 1 milestone May 24, 2017
@jgraham jgraham added the bug label May 24, 2017
@shs96c
Copy link
Contributor

shs96c commented May 24, 2017

This is intended. The rationale is that people will write code that expects everything to work properly, and won't be expecting the unexpected alert, and so won't have error handling code in there. Returning the error allows these users to be aware that something untoward happens, and the dimissal of the prompt means that the next command will work as expected.

For example

@Test
public void thisHasAnAlert() {
  driver.get("http://example.com");
  WebElement element = driver.findElement(By.name("q"));
  // Alert opens because of this interaction
  element.click();
  element.getText();  // Exception thrown here. 
}

@Test
public void shouldReuseTheDriver() {
  // Test runs as expected because the user prompt is already dismissed.
  driver.get("http://example.com");
}

@jgraham
Copy link
Member Author

jgraham commented May 24, 2017

It seems like there ought to at least be an opt-in method of not auto-handling alerts. AFAICT the alert API is mostly useless; it relies on you doing a thing that you know opens an alert and then carefully not running any other commands. You can't be notified that an alert was opened and then inspect it.

@andreastt
Copy link
Member

The logical consequence of way the specification is written now, means we can remove all the alert related endpoints because the user prompt will always either be dismissed or accepted. At the point of the next command, there will be no prompt to interact with.

I also think there needs to be a way to opt out of this.

@shs96c
Copy link
Contributor

shs96c commented May 24, 2017

If the user doesn't specify an unhandled prompt handler, then you can interact with prompts. If the above example had been written the user will have interacted with things properly:

@Test
public void thisHasAnAlert() {
  driver.get("http://example.com");
  WebElement element = driver.findElement(By.name("q"));
  // Alert opens because of this interaction
  element.click();
  Alert alert = driver.getAlert();
  System.out.println(alert.getText());
  alert.dismiss();
}

The "unexpected" bit really does mean "unexpected"

@shs96c
Copy link
Contributor

shs96c commented May 24, 2017

But we could add a new prompt handler of "leave in place"

@shs96c
Copy link
Contributor

shs96c commented May 24, 2017

I'm getting married and in the final days before the wedding. I'm going to replying very slowly to this issue until I return to the UK.

@shs96c
Copy link
Contributor

shs96c commented May 24, 2017

@jgraham's comment about needing to be aware that an alert would open and then interacting with it is correct (though, logically, no different from any other command --- you need to know what's happening to interact with things). I'm totally on-board with adding a "throw an exception, but leave it in place" option to the prompt handler table.

@andreastt
Copy link
Member

andreastt commented May 24, 2017

Right, thanks for your explanation in #925 (comment). I now understand how it’s supposed to work.

But we could add a new prompt handler of "leave in place"

I think this is a good idea. And it’s pluggable into what we have currently.

I'm getting married and in the final days before the wedding. I'm going to replying very slowly to this issue until I return to the UK.

In totally off-topic way, that is great news! Congratulations! 🎆🍾

shs96c added a commit to shs96c/webdriver-spec that referenced this issue Jun 1, 2017
shs96c added a commit that referenced this issue Jun 1, 2017
jlipps pushed a commit to jlipps/webdriver that referenced this issue Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants