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

WindowBounds have not only width and height but also left and top #116

Merged
merged 7 commits into from Feb 26, 2021

Conversation

Nakilon
Copy link
Contributor

@Nakilon Nakilon commented Sep 11, 2020

The test does not pass in headless -- get_position returns the same after repositioning, I don't know why.

lib/ferrum/page.rb Outdated Show resolved Hide resolved
lib/ferrum/page.rb Outdated Show resolved Hide resolved
spec/browser_spec.rb Outdated Show resolved Hide resolved
spec/browser_spec.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@Mifrill Mifrill left a comment

Choose a reason for hiding this comment

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

@Nakilon thanks for the PR! I see 2 logic-branches here:
1 - refactor resize method and add missed test for error:
To resize minimized/maximized/fullscreen window, restore it to normal state first
2 - get_position/set_position implementation
So, let's split it for 2 PR's

A bit refactor of Page#resize like this:

    def resize(width: nil, height: nil, fullscreen: false)
      if fullscreen
        width, height = document_size
        set_window_bounds(windowState: "fullscreen")
      else
        set_window_bounds(windowState: "normal")
        set_window_bounds(width: width, height: height)
      end

      command("Emulation.setDeviceMetricsOverride", slowmoable: true,
                                                    width: width,
                                                    height: height,
                                                    deviceScaleFactor: 1,
                                                    mobile: false,
                                                    fitWindow: false)
    end

    def window_id
      @browser.command("Browser.getWindowForTarget", targetId: @target_id)["windowId"]
    end

    def set_window_bounds(bounds = {})
      @browser.command("Browser.setWindowBounds", windowId: window_id, bounds: bounds)
    end

And keep this PR for get_position/set_position implementation.

So, please merge it with the updated master.

About:

The test does not pass in headless

Can you put some more details? It would be nice to add failed test for this.

@Mifrill
Copy link
Collaborator

Mifrill commented Feb 11, 2021

@Nakilon FYI: I've moved the refactoring of Page#resize into the dedicated PR: #152
Let's keep only get_position/set_position implementation here and please share some details about the issue you mentioned.

@Mifrill
Copy link
Collaborator

Mifrill commented Feb 16, 2021

@Nakilon

The test does not pass in headless -- get_position returns the same after repositioning, I don't know why

This test is green for me locally, and the set/get-position working like a charm, can you please shed some light on your comment?

@Nakilon
Copy link
Contributor Author

Nakilon commented Feb 16, 2021

@Nakilon

The test does not pass in headless -- get_position returns the same after repositioning, I don't know why

This test is green for me locally, and the set/get-position working like a charm, can you please shed some light on your comment?

I guess this line https://github.com/rubycdp/ferrum/pull/116/files#diff-e180ec3f0f9f10ea0a9dc5fe5e4f00f83ba5e263031a80d8954cc9bc52a14b0eR102 was failing unless I use the headless: false. So it kinda was able to move only that window that is visible. Was failing on macOS and I guess in CI too when the pull request didn't have merge conflict.

lib/ferrum/page.rb Outdated Show resolved Hide resolved
lib/ferrum/browser.rb Outdated Show resolved Hide resolved
@Mifrill Mifrill requested a review from route February 26, 2021 07:19
@route
Copy link
Member

route commented Feb 26, 2021

Looking good, let's add docs to readme as well

Mifrill added a commit to Nakilon/ferrum that referenced this pull request Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Way to move the browser window?
3 participants