Skip to content

testBodyReplace #1463

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.

Sign up for GitHub

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

Draft
rubenporras wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −3
Draft

testBodyReplace #1463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion org.eclipse.lsp4e/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Export-Package: org.eclipse.lsp4e;x-friends:="org.eclipse.lsp4e.debug,org.eclips
org.eclipse.lsp4e.ui
Bundle-Vendor: Eclipse LSP4E
Import-Package: com.google.common.base,
com.google.gson;version="2.7.0"
com.google.gson;version="2.7.0",
org.apache.commons.lang;version="[2.6.0,3.0.0)"
Copy link
Contributor Author

@rubenporras rubenporras Dec 10, 2025

Choose a reason for hiding this comment

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

This should rather be a require-plugin, and the plugin should be added to the target file by referencing Orbit.

Automatic-Module-Name: org.eclipse.lsp4e
Service-Component: OSGI-INF/org.eclipse.lsp4e.format.DefaultFormatRegionsProvider.xml
22 changes: 20 additions & 2 deletions ...ipse.lsp4e/src/org/eclipse/lsp4e/operations/hover/FocusableBrowserInformationControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
*******************************************************************************/
package org.eclipse.lsp4e.operations.hover;

import java.lang.reflect.Field;
import java.net.URL;
import java.util.Objects;
import java.util.UUID;

import org.apache.commons.lang.StringEscapeUtils;
import org.eclipse.core.runtime.FileLocator;
import org.eclipse.core.runtime.Platform;
import org.eclipse.jdt.annotation.Nullable;
Expand Down Expand Up @@ -93,6 +96,13 @@ protected void createContent(Composite parent) {
b.setJavascriptEnabled(true);
}


public void setTextInBrowser(final Browser browser, final String text) {
String escapedHtmlBody = StringEscapeUtils.escapeJavaScript(text);
safeExecute(browser, "document.body.innerHTML = \"" + escapedHtmlBody + "\";"); //$NON-NLS-1$//$NON-NLS-2$
Copy link
Contributor Author

@rubenporras rubenporras Dec 10, 2025

Choose a reason for hiding this comment

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

I think this should work but I was not able to see the updated content on the hover. I did not have time to check if the problem is that the javascript is not correct (I think it is because we have this very same code in production for months without issues), or if it is that I would need to also do something in the internals on the base class (for example override the fInput field as well), or that my Language Server was having problems (I found many of those in my LS, but I did not had time to go back and this PR to double check).

updateBrowserSize(browser);
}

private void updateBrowserSize(final Browser browser) {
if (getShell().isDisposed() || browser.isDisposed() || getInput() == null)
return;
Expand All @@ -102,7 +112,7 @@ private void updateBrowserSize(final Browser browser) {
Point hint = computeSizeHint();
setSize(hint.x, hint.y);

if (!"complete".equals(safeEvaluate(browser, "return document.readyState"))) { //$NON-NLS-1$ //$NON-NLS-2$
if (!"complete".equals(safeEvaluate(browser, "if (document.documentElement === null) {return 'no content';} return document.readyState;"))) { //$NON-NLS-1$ //$NON-NLS-2$
Copy link
Contributor Author

@rubenporras rubenporras Dec 10, 2025

Choose a reason for hiding this comment

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

This I think should not be needed with this approach.

UI.getDisplay().timerExec(200, () -> updateBrowserSize(browser));
return;
}
Expand Down Expand Up @@ -183,7 +193,15 @@ public void setInput(@Nullable Object input) {
return;
}
if (html != null && !html.isBlank()) {
super.setInput(styleHtml(html));
try {
Field f = BrowserInformationControl.class.getDeclaredField("fBrowser"); //$NON-NLS-1$
Copy link
Contributor Author

@rubenporras rubenporras Dec 10, 2025

Choose a reason for hiding this comment

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

I do not like to do this, but the base class we are using is not good for customizability. Indeed the documentation clearly says it should not be subclassed, which we are ignoring.

I have another test where I do not use that class at all, but we would need to replicate some code which is a bit scary. I can submit that PR as well if you think it is useful.

f.setAccessible(true);
Browser browser = (Browser) f.get(this);
Objects.requireNonNull(browser);
setTextInBrowser(browser, html);
} catch (NoSuchFieldException | SecurityException | IllegalArgumentException | IllegalAccessException e) {
super.setInput(html);
Copy link
Contributor Author

@rubenporras rubenporras Dec 10, 2025

Choose a reason for hiding this comment

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

should never happen, but it is just a safety net. We should add some logging in this case.

}
} else {
// No content from LS; hide placeholder
super.setInput(""); //$NON-NLS-1$
Expand Down
Loading