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

Fix #1459: Add Capture Full Page Screenshot #1762

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kumy
Copy link

@kumy kumy commented Dec 12, 2021

Please find PR to allow capture full page screenshot. It use the functions linked per @aaltat in #1459 (comment)

This add a new keyword Capture Full Page Screenshot. It also add a boolean parameter full_screen to Capture Page Screenshot.

@kumy
Copy link
Author

kumy commented Jan 29, 2022

Anyone able to review please?

@aaltat
Copy link
Contributor

aaltat commented Jan 29, 2022

I am not anymore maintaining this library, ping @emanlove But in my opinion having new keyword is not good idea, just have argument for the existing one is better.

@kumy
Copy link
Author

kumy commented Jan 29, 2022

Thanks for your reply. If there is no new keyword, then is there a way to set the argument to full_screen=True on library import? 🤔

I was using it like this:

Library    SeleniumLibrary    timeout=10    implicit_wait=0    run_on_failure=capture full page screenshot

@emanlove
Copy link
Member

I agree that an additional separate keyword would not be preferred. One problem I see is adding a parameter to the keyword (Capture Page Screenshot) that itself is a parameter to the run_on_failure library argument. Tatu has mentioned to me that there is code within BrowserLibrary which allows parameters on parameters within library import arguments. This might be better than the alternative of using the Register Keyword To Run On Failure keyword within the script. One question I have there is are we making the library import even more complicated if we do this and have plug-ins?

The other issue is that get_full_page_screenshot_as_base64 is only a method on the Firefox driver and not on any of the others. There should be some code to protect against that. I know there is desire for full page screenshot across browser but think that should be addressed in a different issue (possibly?).

@kumy
Copy link
Author

kumy commented Jan 29, 2022

So if I understand correctly,

  1. remove the new keyword, we then have to user Register Keyword To Run On Failure, should be quite easy.
  2. protect new full_screen parameter to be used on other driver than Firefox.

For 2., I've not yet any idea how to do that. Do you have existing code example from which I could learn from? Thanks

@emanlove
Copy link
Member

emanlove commented Feb 1, 2022

One technique would be to use the hasattr function to check and if if the driver object has the get_full_page_screenshot_as_base64 method.

@kumy
Copy link
Author

kumy commented Apr 2, 2022

@emanlove Hello, I've finally done the requested changes 😄

  • New keyword removed
  • Check function presence or fallback to normal mode.

Could be used like:

*** Settings ***
Library  SeleniumLibrary  run_on_failure=Capture Full Page Screenshot     # […]

# […]

*** Keywords ***
Capture Full Page Screenshot
    Capture Page Screenshot     full_screen=True

Or by using

Register Keyword To Run On Failure      Capture Full Page Screenshot

Thanks for your review

@kumy
Copy link
Author

kumy commented Jun 26, 2022

Hi, is there anything else I can do? Thanks

@aaltat
Copy link
Contributor

aaltat commented Jun 26, 2022

@emanlove why the CI did not run?

@kumy tests are needed.

@emanlove
Copy link
Member

Not sure why the CI tests didn't run. Initial quick review doesn't seem to indicate anything. Doing some more in depth looking ..

@hrnaltnts
Copy link

hrnaltnts commented Jun 29, 2023

Hello
Currently, do have any solution to take full page screenshots using Selenium Library?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants