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

Add support for DevTools to open external links #3517

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

tomgilder
Copy link
Contributor

@tomgilder tomgilder commented Aug 5, 2021

See flutter/devtools#3251 - external links are currently failing when DevTools are embedded in VSCode due to the "allow-popups" sandbox restriction being enabled.

There might be a simpler solution to this that I'm unaware of.

I'm not very experienced with VSCode/TypeScript development, please check carefully 😁

Especially not sure if I got the dispose logic right.

Buddy PR on DevTools: flutter/devtools#3252

Copy link
Member

@DanTup DanTup left a comment

Choose a reason for hiding this comment

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

I added a note on flutter/devtools#3252 (review). If we can't come up with something better and DevTools is happy to have that change, I think this looks reasonable. It would be good to avoid leaking too much VS Code stuff into DevTools if we can help it though.

src/extension/sdk/dev_tools/embedded_view.ts Show resolved Hide resolved
Copy link
Member

@DanTup DanTup left a comment

Choose a reason for hiding this comment

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

I think there's one change we should make in case we need to extend in future, but otherwise looks good to me - thanks!

src/extension/sdk/dev_tools/embedded_view.ts Show resolved Hide resolved
src/extension/sdk/dev_tools/embedded_view.ts Show resolved Hide resolved
@DanTup
Copy link
Member

DanTup commented Aug 16, 2021

Thanks! I'm OOO this week, but will check the dispose is all good next week and merge this for the next version :-)

@DanTup DanTup merged commit 7b92995 into Dart-Code:master Aug 24, 2021
@DanTup DanTup added this to the v3.26.0 milestone Aug 24, 2021
@DanTup
Copy link
Member

DanTup commented Aug 24, 2021

This all looks good to me, and seems to work fine. It does trigger an error because of the change in DevTools to also call allow the original event to be handled, but I think I agree with Jacob that it's better to always call that. It's unlikely the sandbox rules will change, so it should never trigger to windows being opened, and the error is explainable.

Thanks!

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.

2 participants