[draft] Create wait structure

[zhiburt]

Overview

Hello @jonhoo, I've tried to improve *_wait methods by creating a Wait structure which will allow to create it's own rules.

The design is primarily inspired by C# driver.
Here's an example how it's done in C#.

var wait = new WebDriverWait(driver, 100);
wait.Until(d => d.FindElements(By.Id("processing")).Count == 0);

And here this example with new Wait structure.

let mut wait = new Wait::new(client, timeout);
wait.until(|d| async { d.FindElement(Locator::Id("processing")).await.map_or(None, |_| Some(()) });

We also could create a set of predefined methods wait_for_visible, wait_for_not_present etc...

link #27
link #85
link #93

Implementation

Sadly I didn't figure out a way to support closures properly. There's an issue with lifetimes therefore in changed wait_for_find method I created a nested function which takes a cloned Client. That's only because of my incompetence here otherwise it had to be as simple as in example. I've not resolved the issue yet and decided to send the draft with believe that you may know some of pathways to resolve this issue 😄 .

As I understand right now wait_for method may have the same issue?
If you try to use &mut Client in closure there will be a problem occurred.

[jonhoo]

I think your problem likely stems from trying to define a fn closure here. If you just put this code directly into the move |client| {} below, I think you should be fine, and not need the clone?

[jonhoo]

I think, by default, this method should never time out. The user asked us to wait indefinitely. If the user wants a timeout, they can do so by wrapping the returned future in a tokio::time::timeout.

[zhiburt]

I hope that you're speaking about changes in wait_for here's you are right.
It's just a method which I chose to demonstrate the Wait 😄 and didn't noticed that I break it's logic 😄

Probably there could be introduced a new type WaitForever which would do just loop. And after we could amalgamate these by a trait Waiter which has a method until.

[jonhoo]

My argument was more that all you need is the ability to wait forever. If the user wants to stop waiting before the action succeeds, they can just wrap the future in tokio::time::timeout :)

[jonhoo]

I think the whole Wait type only really makes sense if users can define their own waiters, which would mean making this method pub, no?

[zhiburt]

It was meant to be a base implementation on which could be piled different peaces. But I didn't decide at the time it created for users or for lib itself so it just get a smallest scope I was considering.

It may worth it.

[jonhoo]

Hopefully the comments above help a little! I'm still not quite sure what adding this kind of wait module actually helps with? As in, why is it better than the user just writing this loop themselves? The only reason I explicitly implemented wait_for_find and wait_for_navigation is that we can in theory be smarter about the loop in those two specific cases. For wait_for_find, we might be able to use a MutationObserver to only retry the find when the page changes, rather than on a timer. And similarly, for wait_for_navigation, we may be able to hook into a document onload event somehow. I don't think that's true for a "general" wait as implemented here, at which point it feels like the user can just add their own wait loop?

[zhiburt]

I consider it's useful to have an abstractions wait_for_not_visible, wait_for_present, wait_for_editable etc. because it's not often clear how to implement them on it's own.

For example element_is_editable is pretty complicated to have implemented from end user. And it may be not even a completed version.

     let is_displayed = match element.attr("style").await? {
            Some(style) if !style.contains("display: none;") => true,
            None => true,
             _ => false,
     };

      let is_enabled = match element.attr("disabled").await? {
           Some(..) => false,
           _ => true,
      };

      is_displayed && is_enabled

The timer logic could be shifted to MutationObserver if it'll work out.

[zhiburt]

By the way what do you think about adding Element::style method which would return a std::collections::HashMap<String, String> of styles since parsing ;, : on it's own take a while :)

[jonhoo]

I guess my argument is more that those methods should only be implemented in their "one shot" version, so Element::is_editable and Element::is_present. And then the user can just call those in a retry loop with a timer to get the wait_for behavior. It's not clear why we need to provide the looping version in the library itself.

Now, it might be useful to provide something like a Wait struct that users can use to easily get a retry loop, but I don't think we should also implement tons of wait_for_X methods.

By the way what do you think about adding Element::style method which would return a std::collections::HashMap<String, String> of styles since parsing ;, : on it's own take a while :)

I'm not sure I follow? Are you talking about parsing the style tag? I really don't want to do that, because we probably don't want to get into trying to parse CSS strings, as they can get very complicated (just splitting by ; won't work). Besides, authors should probably be querying the current style using .execute and inspecting .style, rather than trying to parse out the style attribute.