JDK-8157686 : JavaFX WebView fails to track URL changes for PJAX websites
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8,9
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: x86
  • Submitted: 2016-05-11
  • Updated: 2018-06-26
  • Resolved: 2017-09-27
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 8
10Fixed 8u171Fixed
Related Reports
Blocks :  
Duplicate :  
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.8.0_92"
Java(TM) SE Runtime Environment (build 1.8.0_92-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.92-b14, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows [Version 10.0.10586]

Darwin deju-mbpro.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016; root:xnu-3248.40.184~3/RELEASE_X86_64 x86_64

A DESCRIPTION OF THE PROBLEM :
JavaFX's WebView component fails to detect URL location changes for websites using PJAX.

For example, I found these two webpages that use PJAX: ghosttunes.com and thesimplefact.org. If you inspect the requests as you browse these websites, you'll see they're using PJAX as you navigate from page to page. WebView successfully loads and renders these pages, but WebEngine's location property fails to fire events to any attached change listeners.


STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
1) Create a JavaFX WebView
2) Add a ChangeListener to WebEngine's location property and print the URL location changes
3) Navigate to a site using PJAX like https://ghosttunes.com/ or http://thesimplefact.org/
4) Navigate to other pages within the site.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
The location property ChangeListener should fire.
ACTUAL -
The location property ChangeListener did not fire.

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
import javafx.application.Application;
import javafx.scene.Group;
import javafx.scene.Scene;
import javafx.scene.web.WebEngine;
import javafx.scene.web.WebView;
import javafx.stage.Stage;

/**
 * @author Dennis Ju
 */
public class TestWebView2 extends Application {

	@Override
	public void start(Stage stage) {
		stage.setWidth(800);
		stage.setHeight(600);

		WebView webView = new WebView();
		WebEngine webEngine = webView.getEngine();
		Scene scene = new Scene(new Group());
		scene.setRoot(webView);
		stage.setScene(scene);
		stage.show();

		webEngine.locationProperty().addListener(
			(observableValue, oldValue, newValue) -> {
				// This listener does not fire for requests using PJAX

				System.out.println(newValue);
			});

		webEngine.load("https://ghosttunes.com");
	}

	public static void main(String[] args) {
		launch(args);
	}
}
---------- END SOURCE ----------


Comments
Changeset: f693e281ade9 Author: ghb Date: 2017-09-27 11:21 +0530 URL: http://hg.openjdk.java.net/openjfx/10-dev/rt/rev/f693e281ade9
27-09-2017

+1
26-09-2017

+1
25-09-2017

Updated webrev http://cr.openjdk.java.net/~ghb/8157686/webrev.12/ As you noticed "dispatchDidNavigateWithinPage" is called from only one place i.e from FrameLoader::loadInSameDocument. This is clear that there is a loader exist for this current frame. For a null document there won't be any navigate and there is no way for a frame which doesn't have document i.e null. Keeping Null check as compared to other place will be redundent here. 2nd, 3rd and incorporated in with webrev.12 I have created JDK-8187800 to address 4th review comment.
22-09-2017

+1
20-09-2017

Its not intentional, I didn't do hg add for the new file. Updated webrev : http://cr.openjdk.java.net/~ghb/8157686/webrev.11/
07-09-2017

Looks like test is missing in the latest webrev, is it intentional?
07-09-2017

Rebased Webrev for 10-dev : http://cr.openjdk.java.net/~ghb/8157686/webrev.10/ Test : Load PJAX based webpage in HelloWebView e.g https://demo.tutorialzine.com/2009/09/simple-ajax-website-jquery/demo.html and perform site navigation and observe the URL update.
29-08-2017

Updated webrev http://cr.openjdk.java.net/~ghb/8157686/webrev.09/ (rebased)
09-02-2017

The latest .08 patch needs to be rebased against the tip of rt, since it has a merge conflict in one of the files (two, actually, but one is just whitespace).
02-02-2017

