Handle local resource references in Edge#setText()

[amartya4256]

This contribution fixes Edge browser for win32 to allow serving webpages using setText method where the source code may refer to local resources.

The problem with setText in the modern browser is that webView2 navigates to a page "about:blank" (which is not the part of the intranet) [reference] and sets the html to the DOM. In case the html contains reference to local resource, the browsers throws error "can't load local resource" because of security reasons. This contribution fixes it by navigating edge to a temporary file created in the temporary folder of the OS and hence allowing it to load the local resources, and then sets the html to the DOM directly.

Note: We only need the file to get a concrete local address instead of "about:local" for setText method. We don't perform any I/O to the file itself.

To test the implementation, you need to set the default browser to Edge. To do it in the source code, change org.eclipse.swt.browser.BrowserFactory.createWebBrowser with the following source code:
return new Edge();

Scope not covered:

• This standalone implementation makes the javadoc tooltip open in an edge browser. (Would be covered in a follow up PR, part of eclipse.jdt.ui)
• The caching of intro page (Welcome page) is also not covered by this PR. (Would be covered in a follow up PR, part of eclipse.platform)

contributes to #213

[github-actions]

Test Results

   486 files  ±0     486 suites  ±0   8m 30s ⏱️ -1s
 4 156 tests ±0   4 148 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 378 runs  ±0  16 286 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit 6ab4c66. ± Comparison against base commit 2592408.

♻️ This comment has been updated with latest results.

[Phillipus]

Isn't this the same as #1378?

[amartya4256]

Isn't this the same as #1378?

The PR got quite complicated so I split it into 2 PRs. This is specifically for fixing the set text behaviour and #1378 is for fixing the deadlock issue.

[fedejeanne]

I didn't have time to test this again but the overall state of the code LGTM. All my comments from #1378 have been addressed and I see them addressed in this PR too.

I can't approve it though since I haven't had time to properly test this new PR and I can't know for sure if splitting the commits of #1378 had some side-effects.

[HeikoKlare]

In general, it's great to see that with this fix Edge's setText() works properly, so even Javadoc, Java element information, Welcome page etc. can be displayed.

