Fix Edge Browser Set Text Issue

[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[bot]]

Test Results

   486 files  ±0     486 suites  ±0   8m 30s :stopwatch: -1s  4 156 tests ±0   4 148 :white_check_mark: ±0   8 :zzz: ±0  0 :x: ±0  16 378 runs  ±0  16 286 :white_check_mark: ±0  92 :zzz: ±0  0 :x: ±0 

Results for commit 6ab4c66a. ± Comparison against base commit 25924084.

:recycle: This comment has been updated with latest results.

[Phillipus]

Isn't this the same as https://github.com/eclipse-platform/eclipse.platform.swt/pull/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.

[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:

• https://github.com/eclipse-platform/eclipse.platform.ui/pull/2211
• https://github.com/eclipse-platform/eclipse.platform/pull/1521
• https://github.com/eclipse-jdt/eclipse.jdt.ui/pull/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.

[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