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

[bidi][js] Update the capture screenshot APIs to include all parameters and remove scroll parameter #13744

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Mar 27, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

W3C BiDi spec is updated for captureScreenshot command. Additional parameters have been added and the "scrollIntoView" parameter is removed for element screenshots. The changes reflect this. This involves a breaking change of using "scrollIntoView" parameter. Avoiding deprecating it since the latest version of browser (including the one in Selenium Gtihub CI) will not support it.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

bug_fix, enhancement


Description

  • Introduced CaptureScreenshotParameters class to support the updated W3C BiDi spec for screenshot capture, allowing for more flexible screenshot options.
  • Added ClipRectangle classes (BoxClipRectangle and ElementClipRectangle) to specify clipping areas for screenshots.
  • Updated browsingContext.js to remove the deprecated scrollIntoView parameter and to support the new screenshot parameter handling.
  • Modified tests to reflect these changes and to test the new functionality.

Changes walkthrough

Relevant files
Enhancement
browsingContext.js
Update Screenshot APIs to Reflect W3C BiDi Spec Changes   

javascript/node/selenium-webdriver/bidi/browsingContext.js

  • Added support for new CaptureScreenshotParameters in captureScreenshot
    method.
  • Removed scrollIntoView parameter from captureElementScreenshot method.
  • Refactored screenshot methods to utilize the updated W3C BiDi spec.
  • +19/-5   
    captureScreenshotParameters.js
    Introduce CaptureScreenshotParameters for Flexible Screenshot Options

    javascript/node/selenium-webdriver/bidi/captureScreenshotParameters.js

  • Introduced CaptureScreenshotParameters class for flexible screenshot
    parameter handling.
  • Added support for specifying origin, imageFormat, and clipRectangle.
  • +64/-0   
    clipRectangle.js
    Add ClipRectangle Classes for Screenshot Clipping               

    javascript/node/selenium-webdriver/bidi/clipRectangle.js

  • Added ClipRectangle base class along with BoxClipRectangle and
    ElementClipRectangle for specifying screenshot clipping areas.
  • +87/-0   
    Tests
    browsingcontext_test.js
    Update Tests to Use New Screenshot Parameter Handling       

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_test.js

  • Updated tests to use CaptureScreenshotParameters.
  • Removed tests for deprecated scrollIntoView parameter.
  • Added tests for box and element screenshots using new parameters.
  • +32/-7   

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @pujagani pujagani added C-nodejs C-devtools BiDi or Chrome DevTools related issues labels Mar 27, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (6c23e5e)

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 27, 2024

    PR Review

    (Review updated until commit 2be499f)

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes to the API interface, addition of new classes, and modifications to existing methods. Understanding the context and ensuring compatibility with the W3C BiDi spec requires a thorough review.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The removal of scrollIntoView parameter in captureElementScreenshot without a deprecation period might break existing implementations relying on this feature.

    Compatibility Concern: The enforcement of CaptureScreenshotParameters instance without providing a fallback or a more graceful error handling might lead to runtime errors for existing codebases not yet updated to the new API changes.

    🔒 Security concerns

    No

    Code feedback:
    relevant filejavascript/node/selenium-webdriver/bidi/browsingContext.js
    suggestion      

    Consider implementing a deprecation warning for the scrollIntoView parameter in captureElementScreenshot method before its removal to provide a smoother transition for users. [important]

    relevant lineasync captureElementScreenshot(sharedId, handle = undefined) {

    relevant filejavascript/node/selenium-webdriver/bidi/browsingContext.js
    suggestion      

    For better error handling, consider checking the type of captureScreenshotParameters more thoroughly, possibly including a check for the expected properties if not using instanceof. This can help in cases where objects mimicking CaptureScreenshotParameters are passed. [medium]

    relevant line!(captureScreenshotParameters instanceof CaptureScreenshotParameters)

    relevant filejavascript/node/selenium-webdriver/bidi/captureScreenshotParameters.js
    suggestion      

    Add validation for quality in imageFormat method to ensure it falls within an acceptable range (e.g., 0-100 for JPEG quality). This prevents runtime errors or unexpected behavior due to invalid values. [important]

    relevant linethis.#map.set('quality', quality)

    relevant filejavascript/node/selenium-webdriver/bidi/clipRectangle.js
    suggestion      

    In BoxClipRectangle and ElementClipRectangle, consider adding checks to ensure the parameters (e.g., x, y, width, height, sharedId) are within expected ranges or formats to prevent creating invalid clip rectangles. [medium]

    relevant linesuper('box')


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 27, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Use more specific error types for parameter validation.

    Consider using a more specific error type than Error for parameter validation in
    captureScreenshot. This can help callers handle errors more gracefully. For example, using
    TypeError when the type of a parameter is incorrect, or a custom error type that extends
    Error for domain-specific errors.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [222]

    -throw new Error(`Pass in a CaptureScreenshotParameters object. Received: ${captureScreenshotParameters}`)
    +throw new TypeError(`Pass in a CaptureScreenshotParameters object. Received: ${captureScreenshotParameters}`)
     
    Enhancement
    Include the received type in error messages for better debugging.

    For the imageFormat method, when quality is provided but not a number, it would be helpful
    to include the actual received type in the error message. This can aid debugging by
    providing more context.

    javascript/node/selenium-webdriver/bidi/captureScreenshotParameters.js [44]

    -throw new Error(`Quality must be a number. Received:'${quality}'`)
    +throw new Error(`Quality must be a number. Received:'${quality}' of type ${typeof quality}`)
     
    Bug
    Remove the redundant getter for type to prevent recursion.

    The type property in the ClipRectangle class is redundantly defined. The getter for type
    is attempting to return its own value, leading to a recursive call and a potential Maximum
    call stack size exceeded error. Remove the getter and directly access the type property.

    javascript/node/selenium-webdriver/bidi/clipRectangle.js [25-27]

    -get type() {
    -  return this.type
    -}
    +// Remove the getter method
     
    Correct the typo in the import statement for CaptureScreenshotParameters.

    There's a typo in the import statement for CaptureScreenshotParameters. It's imported as
    CaptureScreenshotParameter which is undefined. Correct the import to match the exported
    name from captureScreenshotParameters.js.

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_test.js [27]

    -CaptureScreenshotParameter,
    +CaptureScreenshotParameters,
     
    Maintainability
    Extract params construction into a separate method for readability.

    To improve code maintainability, consider extracting the logic for constructing the params
    object in captureScreenshot into a separate method. This can make the method more readable
    and easier to update in the future.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [227-229]

    -let params = {
    -  method: 'browsingContext.captureScreenshot',
    -  params: Object.fromEntries(screenshotParams),
    -}
    +let params = this.createScreenshotParams(screenshotParams)
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @pujagani
    Copy link
    Contributor Author

    /review

    Copy link
    Contributor

    Persistent review updated to latest commit 2be499f

    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.

    1 participant