+1 (im ok with all webrev's)
25-11-2016

[~arajkumar] There are multiple concerns from review comments. Please update which webrev is good to go and the same has to be agreed by others to.
21-11-2016

Lowering Prioritising to tbd_major as problem seen on 8 GA, 8u60 and 9.
21-11-2016

+1
21-11-2016

"#2 For both historyPushStateTest and historyReplaceStateTest, it is better to check "history.length" also, for the former, "history.length" will be incremented for each call and will remain same for later." "1. You can delete the empty line after" above two are incorporated in webrev.05, But remaining can't be done with single change, Please consider all the patch and updated your views. http://cr.openjdk.java.net/~ghb/8157686/webrev.05/ Changes done : replaced : private int historyListenerIndex; (Member) and used AtomicInteger (local). Removed extra empty line as suggested. http://cr.openjdk.java.net/~ghb/8157686/webrev.06/ w.r.t JDK-8151459, This bug was only to address URL change wasn't happening for pushState and replace state. Fix on the native "FrameLoaderClientJava::dispatchDidPushStateWithinPage()" and "FrameLoaderClientJava::dispatchDidReplaceStateWithinPage()" which intern updates WebEngine.location(). This will test for push, replace, state and length, ignoring go and back. http://cr.openjdk.java.net/~ghb/8157686/webrev.07/ Incorporating go and back requires asynchronous WebEngine.location listener call back. This will violate one of the previous review comment and it won't be as similar to JDK-8151459 where its used for single test and managed the lifecycle using @before and @after. If we need to cover all the history operation over javascript i.e push, replace, go, back and state and should be as JDK-8151459, then please consider http://cr.openjdk.java.net/~ghb/8157686/webrev.08/ which is similar to 8151459. Incorporated review comments for "3. You can keep below line as private member variable" "4. You can keep default case for switch (historyListenerIndex) in historyBackAndStateTest and historyGoTest where you can throw exception ?" "Since these cases are related to history push states, i suggest you can create a new file and can add these new test cases."
17-11-2016

Since these cases are related to history push states, i suggest you can create a new file and can add these new test cases. Regarding @Before and @After, You can refer the test case done for JDK-8151459.
16-11-2016

[~ghb] 3. Please provide a valid context, how , when and why we need @Before and @After You can refer this link on when to use: http://junit.sourceforge.net/javadoc/org/junit/Before.html 4. You can keep default case for switch (historyListenerIndex) in historyBackAndStateTest and historyGoTest where you can throw exception ? historyBackAndStateTest : Switch (historyListenerIndex) historyListenerIndex is initialized with -1 serves '0' and '1'. In '1' --> latch.countDown();, There is not way i can provide a default case with Failure. Adding this will lead to dead code. historyGoTest: same as explained above. in this case you can keep just "if , else " instead of switch. If you use switch, then default is recommended.
16-11-2016

1. Switch is little faster than if else : Adding default and with the current states (Which increments from 0 and N where we exit the block), it will be a useless code (dead code) which will never reach. I still don't understand why default case is needed ? 2. My Initial commit was implemented with if else , which was hard to understand and review took more time to understand.
16-11-2016

Thanks Murali & Arun for your valuable comments. Please suggest further Agreed with 1 and 2 3. You can keep below line as private member variable final CountDownLatch latch = new CountDownLatch(1); > By making member variable can fufill only one test case. Please provide a viable solution with private member variable and can reuse the latch in more than one place. and you can think of keeping latch.await(30, TimeUnit.SECONDS); and latch.countDown(); in @Before and @After which gets executed after every test. > Please provide a valid context, how , when and why we need @Before and @After 4. You can keep default case for switch (historyListenerIndex) in historyBackAndStateTest and historyGoTest where you can throw exception ? historyBackAndStateTest : Switch (historyListenerIndex) historyListenerIndex is initialized with -1 serves '0' and '1'. In '1' --> latch.countDown();, There is not way i can provide a default case with Failure. Adding this will lead to dead code. historyGoTest: same as explained above. 5. looks like url0 and url1 are same in each test case, then you can move these 2 variables as private final member variables? One of the comment says minimize private member. Which one to use now ? [~arajkumar] Do you agree to have url0 and url1 (with valid name) as a member variable ?
16-11-2016

1. You can delete the empty line after 369 } finally { 370 historyStates.forEach(state -> 371 assertTrue(state.script + " : Failed", state.succeeded)); 372 373 } 2. Can you make below non-hard coded latch.await(30, TimeUnit.SECONDS); 3. You can keep below line as private member variable final CountDownLatch latch = new CountDownLatch(1); and you can think of keeping latch.await(30, TimeUnit.SECONDS); and latch.countDown(); in @Before and @After which gets executed after every test. 4. You can keep default case for switch (historyListenerIndex) in historyBackAndStateTest and historyGoTest where you can throw exception ? 5. looks like url0 and url1 are same in each test case, then you can move these 2 variables as private final member variables?
16-11-2016

