From 932b4a25998373171012afae08a02a9ab0b56607 Mon Sep 17 00:00:00 2001 From: Sebastian Ratz Date: Thu, 5 Sep 2024 19:48:24 +0100 Subject: [PATCH] Edge: Improve handling of LocationListener#changed() Previously, the changed() method was not called consistently. Move the firing of 'changed' LocationEvents in Edge from handleSourceChanged() to handleNavigationCompleted(). Add a test case to Test_org_eclipse_swt_browser_Browser for this scenario (setText() twice). --- .../win32/org/eclipse/swt/browser/Edge.java | 54 +++++++++++-------- .../Test_org_eclipse_swt_browser_Browser.java | 13 +++++ 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java b/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java index d9892ac8c97..fc427cfe658 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java +++ b/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java @@ -666,9 +666,10 @@ int handleNavigationStarting(long pView, long pArgs, boolean top) { listener.changing(event); if (browser.isDisposed()) return COM.S_OK; } + // Save location and top for all events that use navigationId. + // will be eventually cleared again in handleNavigationCompleted(). + navigations.put(pNavId[0], event); if (event.doit) { - // Save location and top for all events that use navigationId. - navigations.put(pNavId[0], event); jsEnabled = jsEnabledOnNextPage; settings.put_IsScriptEnabled(jsEnabled); // Register browser functions in the new document. @@ -690,23 +691,13 @@ int handleSourceChanged(long pView, long pArgs) { // to an empty string. Navigations with NavigateToString set the Source // to about:blank. Initial Source is about:blank. If Source value // is the same between navigations, SourceChanged isn't fired. - // TODO: emit missing location changed events - long[] ppsz = new long[1]; - int hr = webView.get_Source(ppsz); - if (hr != COM.S_OK) return hr; - String url = getExposedUrl(wstrToString(ppsz[0], true)); - browser.getDisplay().asyncExec(() -> { - if (browser.isDisposed()) return; - LocationEvent event = new LocationEvent(browser); - event.display = browser.getDisplay(); - event.widget = browser; - event.location = url; - event.top = true; - for (LocationListener listener : locationListeners) { - listener.changed(event); - if (browser.isDisposed()) return; - } - }); + // Calling LocationListener#changed() was moved to + // handleNavigationCompleted() to have consistent/symmetric + // LocationListener.changing() -> LocationListener.changed() behavior. + // TODO: #fragment navigation inside a page does not result in + // NavigationStarted / NavigationCompleted events from WebView2. + // so for now we cannot send consistent changing() + changed() events + // for those scenarios. return COM.S_OK; } @@ -795,11 +786,32 @@ int handleNavigationCompleted(long pView, long pArgs, boolean top) { long[] pNavId = new long[1]; args.get_NavigationId(pNavId); LocationEvent startEvent = navigations.remove(pNavId[0]); - if (webView_2 == null && startEvent != null && startEvent.top) { - // If DOMContentLoaded isn't available, fire + if (startEvent == null) { + // handleNavigationStarted() always stores the event, so this should never happen + return COM.S_OK; + } + if (webView_2 == null && startEvent.top) { + // If DOMContentLoaded (part of ICoreWebView2_2 interface) isn't available, fire // ProgressListener.completed from here. sendProgressCompleted(); } + int[] pIsSuccess = new int[1]; + args.get_IsSuccess(pIsSuccess); + if (pIsSuccess[0] != 0) { + browser.getDisplay().asyncExec(() -> { + if (browser.isDisposed()) return; + LocationEvent event = new LocationEvent(browser); + event.display = browser.getDisplay(); + event.widget = browser; + event.location = startEvent.location; + event.top = startEvent.top; + for (LocationListener listener : locationListeners) { + listener.changed(event); + if (browser.isDisposed()) return; + } + }); + } + return COM.S_OK; } diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java index ecb7c833519..700b58811ce 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java +++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_browser_Browser.java @@ -606,6 +606,19 @@ public void test_LocationListener_changed() { boolean passed = waitForPassCondition(changedFired::get); assertTrue("LocationListener.changed() event was never fired", passed); } +@Test +public void test_LocationListener_changed_twoSetTextCycles() { + AtomicInteger changedCount = new AtomicInteger(); + browser.addLocationListener(changedAdapter(e -> changedCount.incrementAndGet())); + shell.open(); + browser.setText("Hello world"); + boolean passed = waitForPassCondition(() -> changedCount.get() == 1); + assertTrue("LocationListener.changed() event was never fired", passed); + browser.setText("2nd text"); + passed = waitForPassCondition(() -> changedCount.get() == 2); + assertTrue("LocationListener.changed() event was not fired for the 2nd text change", passed); +} + @Test public void test_LocationListener_changingAndOnlyThenChanged() { // Test proper order of events.