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

Update geckodriver version numbers #762

Closed
wants to merge 3 commits into from
Closed

Update geckodriver version numbers #762

wants to merge 3 commits into from

Conversation

MLJBrackett
Copy link

Update the references of geckodriver 0.15 -> 0.26 & Check if new implementation is needed

Fixes #651

@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #762 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #762   +/-   ##
=======================================
  Coverage   40.10%   40.10%           
=======================================
  Files          29       29           
  Lines        3159     3159           
=======================================
  Hits         1267     1267           
  Misses       1892     1892           
Impacted Files Coverage Δ
automation/CommandSequence.py 86.56% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a8bd88...8ceb6dd. Read the comment docs.

@MLJBrackett
Copy link
Author

As of right now there are 9 references of "geckodriver" and 4 references of "geckodriver 0.15"

Geckodriver references:

  1. selenium_firefox.py
  2. BrowserManager.py
  3. CHANGELOG.md
  4. automation/TaskManager.py
  5. automation/CommandSequence.py
  6. scripts/environment-unpinned.yaml
  7. environment.yaml
  8. README.md
  9. docs/OpenWPM.svg

Out of the 9 there are only two files that will need to be updated

  1. automation/CommandSequence.py
  2. README.md

CommandSequence.py

def screenshot_full_page(self, suffix="", timeout=30):
        """Save a screenshot of the entire page.

        NOTE: geckodriver v0.15 only supports viewport screenshots. To
        screenshot the entire page we scroll the page using javascript and take
        a viewport screenshot at each location. This method will save the
        parts and a stitched version in the `screenshot_path`. We only scroll
        vertically, so pages that are wider than the viewport will be clipped.
        See: https://github.com/mozilla/geckodriver/issues/570

        The screenshot produced will only include the area originally
        loaded at the start of the command. Sites which dynamically expand as
        the page is scrolled (i.e. infinite scroll) will only go as far as the
        original height.

        NOTE: In geckodriver v0.15 doing any scrolling (or having devtools
        open) seems to break element-only screenshots. So using this command
        will cause any future element-only screenshots to be mis-aligned
        """
        self.total_timeout += timeout
        if not self.contains_get_or_browse:
            raise CommandExecutionError(
                "No get or browse request preceding " "the dump page source command",
                self,
            )
        command = ScreenshotFullPageCommand(suffix)
        self._commands_with_timeout.append((command, timeout))

I don't know as of right now if there needs to be changes to anything with the above code, but with the updated version of geckodriver there is now a command to get the entire screen.

The original implementation PR here: #156
The release that added the new function: https://github.com/mozilla/geckodriver/releases/tag/v0.24.0

It would also seem that there is already an open issue for updating this code.
Issue here: #665

So either the code needs to be updated or we can remove the two 'NOTE:' comments in the code

README.md

* Screenshots
    * Selenium 3 can be used to screenshot an individual element. None of the
        built-in commands offer this functionality, but you can use it when
        [writing your own](https://github.com/citp/OpenWPM/wiki/Platform-Demo#adding-a-new-command). See the [Selenium documentation](https://seleniumhq.github.io/selenium/docs/api/py/webdriver_remote/selenium.webdriver.remote.webelement.html?highlight=element#selenium.webdriver.remote.webelement.WebElement.screenshot).
    * Viewport screenshots (i.e. a screenshot of the portion of the website
        visible in the browser's window) are available with the
        `CommandSequence::save_screenshot` command.
    * Full-page screenshots (i.e. a screenshot of the entire rendered DOM) are
        available with the `CommandSequence::screenshot_full_page` command.
        * This functionality is not yet supported by Selenium/geckodriver,
          though [it is planned](https://github.com/mozilla/geckodriver/issues/570).
          We produce screenshots by using JS to scroll the page and take a
          viewport screenshot at each location. This method will save the parts
          and a stitched version in the `screenshot_path`.
        * Since the screenshots are stitched they have some limitations:
            * On the area of the page present when the command is called will
              be captured. Sites which dynamically expand when scrolled (i.e.,
              infinite scroll) will only go as far as the original height.
            * We only scroll vertically, so pages that are wider than the
              viewport will be clipped.
            * In geckodriver v0.15 doing any scrolling (or having devtools
              open) seems to break element-only screenshots. So using this
              command will cause any future element-only screenshots to be
              misaligned.

The above quote references how Full-page screenshots are not currently supported.

@birdsarah Seeing that you are the poster of the issue #651 & #665 could you direct me on how to move forward with my findings.

@MLJBrackett MLJBrackett marked this pull request as ready for review October 13, 2020 18:27
@MLJBrackett
Copy link
Author

I have just taken a look at #665 and contributor vringar has stated that we don't have the functionality yet to implement the code for a full screenshot yet since we are on an outdated version of selenium #665 (comment). For now I've added another note stating that the functionality is now included in geckodriver 0.24 but the code is still using the stitching method.

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.

Update references to geckodriver 0.15
1 participant