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: prevent misuse of transformOptions #389

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

canstand
Copy link
Collaborator

@canstand canstand commented Nov 3, 2023

transformOptions was previously restricted and does not report misuse.

Now no longer restricted, and it is more intuitive to take overrides as the second argument when calling it.

@@ -337,7 +337,6 @@ func (p *pageImpl) Screenshot(options ...PageScreenshotOptions) ([]byte, error)
}
}
overrides["mask"] = masks
options[0].Mask = nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can be overridden without assigning nil

@canstand canstand marked this pull request as ready for review November 3, 2023 10:45
@canstand canstand merged commit ddc7abd into playwright-community:main Nov 4, 2023
13 checks passed
@Lonka
Copy link

Lonka commented Nov 6, 2023

Hi,

I have a little question about how to fix this issue. I understand the playwright is based on the DevTools Protocol to control or monitor the test actions from browser.

So I think the CDP must has the screenshop interface, it's correctly for this link. captureScreenshot

But the screenshot properties of playwright-go provides is a bit different from CDP, such as Timeout, OmitBackground, etc. PageScreenshotOptions

As mentioned above, I think the channel.Send command that are changed more times as this request, is NOT sent to CDP directly. It's maybe send to the playwright service which running on the nodejs. But in the playwright opensource, I could not find where to receive the commands from the playwright-go.

I would like to learn the channel.Send command is wrappered args to what payload(JSON?), and send to where? and where to receive this command. If it is possible may I know the received code in where? thanks.

@canstand
Copy link
Collaborator Author

canstand commented Nov 6, 2023

playwright-go communicates with playwright server cli via stdin/out, see

transport := newPipeTransport(stdin, stdout)

Protocol reference

@canstand canstand linked an issue Nov 6, 2023 that may be closed by this pull request
@mxschmitt
Copy link
Member

Playwright for Go (client) -> Playwright driver (server) -> CDP

@Lonka
Copy link

Lonka commented Nov 7, 2023

Hi all,

Thank you for protocol reference link and data flow describe, which helped me understood how the playwright-go to call the CDP throught playwright driver.

In playwright-go, write the message via stdin/out as @canstand said.

if _, err = t.stdin.Write(lengthPadding); err != nil {
return err
}
if _, err = t.stdin.Write(msg); err != nil {
return err
}

The message is formattd as JSON, like as below:

{
    "guid": "page@062788f754dd92af98243a1c957eb01a",
    "id": 22,
    "metadata":
    {
        "apiName": "SerializeCallStack",
        "isInternal": false,
        "location":
        {
            "file": "connection.go",
            "line": 152
        },
        "wallTime": 191577195
    },
    "method": "screenshot",
    "params":
    {
        "mask":
        [
            {
                "frame":
                {
                    "guid": "frame@c1bdcb321ef5827508f483b621232c6e"
                },
                "selector": "//*[@id=\"btnLogin\"]"
            }
        ],
        "maskColor": "rgba(255,0,255,0.5)"
    }
}

And the code of the playwright driver to receive the message as below:
https://github.com/microsoft/playwright/blob/19b0f5ccb38fb0dee6e04b44448a8a2a212ee393/packages/playwright-core/src/cli/driver.ts#L39C1-L41

The stdin/out protocol must reference cli protocol.

Thanks again.

@canstand canstand deleted the issue-388 branch December 19, 2023 02:46
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.

Does work the page.Screenshot() function with MaskColor?
3 participants