Menu
▾
▴
4 Attachments
#1984 Reworking the JS Event listeners implementation
Problem in brief
Thorough testing has revealed event listeners in HtmlUnit are implemented quite liberally in a few places with regards to how they should work. It's somewhat surprising they're working so well in practice.
This bug report attempts to address the problems and tackle at fixing them. Scope includes xyz.addListenerEvent('foo', function () { ... }) listeners and xyz.onfoo = function () { ... } property handlers.
Problems noticed
- Ordering of listeners called during the at target phase should honour the order in which the listeners were added, but does not.
- Ordering of property handlers such as
window.onloadshould honour the timing at which the property was set, with respect to other event listeners, but does not. - Events where
event.bubblesisfalseshould not have a bubbling phase, but does. - The load event for
Windowshould only traverseWindowbut traversesWindow, Documentinstead. - The load event for everything else should traverse from
Documentto all levels down to the element in question, but traverses the element in question only. (Every place usingEventTarget.executeEventLocally()potentially applies to this and should probably be using the standardEventTarget.fireEvent()instead.) - The load event for the
<frame>element should be treated like "everything else" above, but is not. (On the other hand, theonloadproperty for<body>and<frameset>is correctly tied toWindow.) - The propagation path of events should not be affected by changes to the DOM tree by intermediate listeners, and therefore the bubbling phase should always traverse the same nodes as the capturing phase only in reverse, but is and does not.
- The return value of event listeners should be ignored (really? it looks that way.. maybe it was different in older IE) and only that of the property handler should be used, but is not.
Test cases
There are five tests which I'm putting in the comments because it's quite long.
test_onload.htmltest_frame.htmltest_click.htmltest_nested_click.htmltest_event_return_value.html
These extra two I'm including but haven't fixed yet. They're limited to the load/error behaviour of <script> and <img> and require fixing up some implementation in HtmlScript and HtmlImage.
test_script_onload.htmltest_img_onload.html
Possible fix
The fix spans three files:
html/HtmlPage.javajavascript/host/event/EventListenersContainer.javajavascript/host/event/EventTarget.java
Changes to HtmlPage is the diff below.
The changes for EventListenersContainer.java and EventTarget.java are rather extensive so I'm attaching the modified files along with their respective base versions suffixed with -2.32.txt.
--- a/src/main/java/com/gargoylesoftware/htmlunit/html/HtmlPage.java
+++ b/src/main/java/com/gargoylesoftware/htmlunit/html/HtmlPage.java
@@ -86,15 +86,16 @@
import com.gargoylesoftware.htmlunit.javascript.PostponedAction;
import com.gargoylesoftware.htmlunit.javascript.SimpleScriptable;
import com.gargoylesoftware.htmlunit.javascript.host.Window;
-import com.gargoylesoftware.htmlunit.javascript.host.dom.Node;
import com.gargoylesoftware.htmlunit.javascript.host.event.BeforeUnloadEvent;
import com.gargoylesoftware.htmlunit.javascript.host.event.Event;
+import com.gargoylesoftware.htmlunit.javascript.host.event.EventTarget;
import com.gargoylesoftware.htmlunit.javascript.host.html.HTMLDocument;
import com.gargoylesoftware.htmlunit.protocol.javascript.JavaScriptURLConnection;
import com.gargoylesoftware.htmlunit.util.EncodingSniffer;
import com.gargoylesoftware.htmlunit.util.UrlUtils;
import net.sourceforge.htmlunit.corejs.javascript.Context;
+import net.sourceforge.htmlunit.corejs.javascript.ContextFactory;
import net.sourceforge.htmlunit.corejs.javascript.Function;
import net.sourceforge.htmlunit.corejs.javascript.Script;
import net.sourceforge.htmlunit.corejs.javascript.Scriptable;
@@ -1214,18 +1215,28 @@ private boolean executeEventHandlersIfNeeded(final String eventType) {
// Execute the specified event on the document element.
final WebWindow window = getEnclosingWindow();
if (window.getScriptableObject() instanceof Window) {
- final DomElement element = getDocumentElement();
- if (element == null) { // happens for instance if document.documentElement has been removed from parent
- return true;
- }
+ // We need the 'Document' node for these load events but getDocumentElement() returns
+ // <html> (HtmlHtml) which is one below that. The 'Document' node is incidentally just
+ // us (HtmlPage). (Some tidbits at https://www.w3.org/TR/DOM-Level-3-Events/#event-flow)
+ final DomNode node = this;
final Event event;
if (eventType.equals(Event.TYPE_BEFORE_UNLOAD)) {
- event = new BeforeUnloadEvent(element, eventType);
+ event = new BeforeUnloadEvent(node, eventType);
}
else {
- event = new Event(element, eventType);
+ event = new Event(node, eventType);
+ }
+
+ // This is the same as DomElement.fireEvent() and was copied
+ // here so it could be used with HtmlPage.
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Firing " + event);
}
- final ScriptResult result = element.fireEvent(event);
+
+ final EventTarget jsNode = node.getScriptableObject();
+ final ContextFactory cf = ((JavaScriptEngine) getWebClient().getJavaScriptEngine()).getContextFactory();
+ final ScriptResult result = cf.call(cx -> jsNode.fireEvent(event));
+
if (!isOnbeforeunloadAccepted(this, event, result)) {
return false;
}
@@ -1253,7 +1264,12 @@ private boolean executeEventHandlersIfNeeded(final String eventType) {
else {
event = new Event(frame, eventType);
}
- final ScriptResult result = ((Node) frame.getScriptableObject()).executeEventLocally(event);
+
+ // This fires the "load" event for the <frame> element which, like all non-window
+ // load events, propagates up to Document but not Window. The "load" event for
+ // <frameset> on the other hand, like that of <body>, is handled above where it is
+ // fired against Document and directed to Window.
+ final ScriptResult result = frame.fireEvent(event);
if (!isOnbeforeunloadAccepted((HtmlPage) frame.getPage(), event, result)) {
return false;
}
Five tests are as follows:
test_onload.htmlDOMContentLoadedforwindowanddocumentas well as how capturing / bubbling phases are handled.loadforwindowanddocument, and how they relate to theonloadproperty of<body>. Verifies handling of the at target phase.event.eventPhasefor a non-bubbling event after the bubbling phase.test_frame.html<frame>.onloadproperty of a<frameset>catches theloadevent for thewindow.test_click.htmlclickevent) with regards to handling of the capturing / bubbling / at target phases.test_nested_click.htmltest_event_return_value.htmlTwo extra tests are as follows:
test_script_onload.htmltest_img_onload.htmlloadanderrorevents of<script>and<img>EventTarget.fireEvent()rather thanEvent.executeEventLocally().Will work on this - looks like a great improvement
I've made further fixes to
EventTarget.javawhich I'm attaching here.Fixes:
isAttachedvariable.I've changed
test_nested_click.html(attached) to include a test for this. This obsoletestest_click.htmlbecause the tests now overlap.Notes on subtle changes
Documenthas changed fromorg.w3c.dom.Documenttocom.gargoylesoftware.htmlunit.javascript.host.dom.Document.BrowserVersionFeatures.JS_EVENT_WINDOW_EXECUTE_IF_DITACHEDis no longer used because Chrome doesn't seem to behave that way.Test 1: Standard propagation
Test 2: Detached propagation
Have added all your cases as test cases to Window3Test; the expectations are the ones i got when runnung unsing the rela browsers (switching test.properties to real browsers).
Have added all your patches -> please verigy the changes and the tests.
Now the build is not happy, some other test cases are failing - any idea
Woah, that was quick! I'm in the process of downloading trunk snapshot @ r15520 so I can try run the tests.
Btw, how did you do this? I'm doing it by placing a breakpoint after
startWebServer()inloadPage2()and directing Chrome tohttp://localhost:12345. Is there a better way?Or simply you can have a look at he build server https://ci.canoo.com/teamcity/project.html?projectId=HtmlUnit&branch_HtmlUnit=all_branches (Login as Guest)
Oh that's handy..
Okay, I figured out one problem. We had fix from a while back that I hadn't reported here.
It's problem between
<iframe>s andDOMContentLoadedbut our test for it isn't very minimal (it imports jQuery). However, I thinkWindow3Test.onloadFrame()now covers this so I'm pasting the fix only:Have commited you last change also and made some imporvements on the tests. Looks a bit better now.
But our test suite still reports many problems.
Will have a look at some of the test cases also, maybe i can migrate some to WebDriverTestCase and see if the expectations are still true.
But there is this EventTest - At the moment i have no idea why real chrome is different is some tests from the other browsers and in some not. I can't see the difference in the test cases - any idea?
I'm going through them one by one and here's what I've done so far:
This fixes:
EventTest.iframeOnloadEventTest.thisInEventHandlerTypingTest.canSafelyTypeOnElementThatIsRemovedFromTheDomOnKeyPressI thought the test was wrong because testing with Chrome against http://localhost:12345 didn't produce that result in
CHROME.Why you think we should change this?
@Alerts(DEFAULT = "frame1")
public void thisInEventHandler() throws Exception {
When running these test with real browsers i got excactly the original results. Do you get really "frame1" with Chrome?
Strange, I'm using Chrome 68.0.3440.106 and that's how it works here. (Image attached)
However, I noticed if I save the HTML to a file and open that, I don't get "frame1".
I think the difference is timing, and how long it takes Chrome to "load"
about:blank. This is demonstated by putting anonloadon the<iframe>like so:If you change
about:blankto something likehttp://www.example.comto make it slower, I think you'll also get "frame1".Above all, think the purpose of
EventTest.thisInEventHandleris to testthisin an event, and in this regard, timing problem aside, I think HtmlUnit's behaviour can be considered "correct" for Chrome.@RBRi
Here's another fix, this should fix bulk of the
onbeforeunloadproblems:I've changed my mind with regards to above, I think this is better:
@RBRi Thanks for apply the changes.
Some insight:
onloadImgandonloadScriptare NotYetImplemented as they require changes toHtmlImageandHtmlScript.These lines in
onload [IE]andonloadFrame[IE]are the result of IE11 firingloadfor<script>, even though they have nosrc. We can handle this with special branching when we get around to fixingHtmlScript.stopPropagation [IE]: I'm working on a new test case and a fix for this currently.propagationNestedDetached: This is working in my fork so I think there's another fix I have locally that I haven't reported here . I'll try pinpoint what it is.I'm attaching another patch (
fixes.patch.txt) with the following changes:stopPropagation [IE]Event.returnValuewhich is backed byEvent.defaultPrevented.BeforeUnloadEvent.returnValuein Chrome/FF and IE11/Edge.Subtle changes:
JS_CALL_RESULT_IS_LAST_RETURN_VALUEbut I've left the code because deleting it creates too much diff in this patch.EventTarget.fireEvent()'s return value.Test expects:
test_event_return_priority1.html:test_event_return_priority2.htmlTests added and your code is hopefully merged in. But again more tests failing - have i done it wrong?
My bad, this serves me right for not refactoring.
Fixes
1. This one's a straight out bug
Fixes:
BeforeUnloadEvent2Test.returnString2. We really should get rid of
EventTarget.fireEvent()'s return value because it's ambiguous, but it appears to bepublicAPI so I wasn't sure if we can... Anyhow, here's a stop-gap fix that prevents us from doing wholesale refactoring.Fixes:
HtmlRadioButtonInputTest.setChecked3. There's a slight problem with r15529
You can't get rid of
Event.getReturnValue()/Event.setReturnValue()because it's required byEvent.returnValue. (You probably saw that there was no@JsxGetter/@JsxSetterbut it's done by reflection inEvent.initEvent())I like the change to
HtmlPage.javabut I think the change toEvent.javashould be reverted. I need to give you my test forEvent.returnValue.Fixes:
Event2Test.returnPriority4.
Node2Test.eventListener_return_false [IE]is slightly stale (probably pre-IE11) so here's a fixOh, I see now why you got rid of
Event.getReturnValue()/Event.setReturnValue().I'd intended it to be used but I hadn't tested it.
Here's now a test for this (because it's used for Chrome/Edge) and some fixes required to make it work. I'm also attaching the same test as an html file incase you want to try it in a browser.
Have to work on this later...
Btw. working on the migration of HtmlUnit code to github, hope this will make it simpler for you and all the other to provide patches. But this requires time.
Yes!!!
That should definitely lower the hurdle of contributors.
Here's some more fixes:
There was a very subtle diff missing: (Fixes
Window3Test.propagationNestedDetached)Regarding
Event.returnValueI'm on the fence about my previous comment regarding
Event.getReturnValue()/Event.setReturnValue()(sourceforge.net).On one hand, applying that diff gives us a more or less faithful implementation of what Chrome/Edge does with
Event.returnValue.On the other,
EVENT_RETURN_VALUE_IS_PREVENT_DEFAULTis hacky and has a poor feature advantage, especially sinceEvent.returnValueis non-standard and developers are recommeneded to useEvent.preventDefault()anyhow (developer.mozilla.org).I'm fine with just going with what you did in r15529 and removing
EVENT_RETURN_VALUE_IS_PREVENT_DEFAULTaltogether. In that case, these changes are still required:Last edit: Atsushi Nakagawa 2018-08-20
I like what you did in r15531-r15535. It simplifies my hack alot.
Just once thing I noticied, I thought it should be like this:
Because calling
event.preventDefault()changes the value ofevent.returnValue. (The failing test inWindow3Test.stopPropagation [Chrome]tests for this.)FWIW, this also applies to Edge.