Few more comments: #1 + private int historyListenerIndex; + Need not be a field, declare it as a local variable for each test case which uses this. #2 For both historyPushStateTest and historyReplaceStateTest, it is better to check "history.length" also, for the former, "history.length" will be incremented for each call and will remain same for later.
16-11-2016

I missed a point, variable can't be altered inside a lamda.(it should be effectively final) So you please ignore my first comment, else you could try using AtomicInteger which comes with methods to increment and decrement.
16-11-2016

Update webrev : http://cr.openjdk.java.net/~ghb/8157686/webrev.04/ All 5 history.* api's are tested considering well documented description from https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onpopstate. Agreed that its hard to understand, now i have split history state into multiple method (which is less easier to understand and with updated comments on each state). 1. class HistoryState can be a inner class inside a test method. Kept outside the scope considering the use in future, With updated webrev, This class is used in multiple methods (Test). 2. "private int historyStateIndex = -1;" can be a static member inside class HistoryState, since it's scope is limited only to this testcase This can't be done due to "Inner class cannot have static declaration" (Compilation error). history.pushState(obj, title, newURL) history.replaceState(obj, title, newURL) Above two will navigate page in forward direction and contains 'obj' which can / will be queried with 'history.state'. Title is empty for the HistoryState object due to considering the current context of the bug, I intend not to add to many assert which caused confusion. Please ref : https://developer.mozilla.org/en-US/docs/Web/API/History_API for these below api details. history.back() history.go(N) history.state
11-11-2016

The new changset is bit hard to follow, I'm fine with it if other reviewer agrees to go with webrev.03. Few comments on webrev.03, 1. class HistoryState can be a inner class inside a test method. 2. "private int historyStateIndex = -1;" can be a static member inside class HistoryState, since it's scope is limited only to this testcase
10-11-2016

Updated webrev http://cr.openjdk.java.net/~ghb/8157686/webrev.03 Will catch all the states and also for initial load fail.
10-11-2016

pjax works by grabbing html from your server via ajax and replacing the content of a container on your page with the ajax'd html (Source: PJAX https://github.com/defunkt/jquery-pjax ). Root cause : During navigation (Without page reload / current mainframe not replaced), Location listerner were not update. Current Junit test loads valid page and operate on different history states i.e push, replace, back and go. We need backend server and content with pjax library (similar mechanism) to fully simulate the test behaviour. 1. What if file load fails? "archive-root0.html" is only resource which will be loaded and respective "SUCCEEDED" will follow on the history state manipulation which intern updates the Location listener. 2. What if the below block executed immediately after a load? } else if(oldValue.endsWith(page0) && newValue.endsWith(page3)) { + latch.countDown(); + } Continuing from Q1. There will be only one SUCCEEDED state. Please ref my previous comment keeping note of execution of history.pushState and history.replaceState are synchronous (call to location listener update), where as history.back and history.go are asynchronous. else if block mentioned above is the last location update. Concerning area is if we change the order of history state of if we have any bugs from in native! I have two solution to over come the concern. Opt1: Split this test into 4 individual unit test which does tests in isolation. Opt2: Have CountDownLatch with 3 and count down in all three branch, Even this will not clear your concern as which block executed in which order. Suggest any of above or any alternate way ?
09-11-2016

I'm concerned about putting test validation inside a branching, somehow we are missing few conditions which can lead to timeout. e.g. 1. What if file load fails? 2. What if the below block executed immediately after a load? } else if(oldValue.endsWith(page0) && newValue.endsWith(page3)) { + latch.countDown(); + } If you expect a test sequence, then it is better to put a counter, which you can test on each branch to ensure the sequence goes on the right track.
08-11-2016

