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

Implement wait_for_selector #236

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

### Added

- `Ferrum::Browser#wait_for_selector`: returns instance of `Ferrum::Node` that's matched by provided selector.
- Alias `Ferrum::Frame#content=` to `Ferrum::Frame#set_content`
- Alias `Ferrum::Node#propery` to `Ferrum::Node#[]`
- Implement `Ferrum::Network#blacklist=` and `Ferrum::Network#whitelist=`
Expand Down
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,18 @@ browser.go_to("https://google.com/")
browser.body # => '<html itemscope="" itemtype="http://schema.org/WebPage" lang="ru"><head>...
```

#### wait_for_selector : `Node`

```ruby
browser.wait_for_selector(css: "body", timeout: 5000) # => [Node]
browser.wait_for_selector(xpath: "//body", interval: 200) # => [Node]
```

* options `Hash`
* `:css` (String) - selector in css format, not specified by default.
* `:xpath` (String) - selector in xpath format, not specified by default.
* `:timeout` (Integer) - timeout for the selector-waiting in milliseconds, 3000 ms by default.
* `:interval` (Integer) - interval that used to compute attempts for the selector-waiting (max attempts = timeout / interval), 100 by default.

## Screenshots

Expand Down
2 changes: 1 addition & 1 deletion lib/ferrum/browser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class Browser
evaluate evaluate_on evaluate_async execute evaluate_func
add_script_tag add_style_tag bypass_csp
on goto position position=
playback_rate playback_rate=] => :page
playback_rate playback_rate= wait_for_selector] => :page
delegate %i[default_user_agent] => :process

attr_reader :client, :process, :contexts, :logger, :js_errors, :pending_connection_errors,
Expand Down
27 changes: 27 additions & 0 deletions lib/ferrum/frame/dom.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,33 @@ def body
evaluate("document.documentElement.outerHTML")
end

def wait_for_selector(css: nil, xpath: nil, timeout: 3000, interval: 100)
evaluate_func(%(
Copy link
Contributor

Choose a reason for hiding this comment

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

To be more consistent with the rest of the gem, would you mind changing this to use a JS heredoc? This is helpful because many editors support code highlighting in the language specified:

Original:
image

VS:

Subl:
image

Code:
image

I haven't tried the code out yet, but I'm looking forward to this feature!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it makes sense, will change, thanks!

function(selector, isXpath, timeout, interval) {
var attempts = 0;
var max = timeout / interval;
function waitForSelector(resolve, reject) {
if (attempts > ((max < 1) ? 1 : max)) {
return reject(new Error("Not found element match the selector: " + selector));
}
var element = isXpath
? document.evaluate(selector, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue
: document.querySelector(selector);
if (element !== null) {
return resolve(element);
}
setTimeout(function () {
waitForSelector(resolve, reject);
}, interval);
attempts++;
}
return new Promise(function (resolve, reject) {
waitForSelector(resolve, reject);
});
}
), css || xpath, css.nil? && !xpath.nil?, timeout, interval, awaitPromise: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was feeling like this arguments list was a little crowded, and also noticed this function could be incorrectly called without raising an exception: one of css or xpath is required, but not handled. What do you think about something like this:

      def wait_for_selector(css: nil, xpath: nil, timeout: 3000, interval: 100)
        raise ArgumentError.new("Provide :css or :xpath, but not both") if css && xpath
        raise ArgumentError.new("Provide :css or :xpath") unless css || xpath

        evaluate_func(<<~JS, css || xpath, !!xpath, timeout, interval, awaitPromise: true)
          function(selector, isXpath, timeout, interval) {
            var attempts = 0;

The guard clauses protect the arguments from being called incorrectly, and also simplify the resolution to isXpath, since it can now only be one or the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, it looks like too much for one method to be responsible for selector/XPath and also for validations.
However, I see the confusing point here as well. In this case, I think we should use simple methods for more clear API, like those:

      def wait_for_selector(css, **options)
        wait_for_element(css: css, **options)
      end
      def wait_for_xpath(xpath, **options)
        wait_for_element(xpath: xpath, **options)
      end

The point to merge two ways of element-find logic was a follow a DRY principle for a common JS function, especially this part:

              var element = isXpath
                ? document.evaluate(selector, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue
                : document.querySelector(selector);

therefore, I think, we can find a way to refactor it with a private function by passing a js function, gimme see closer, will do an update soon.

Copy link
Contributor

@ttilberg ttilberg Feb 1, 2022

Choose a reason for hiding this comment

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

Nice one, yes, I think this is a better direction. Consider wait_for_css rather than wait_for_selector as a selector could generically mean css or xpath. It makes the expected arg more clear.

end

def xpath(selector, within: nil)
expr = <<~JS
function(selector, within) {
Expand Down
4 changes: 2 additions & 2 deletions lib/ferrum/frame/runtime.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ def execute(expression, *args)
true
end

def evaluate_func(expression, *args, on: nil)
call(expression: expression, arguments: args, on: on)
def evaluate_func(expression, *args, on: nil, **options)
call(expression: expression, arguments: args, on: on, **options)
end

def evaluate_on(node:, expression:, by_value: true, wait: 0)
Expand Down
2 changes: 1 addition & 1 deletion lib/ferrum/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def reset
delegate %i[at_css at_xpath css xpath
current_url current_title url title body doctype content=
execution_id evaluate evaluate_on evaluate_async execute evaluate_func
add_script_tag add_style_tag] => :main_frame
add_script_tag add_style_tag wait_for_selector] => :main_frame

include Animation
include Screenshot
Expand Down
72 changes: 72 additions & 0 deletions spec/browser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,78 @@ module Ferrum
expect(browser.evaluate("window.last_hashchange")).to eq("#foo")
end

context "wait_for_selector" do
before do
browser.go_to("/ferrum/with_js")
end

it "returns Node by provided selector" do
expect(browser.wait_for_selector(css: "div#wait_for_selector")).to be_kind_of(Ferrum::Node)
expect(browser.wait_for_selector(xpath: "//div[@id='wait_for_selector']")).to be_kind_of(Ferrum::Node)
end

it "waits for provided css selector" do
expect(browser.wait_for_selector(css: "div#wait_for_selector")).not_to be_nil
end

it "waits for provided css hidden selector" do
expect(browser.wait_for_selector(css: "div#wait_for_hidden_selector")).not_to be_nil
end

it "waits for provided xpath selector" do
expect(browser.wait_for_selector(xpath: "//div[@id='wait_for_selector']")).not_to be_nil
end

it "waits for provided xpath hidden selector" do
expect(browser.wait_for_selector(xpath: "//div[@id='wait_for_hidden_selector']")).not_to be_nil
end

it "raises error when default timeout exceed" do
expect do
browser.wait_for_selector(css: "div#not_existed_element")
end.to raise_error(Ferrum::JavaScriptError, /Not found element match the selector/)
end

it "raises error when timeout exceed" do
expect do
browser.wait_for_selector(css: "div#wait_for_selector", timeout: 800)
end.to raise_error(Ferrum::JavaScriptError, /Not found element match the selector/)
end

it "raises error when provided invalid css" do
expect do
browser.wait_for_selector(css: "//div[@id='wait_for_selector']")
end.to raise_error(Ferrum::JavaScriptError, /Failed to execute 'querySelector' on 'Document'/)
end

it "raises error when provided invalid xpath" do
expect do
browser.wait_for_selector(xpath: "div#wait_for_selector")
end.to raise_error(Ferrum::JavaScriptError, /Failed to execute 'evaluate' on 'Document'/)
end

it "waits less than provided timeout when node found" do
Timeout.timeout(1) do
expect(browser.wait_for_selector(css: "div#wait_for_selector", timeout: 2000)).not_to be_nil
end
end

it "waits for selector within frame" do
browser.execute <<-JS
setTimeout(function(){
document.body.innerHTML += "<iframe src='about:blank' name='frame'>";
var iframeDocument = document.querySelector("iframe[name='frame']").contentWindow.document;
var content = "<html><body><div id='wait_for_selector_within_frame'></div></body></html>";
iframeDocument.open("text/html", "replace");
iframeDocument.write(content);
iframeDocument.close();
}, 900);
JS
frame = browser.wait_for_selector(xpath: "//iframe[@name='frame']").frame
expect(frame.wait_for_selector(xpath: "//div[@id='wait_for_selector_within_frame']")).not_to be_nil
end
end

context "current_url" do
it "supports whitespace characters" do
browser.go_to("/ferrum/arbitrary_path/200/foo%20bar%20baz")
Expand Down
2 changes: 1 addition & 1 deletion spec/network/response_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class Network
%r{/ferrum/jquery.min.js$} => File.size("#{PROJECT_ROOT}/spec/support/public/jquery-1.11.3.min.js"),
%r{/ferrum/jquery-ui.min.js$} => File.size("#{PROJECT_ROOT}/spec/support/public/jquery-ui-1.11.4.min.js"),
%r{/ferrum/test.js$} => File.size("#{PROJECT_ROOT}/spec/support/public/test.js"),
%r{/ferrum/with_js$} => 2343
%r{/ferrum/with_js$} => 2919
}

resources_size.each do |resource, size|
Expand Down
17 changes: 17 additions & 0 deletions spec/support/views/with_js.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,23 @@
display: inline;
}
</style>
<script>
$(document).ready(function(){
setTimeout(function(){
const div = document.createElement('div');
div.setAttribute('id', 'wait_for_selector');
document.body.appendChild(div);
}, 900);
});
$(document).ready(function(){
setTimeout(function(){
const div = document.createElement('div');
div.setAttribute('id', 'wait_for_hidden_selector');
div.setAttribute('style', 'display:none;');
document.body.appendChild(div);
}, 900);
});
</script>
</head>

<body>
Expand Down