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

[vscode] Add WebviewOptions.enableForms #59

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

Conversation

sgraband
Copy link

What it does

Added the optional enableForms field to WebviewOptions.
This allows the allow-forms sandbox attribute to be set.

Contributed on behalf of STMicroelectronics

How to test

To test you can use the following VSCode extension:

webview-forms-0.0.1.zip

It contributes 6 commands (category: Test enableForms) that will create a webview with different options set (You can see the options set in the command name and the webview will display them as well).
When you opened the webview you can inspect the DevTools to see that the allow-forms sandbox attribute is correctly set (or not set).

Review checklist

Reminder for reviewers

Copy link

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Suggested tests with the provided example extension worked as expected for the inner iframe. I don't know if the outer one is even expected to have adapted sandbox properties as well. These are set via element.sandbox.add('allow-scripts', 'allow-forms', 'allow-same-origin', 'allow-downloads'); in webview.ts
image

@sgraband
Copy link
Author

Since the outer frame is also not changed by the enableScripts field my understanding is that we need all of these options to even be able to load the main.js script inside. So my proposal would be to just discuss this in the upstream PR if this needs changes. Is this fine with you?

@lucas-koehler
Copy link

@sgraband Yes that is fine with me. I agree that most likely no changes are necessary there but wanted to point it out just to be sure there is no oversight :)

Added the optional `enableForms` field to `WebviewOptions`.
This allows the `allow-forms` sandbox attribute to be set.

Contributed on behalf of STMicroelectronics
@sgraband sgraband force-pushed the sgr/11517_webview_enable_forms branch from 5b0bf68 to 6e6ea20 Compare December 13, 2022 08:21
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.

3 participants