Clean up keywords related to alerts

[pekkaklarck]

The current alert related keywords are a mess. Biggest problems:

1. Selecting should the alert be dismissed by pressing Ok or Cancel needs to be done using separate Choose Ok/Cancel On Next Confirmation keywords. I think there used to be a technical reason to do it this way, but looking at the current code this doesn't seem to be the case. Much better to just pass this information as a normal argument to keywords dismissing alerts.

2. There are several keywords with same/similar functionality. In practice Confirm Action, Dismiss Alert an Get Alert Message all close the alert by default.

3. Strange configuration options and default values. For example, Dismiss Alert actually accepts the alert by default and needs to be separately configured to dismiss it.

---

If I've understood it correctly, there can be three kind of Javascript alerts:

1. An alert with a message and Ok button (window.alert).
2. An alert with a message and Ok/Cancel buttons (window.confirm)
3. An alert with a message, text input field, and Ok/Cancel buttons (window.prompt)

We need to think what kind of a set of keywords are needed to handle these different alerts. The more we can keep backwards compatibility (i.e. keyword names) the better, but the most important goal is making the set of keywords consistent and easy to use.

[pekkaklarck]

The current situation is so bad that I assign this tentatively to SL 3.0 scope. If there's too much work, it may slip to 3.1. In that case we should at least enhance the docs a little.

[pekkaklarck]

My initial proposal for a list of alert keywords is below. It's based on the actions you actually can do with alerts and influenced also by the existing set of keywords and how they behave.

1. Alert Should Be Present [expected_message=None]: Existing keyword but changed argument name. Old functionality is OK, but could also return the actual alert message. Can also consider adding negative variant Alert Should Not Be Present.

2. Handle Alert [action=ACCEPT]: New keyword. By default accepts the alert (i.e. presses Ok), but allows dismissing it (i.e. pressing Cancel) by using like action=DISMISS, and could also support action=LEAVE or similar to leave the alert in place. Should return the alert message.

3. Input Text Into Alert [text, action=ACCEPT]: Mostly the same as the current Input Text Into Prompt but accepts the alert by default. Action can be also be configured using the action argument exactly like with Handle Alert.

[pekkaklarck]

My proposal above would mean that many of the existing keywords would need to be deprecated and removed later. My proposed actions and reasoning is below. Notice that with term loud deprecation I mean emitting a deprecation warning right now and silent deprecation means documenting that keyword is deprecated but not yet emitting a warning. Loudly deprecated keywords could then be removed already in SL 3.1, and silently deprecated keywords could be loudly deprecated at that point.

1. Choose Ok/Cancel On Next Confirmation: Totally useless keywords nowadays. Much better to pass the action to the keyword handling the alert as an argument directly. These should be deprecated loudly. Could even consider removing their underlying functionality altogether already at this point, but that would obviously be backwards incompatible change.

2. Confirm Action: Can work as an alias for Handle Alert but should be silently deprecated. If the functionality of the above keywords is removed, won't work correctly if Choose Cancel On Next Confirmation is used.

3. Get Alert Message: This keyword currently dismisses the alert by default and can be configured to leave the alert in place with dismiss=False. In my opinion that is really strange behavior, and I'd like to change it so that alerts are accepted by default but it's possible to dismiss or leave them. The problem is that changing the default behavior would be backwards incompatible. Although I'd prefer an explicit keyword for getting the alert message, I don't think it's worth the effort deprecating the current behavior and changing it later. The proposed Handle Alert returns the message, and support everything this keyword should. Thus I think we should just silently deprecate this keyword in favor of Handle Alert.

4. Dismiss Alert: This keyword currently accepts the alert by default and requires configuration to actually dismiss it. The proposed 'Handle Alert` handles this much better and this keyword can be silently deprecated.

5. Input Text Into Prompt: This keyword doesn't accept the alert, it cannot even be configured, meaning that Confirm Action or some other keyword needs to be called separately afterwards. Also the name with prompt is a bit weird, although I know this particular alert is created by using window.prompt(). The proposed Input Text Into Alert is consistently named and also accepts the alert by default.

[boakley]

How about instead of multiple new keywords, you add a single new keyword with a name like With the next alert that takes actions as arguments.

For example:

    With the next alert
    ...  Click  OK

    With the next alert
    ...  Input text  Hello, world

    With the next alert
    ...  message should be  Thank you for signing up!
    ...  AND  click ok

Another possible name might be When the alert pops up

    When the alert pops up  Click  OK
    When the alert pops up  Input text  Hello, world
    When the alert pops up  message should be  Thank you for signing up  AND  click  ok

Note: "Click", "input text", "message should be" aren't keywords, though they could have the same as existing keywords I suppose.

[pekkaklarck]

Examples by @boakley look nice but I see two problems with them in this context:

• The style is inconsistent with other kws in this lib.
• It's a bit strange to say With next alert when the alert is likely to have occurred at that point.

I wouldn't thus use this style here, but in some other context it could work really well. Also, I propose adding just one totally new keyword Handle Alert.

[boakley]

The name could be `with the current alert` instead of `with next alert`, to satisfy your second concern. I certainly think it looks better in a test than "Handle Alert", but I realize that's pretty subjective.

…

On Thu, Sep 28, 2017 at 9:47 AM, Pekka Klärck ***@***.***> wrote: Examples by @boakley <https://github.com/boakley> look nice but I see two problems with them in this context: - The style is inconsistent with other kws in this lib. - It's a bit strange to say With next alert when the alert is likely to have occurred at that point. I wouldn't thus use this style here, but in some other context it could work really well. Also, I propose adding just one totally new keyword Handle Alert. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#933 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEmYuAn0CXOUIq4ky3M9y-lunHdYsQJks5sm7F8gaJpZM4Pm4aY> .

[pekkaklarck]

Yeah, these examples look nice. But as I said, the style they use is totally different than what other keywords use and I really like consistency. Also, the keywords provided by this library aren't really meant to be used directly on the test case level. They should be used by higher level keywords created as Robot's user keyword or by higher level libraries.

[pekkaklarck]

After some more investigation I noticed that some of the things I considered really strange were in fact repressions (#934). At the same time I found some new strange things, like Input Text Into Prompt not accepting the alert automatically. I have updated my earlier comments based on this new insight.