My central concern is about the introduction of a public BASE_URI as a replacement for the generally used about:blank. Conceptually, I would like to have this hidden as an implementation detail of the browser. I.e., there should not be any magic URL used when setting custom contents via setText() against which consumers compare (like they now do for about:blank. From what I have seen so far, the information demand of consumers is to decide whether a page with some custom text is shown. To this end, the browser could provide an according interface providing the required information while hiding how this is implemented for a specific browser. For WebKit and IE that might be done with the about:blank page, Edge might use a custom base URL, and that implementation detail may even be changed as desired or required without affecting consumers relying on that interface.

As a concrete proposal, some additional method for Browser like an isLocationForCustomText(String location) (may be called differently, accept a URL instead, ...) could be provided. This method could either match the provided location for some known strings (about:blank, BASE_URI, starting with data: or the like) or delegate the decision to the used WebBrowser so that only the specific location for that browser implementation is considered.

Another benefit of that solution is that we could add such a method in a separate PR, adapt the known consumers to use that method, and then have this PR merged for adapting the Edge browser. That would avoid that merging this PR in the current state, Java element links in JDT would be broken when using Edge as the default browser until JDT is adapted to also match the new magic BASE_URI: https://github.com/eclipse-jdt/eclipse.jdt.ui/blob/caf5a4b29592e6d3911a2491aa6b811b52d66f74/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/viewsupport/JavaElementLinks.java#L599-L611

[HeikoKlare]

In order to validate this solution, I've read the according Microsoft documentation for these use cases of Edge browser: https://learn.microsoft.com/en-us/microsoft-edge/webview2/concepts/working-with-local-content

With that information, this still seems to be a proper solution for the problem. I only have some minor comments left.

[sratz]

AFAIK all the other browsers consistently use about:blank as the location for text written via setText().

That means:

• Browser.getext() returns 'about:blank'
• LocationEvent#location contains 'about:blank'

And there are many many code pieces where 'about:blank' is checked.

With this approach this would change, meaning that all consumers would have to adapt and do something like

-if ("about:blank".equals(event.location)) {
+if (browser.isLocationForCustomText(event.location)) {

I was wondering: To maximize compatibility, could we instead hide this fact and fake getUrl() to return 'about:blank' and handle this case internally?

[HeikoKlare]

I was wondering: To maximize compatibility, could we instead hide this fact and fake getUrl() to return 'about:blank' and handle this case internally?

Interesting idea. I would expect that we have already considered that, but I am not sure anymore. This would of course also involve the location listeners to be provided with the faked location. @amartya4256, do you have any insights on this?

With this approach this would change, meaning that all consumers would have to adapt and do something like

-if ("about:blank".equals(event.location)) {
+if (browser.isLocationForCustomText(event.location)) {

That's true. For some essential repos, there are already follow-up PRs to do this:

• Adapted the client to use new Browser API eclipse.platform.ui#2211
• Adapted the client to use new Browser API eclipse.platform#1521
• Adapted the client to use new Browser API eclipse-jdt/eclipse.jdt.ui#1597

In my opinion, independent of whether the proposal of faking the URL behavior works or not, this adaptation should be done. Currently, all consumers rely on the usage of magic values, which will forever be unchangeable. Hiding this behind an API improves the design and makes the behavior exchangeable.

[amartya4256]

I was wondering: To maximize compatibility, could we instead hide this fact and fake getUrl() to return 'about:blank' and handle this case internally?

Interesting idea. I would expect that we have already considered that, but I am not sure anymore. This would of course also involve the location listeners to be provided with the faked location. @amartya4256, do you have any insights on this?

With this approach this would change, meaning that all consumers would have to adapt and do something like

-if ("about:blank".equals(event.location)) {
+if (browser.isLocationForCustomText(event.location)) {

That's true. For some essential repos, there are already follow-up PRs to do this:

• Adapted the client to use new Browser API eclipse.platform.ui#2211
• Adapted the client to use new Browser API eclipse.platform#1521
• Adapted the client to use new Browser API eclipse-jdt/eclipse.jdt.ui#1597

In my opinion, independent of whether the proposal of faking the URL behavior works or not, this adaptation should be done. Currently, all consumers rely on the usage of magic values, which will forever be unchangeable. Hiding this behind an API improves the design and makes the behavior exchangeable.

I agree with @HeikoKlare. The decision to provide an API for this was taken upon the fact that it would abstract away the behaviour of browser, hiding the URLs that clients have to hard code at the moment which in my opinion is a bad practice. It also opens up possibilities for integrating browsers in the future which mgiht have a different behaviour. We can't expect every browser to behave the same, which we saw already between Edge and IE. Also, the way Webkit is implemented, it seems to force itself to comply with how IE behaves (Async to Sync, Open Window events, ABOUT_BLANK url, etc) and that should not be the case. We should have the API for such things where the web browsers can take decisions on their own or execute in their own fashion and provide the appropriate result accordingly.

As mentioned, we already have the PRs to adapt the clients to use the API and new PRs would be following up for WebKit's internal usage of this API as well. As an idea, we can also do the same refactoring for IE.

[laeubi]

Maybe one can combine the approach having a constant for about:blank and adding a hint to the new method where it is relevant, let browsers consistently report this as the location and have a a (default) implemented API method Browser#isLocationForCustomText(event.location)... that way implementers can move to the new methods more smoothly.

[HeikoKlare]

Maybe one can combine the approach having a constant for about:blank and adding a hint to the new method where it is relevant, let browsers consistently report this as the location and have a a (default) implemented API method Browser#isLocationForCustomText(event.location)... that way implementers can move to the new methods more smoothly.

I agree. That's why I asked whether Sebastian's proposal is feasible, so that with that approach we might achieve an implementation of Edge browser that works with existing client implementations for now, but still provide a path to improve the design, i.e., migrate from comparison against magic value about:blank to usage of Brower#isLocationForCustomText(). The only point I do not understand is the constant for about:blank. You would need to migrate to usage of that constant as well, but in that case you could (and should) directly migrate to the new method. Am I missing something here?

[amartya4256]

ntation of Edge browser that works with existing client implementations for now, but still provide a path to improve the design, i.e., migrate from comparison against magic value about:blank to usage of Brower#isLocationForCustomText(). The only point I do not understand is the constant for about:b

It is feasible, but only getUrl would not be enough. All the callbacks in Edge where the event contains a location needs to be validated and mutated if it is a custom text url

[sratz]

In general this looks good. Also, with this PR merged, my PR #1451 will become easier, since no more of those data:text/html; URLs will appear with the physical file URL being used.

Regarding hiding the internal URL and exposing about:blank for compatibility:

@sratz This PR now implements your proposal from #1395 (comment)
What do you think about the revised changes?

There is one more corner case to consider: The application layer consumers will now only see about:blank but they are still encouraged to not compare that literally, but to call Browser#isLocationForCustomText(String).

That means, that this method now also needs to return true for an about:blank argument.

I have one more general remark:
Why is this HTML file a static one? Isn't this problematic when multiple Edge instance exist and all share the same file?

[HeikoKlare]

There is one more corner case to consider: The application layer consumers will now only see about:blank but they are still encouraged to not compare that literally, but to call Browser#isLocationForCustomText(String).

That means, that this method now also needs to return true for an about:blank argument.

Currently, Browser#setLocationForCustomText(String) returns true exactly when passing about:blank into it. Still, we should revise that method (and the discussed provision of such information via the location change event) in subsequent PRs to improve the design. The result of this PR will (only) be that Edge behaves like the other browsers. A follow-up will be to enhance the Browser API to get rid of the comparison against "about:blank" in clients and to adapt the clients accordingly. In order words: this PR would work completely independent from the Browser#isLocationForCustomText(String) method.

I have one more general remark: Why is this HTML file a static one? Isn't this problematic when multiple Edge instance exist and all share the same file?

In my understanding, this file is not used at all. It is just an empty dummy that allows to provide WebView with custom contents of the document to display. The file is never written, so in my opinion it makes sense to share it across all WebView instances.

[sratz]

Currently, Browser#setLocationForCustomText(String) returns true exactly when passing about:blank into it. Still, we should revise that method (and the discussed provision of such information via the location change event) in subsequent PRs to improve the design. The result of this PR will (only) be that Edge behaves like the other browsers. A follow-up will be to enhance the Browser API to get rid of the comparison against "about:blank" in clients and to adapt the clients accordingly. In order words: this PR would work completely independent from the Browser#isLocationForCustomText(String) method.

Oh, right, I was confused by the same name of the private method in Edge and the public method in Browser.

In my understanding, this file is not used at all. It is just an empty dummy that allows to provide WebView with custom contents of the document to display. The file is never written, so in my opinion it makes sense to share it across all WebView instances.

Ah, right; we're only tricking Edge into thinking we are dealing with an actual HTML file on disk, but are then still manipulating the DOM via JavaScript.

[HeikoKlare]

I see all comments addressed and no outstanding complains with respect to this PR.

In case there are no objections, I plan to merge this by the end of the day to have it available for further testing in tomorrow's I-Build.

[sratz]

This change broke working with the DOM in ProgressListeners.

Consider:

public class Demo {

	public static void main(String[] args) {
		Display display = Display.getDefault();
		Shell shell = new Shell();
		shell.setLayout(new FillLayout());
		Browser browser = new Browser(shell, SWT.EDGE);
		browser.setText("""
				<html>
					<head>
						<script src=\"file://c:/test.js\"></script>
					</head>
					<body>
						<h1>Hello, World!</h1>
					</body>
				</html>
				""");
		browser.addProgressListener(ProgressListener.completedAdapter(event -> {
			String script = """
					var h1s = document.getElementsByTagName("h1");
					alert("Found h1s: " + h1s.length);
					"""; //$NON-NLS-1$
			browser.execute(script);
		}));

		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}
		display.dispose();
	}

}

This should bring up a Found h1s: 1 alert, but actually shows Found h1s: 0.

The debugging tools show:

VM1955:1 [Violation] Avoid using document.write().
VM1955:1 [Violation] Parser was blocked due to document.write(<script>)
VM1955:1 [Violation] Parser was blocked due to document.write(<script>)

Changing

-execute("document.open(); document.write(`" + lastCustomText + "`); document.close();");
+execute("document.documentElement.innerHTML = `" + lastCustomText + "`;");

fixes the problem