[Bugfix & Enhancement] Screenshot

[yaquawa]

Hey, thanks for the impressive works!!

I just found some issues, hopefully this can be merged 😆

Solved

1. When using the Emulation, viewport won't get resized.
2. Can't get the correct full window size when element positioned using the position: absolute;.
3. Lazy-load images won't get loaded when capturing the full document.
4. The image scale won't change on higher pixel ratio device(window.devicePixelRatio > 1) when taking screenshot of an element. (I have reported the issue here)
5. Added capture_viewport & capture_viewport_as_base64 methods

Enhancement

1. Use the chrome-driver's implementation to capture the element's screenshot.
2. Added capture_screenshot_as_base64 method to both BaseWebDriver and WebDriverElement.
3. Added a parameter waiting_time for waiting images to be loaded.

Breaking changes

Deprecated screenshot method, the new method name is capture_screenshot(signature changed).

I think we shouldn't force user to place the image to the temp directory. It's the user's responsibility to determine where the saved file should go 😀
(not yet fixed the test case and docs, if you are ok for this behavior I'll go to fix it.)

[douglascamata]

It's still possible to keep the same signature like this:

def screenshot(self, name="", suffix=".png", full=False, waiting_time=0):
        if full:
            self.full_screen()
            # trigger the `scroll` event to ensure lazy-load images can be rendered.
            self.execute_script("window.dispatchEvent(new Event('scroll'))") 
        if waiting_time > 0:
            time.sleep(waiting_time)

        if not name:
             (fd, name) = tempfile.mkstemp(prefix=name, suffix=suffix)
             os.close(fd)
        result = self.driver.get_screenshot_as_file(name)

        self.recover_screen()

        return result

What do you think?

[yaquawa]

Thanks for the review!

This way, the return value should be the filename otherwise user can't find it.
but, I still think this should be the user's responsibility to specify the file path, so make the code easier to understand and predictable also more low coupling.

I mean it can be more readable if you write it something like this:

browser.screenshot(get_temp_filename())


# helper...
def get_temp_filename(prefix=None, suffix='.png'):
    (fd, name) = tempfile.mkstemp(prefix=prefix, suffix=suffix)
    os.close(fd)

    return name

# and we can overwrite saved file every time by doing this:
browser.screenshot('~/latest.png')

[douglascamata]

I understand that. I'm just trying to think of something that wouldn't break backwards compatibility. In my idea you can still overwrite the file a call like the one you gave browser.screenshot('~/latest.png') would work as you expect.

About the path: while the user should be responsible for a path, we can still provide a default temporary path and if the user really cares they can specify it.

About what the function should return: why we should return the file path if the user is giving the file path to the function? Having a file descriptor, for example, makes it very easy to compare the screenshot with a test fixture.

If we're breaking backwards compatibility, we could change from using tempfile.mkstemp to use tempfile.NamedTemporaryFile (https://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile) and return a higher level file object.

[yaquawa]

In my idea you can still overwrite the file a call like the one you gave browser.screenshot('~/latest.png') would work as you expect.

I don't think this would work, the file name will change every time when calling the tempfile.mkstemp.

About what the function should return: why we should return the file path if the user is giving the file path to the function?

I mean if you are going to use the tempfile.mkstemp then we should return the file path, because the file name changes every time. In this commit screenshot() returns True if succeed or False if not(which is the value selenium returns).

[douglascamata]

I don't think this would work, the file name will change every time when calling the tempfile.mkstemp.

This function is only called whenever you don't give a name parameter to the function. If a name is not given, tempfile.mkstemp doesn't run.

I see the problem then. And I think there is no solution on how to move forward without breaking backwards compatibility. The backwards compatibility is important though. Changing functions signature and return values needs to be done slowly.

We should:

• Leave the current functions as they are today and add a deprecation warning, like done in HTTP Error 403: request disallowed by robots.txt #702.
• Implement your proposed logic in new functions, with a different name.
• Release a new version of Splinter with a minor version increment with your added feature. For some time you new implementation and the old one will have to live together.
• When the deprecation has been there for long enough in the old functions, on the next major release (where breaking changes can be introduced) we drop them and only your improved versions of such functions will stay.

Does this sound like a good plan for you, @yaquawa?

[yaquawa]

For example like Laravel, they have the major versions for each branch.
So like what they do, we can create a branch called 0.11.x. On that branch, this commit won't be merged at all, instead we add some warning to the screenshot method on that branch. For this commit, just merge it into the master branch.

[douglascamata]

I'm sorry, but our model is not exactly the same as Laravel. Why not proceed as I proposed? I don't understand why you don't like my proposal... it's the way we used to work with breaking backwards compatibility and deprecations.

[yaquawa]

I'm all ok for your proposal 👌
I just thought that the newly added methods have to be renamed back again when they are dropped, which may take a little effort too. Anyway I'm going to respect for the way how you manage this repo! Thank you for your time😀

[douglascamata]

Thanks for the patience, @yaquawa. I'm sorry it was not clear until now. I'm eager to merge your contribution :)

[yaquawa]

Thanks! @douglascamata
I've putted the old method back, and added the new one as capture_screenshot(which matches the CDP's method name).

[douglascamata]

@yaquawa hey, I was having some holidays for the last 2 weeks and a half. How is it going here? Just ping me whenever you think the PR is ready 💪

[yaquawa]

Hi @douglasmiranda, all the changes I've made are described on the top thread of this topic😀I think this PR is ready to go!

[douglascamata]

Hey @yaquawa, I'm really sorry for your bad experience in this PR with my delayed reviews. 😞

I only have a very minor comment left, and you have to fix a merge conflict. Then we can finally merge this!

[douglascamata]

What do you think about using the form warnings.warn('Deprecated, use `self.capture_screenshot()` instead.', FutureWarning) to be more explicit about the intent of the warning? It comes from https://docs.python.org/3/library/exceptions.html#FutureWarning.

The same applies for the other usages.

[yaquawa]

Sorry for the delayed reviews here too..!
I'm all for it 😀

[jsfehler]

Since we're relying on Selenium's element capture at this point, this PR is unnecessary.