Updated webrev : http://cr.openjdk.java.net/~ghb/8157686/webrev.02 All the listeners are proporated (Sequence of execution mentioned below). Assert message is ignored and only timeout is shown. Removed the message part in assert in the updated webrev. test.javafx.scene.web.HistoryTest > historyOnPopStateTest FAILED java.lang.Exception: test timed out after 30000 milliseconds Script execution: Load1 : webEngine.load("archive-root0.html") Script1 : webEngine.executeScript("history.pushState("{page: 1}, "title 1", "?archive-root1.html");"); Script2 : webEngine.executeScript("history.pushState("{page: 2}, "title 2", "?archive-root2.html");"); Script3 : webEngine.executeScript("history.replaceState("{page: 3}, "title 1", "?longselectorlist.html");"); Script4 & Script 5: webEngine.executeScript("history.pushState("history.back();"); Script 6 : webEngine.executeScript(history.go(2);"); Sequence of Execution and callbacks Load1: Location : Old == NULL & New == archive-root0.html No further assertion / script execution , return State : SUCCEEDED Script1: Location Old = archive-root0.html & New = archive-root0.html?archive-root1.html No assertion / script execution , return assert Page1 is loaded Script2: Location Old = archive-root0.html?archive-root1.html & New = archive-root0.html?archive-root2.html No assertion / script execution , return assert Page2 is loaded Script3: Location Old = archive-root0.html?archive-root2.html & New = archive-root0.html?longselectorlist.html No assertion / script execution , return assert Page 3 is loaded Script4: Location Old = archive-root0.html?longselectorlist.html & New = archive-root0.html?archive-root1.html Script4: Location Old = archive-root0.html?archive-root1.html & New = archive-root0.html Script6: Location Old = archive-root0.html & New = longselectorlist.html latch.countDown(); //TEST ENDS Had implemented same states as discussed in MDN https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onpopstate
07-11-2016

I don't think that the asserts inside the listeners will be propagated to your test, so might not be effective (although it might cause be printed and also cause the test to timeout, so may be OK). Have you checked what happens when one of the asserts fails?
05-11-2016

Updated webrev : http://cr.openjdk.java.net/~ghb/8157686/webrev.01/ For all the asserts i would be comparing with the last words (Either query string or file name), Instead of getting url substring used String.endsWith(). URL : <scheme>://<authority><path>?<query>#<fragment> url inputs : u1: file:///Users/GHB/dev/openjfx/8u-dev/rt/.idea/out/test/web/test/html/archive-root0.html u2: file:///Users/GHB/dev/openjfx/8u-dev/rt/.idea/out/test/web/test/html/archive-root0.html?archive-root1.html For u1 comparing with file name (without query string) requires either substring or split. Instead String.endsWith() is enough to all the asserts used. Using URL has a l URL oldUrl = new URL(oldValue); URL newUrl = new URL(newValue); if (oldUrl.getQuery() != null && oldUrl.getQuery().equalsIgnoreCase(page3) && newUrl.getQuery().equalsIgnoreCase(page1)) { webEngine.executeScript("history.back();"); } else if (oldUrl.getQuery() != null && oldUrl.getQuery().equalsIgnoreCase(page1) && newValue.substring(newValue.lastIndexOf('/') + 1).equalsIgnoreCase(page0)) { assertNull("history.state should be null ", webEngine.executeScript("history.state")); webEngine.executeScript("history.go(2);"); } else if (oldValue.substring(oldValue.lastIndexOf('/') + 1).equalsIgnoreCase(page0) && newUrl.getQuery() != null && newUrl.getQuery().equalsIgnoreCase(page3)) { latch.countDown(); }
04-11-2016

lgtm with nit. >> oldValue.substring(oldValue.lastIndexOf('?') + 1). Try to use URL class to extract path info instead of above cryptic code.
02-11-2016

Webrev : http://cr.openjdk.java.net/~ghb/8157686/webrev.00/ Root cause : WebEngine URL was not updated for history.pushState, history.replaceState. Fix : Update the URL for Frameloader callback "dispatchDidPushStateWithinPage" and "dispatchDidReplaceStateWithinPage". Info : Sites which is described in description uses pjax (external jquery library Ref http://pjax.herokuapp.com/). During page navigation sub pages are loaded from xmlhttpreqeust and main page is not reloaded (History will work as expected). JUnit : For actual testing with page navigation and URL update we need http server which responds to xmlhttpreqeust (which works as expected). History push and replace state is tested as explained in https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onpopstate.
28-10-2016

WebEngine.locationProperty() listener will be called if there is change in the WebEngine main page location. For the given URL "https://ghosttunes.com" and "http://thesimplefact.org/" Main page url is not changed (Re-directed or moved) to different location. Same behavior exhibits in Firefox and Chrome. To confirm the locationProperty behavior please test web page which has re-direct (e.g https://www.google.com --> https://www.google.co.in or to region specific domain)
25-05-2016