Fix links opening when embedded in VSCode #3252
Conversation
|
|
||
| import 'dart:html'; | ||
|
|
||
| final isVSCodeEmbedded = window.navigator.userAgent.contains('Electron'); |
There was a problem hiding this comment.
It might be nice to have this somewhere more shared and use it here too:
There was a problem hiding this comment.
Would it better to inject a param from DartCode, to make sure it's running in VSCode? I guess it's possible there would be some other Electron-based browser running DevTools
There was a problem hiding this comment.
In the other code (where keystrokes are passed up) this check was to prevent other websites hosting DevTools and enabling this (see #2775 (comment)). It's probably less of a concern here, though it might be good to be consistent (I'll defer to @jacob314!).
There was a problem hiding this comment.
There is also the existing query parameter passed in that indicates whether to use the embedded version of DevTools. That could be reasonable to use in combination with checking for electron to ensure that we only show in VSCode.
DanTup
left a comment
There was a problem hiding this comment.
@jacob314 it feels a little awkward leaking more VS Code details into DevTools, but I can't think of a better way to do this. VS Code's iframes are sandboxes and can't open new windows. VS Code attaches click handlers to normal webviews and does this itself, but since DevTools runs inside another iframe inside that, it can't attach a handler to them (same reason we have to send the key events up too).
One option could be to try to eliminate the webview (this would mean injecting the raw index HTML into the webview and setting a base href so the links are all correct), but I suspect that might not play nice with routing etc.
|
|
||
| Future<void> launchUrl(String url, BuildContext context) async { | ||
| if (isVSCodeEmbedded) { | ||
| launchUrlVSCode(url); |
There was a problem hiding this comment.
can we call launchUrlVSCode(url) in addition to rather than instead of the regular url launch call? My impression is that wouldn't be a problem for vscode as popups are blocked anyway. That way if we are wrong about an app actually being VSCode, it won't be a problem as the url will still launch if it can.
There was a problem hiding this comment.
Yep, done. Have tested in VSCode and in browser and this solution works.
I agree it is a bit awkward but I think it is simpler than the alternatives. As long as we only trigger the custom behavior for the embedded version of DevTools or if we continue to dispatch the regular browser open request I think there is low risk. |
| final isVSCodeEmbedded = window.navigator.userAgent.contains('Electron'); | ||
|
|
||
| void launchUrlVSCode(String url) { | ||
| window.parent?.postMessage({'command': 'launchUrl', 'data': url}, '*'); |
There was a problem hiding this comment.
To mirror my comment in the Dart-Code repo, I think it's better if this is 'data': { 'url': url } so that if we need to pass additional data through in future it's not a breaking change (from a string to a map/object).
ffbc018 to
1ca9d39
Compare
|
@jacob314 this looks good to me and I plan to merge the VS Code part. I'll let you check/merge this if you're otherwise happy. |
Fixes #3251
Buddy PR on DartCode: Dart-Code/Dart-Code#3517